RuntimeHelpers.CreateSpan support is failing in some circumstances

[stephentoub]

dotnet/roslyn#61414 updated Roslyn with the ability to target RuntimeHelpers.CreateSpan. #79461 updates dotnet/runtime to pull in a newer version of Roslyn that contains the improvements and updates a bunch of code to take advantage of CreateSpan. But CI for that is failing due to ArgumentExceptions coming from CreateSpan due to what appears to be alignment issues. @davidwrighton is investigating, and I'm opening this to track, as we may need to back the changes out of Roslyn if it's an issue with the runtime or linker...

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[stephentoub]

There's at least one issue here in the Roslyn support. When the data is the 2/4/8 bytes in size, today it uses the built-in system types (short, int, long), but we shouldn't be doing so when the alignment is > 1, as those types have a .pack of 1, and thus a rewriter may not align the resulting data correctly. EDIT: These were fixed in dotnet/roslyn#65919.

It appears there may still be an additional issue beyond that, however, though I've only been able to repro the impact of the above locally.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

---

dotnet/roslyn#61414 updated Roslyn with the ability to target RuntimeHelpers.CreateSpan. #79461 updates dotnet/runtime to pull in a newer version of Roslyn that contains the improvements and updates a bunch of code to take advantage of CreateSpan. But CI for that is failing due to ArgumentExceptions coming from CreateSpan due to what appears to be alignment issues. @davidwrighton is investigating, and I'm opening this to track, as we may need to back the changes out of Roslyn if it's an issue with the runtime or linker...

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-System.Runtime.CompilerServices
Milestone:	-

[MichalStrehovsky]

There's at least one issue here in the Roslyn support. When the data is the 2/4/8 bytes in size, today it uses the built-in system types (short, int, long), but we shouldn't be doing so when the alignment is > 1, as those types have a .pack of 1, and thus a rewriter may not align the resulting data correctly.

We really should enforce this in the runtime and not wait for IL rewriters to break this - it was an unadressed piece of feedback when this was introduced: #63977 (comment). Roslyn using primitive types was specifically my example there :).

[stephentoub]

@sbomer, seems likely there's a linker bug here. Some of the legs in #79461 are failing catastrophically with errors like:

      System.ArgumentException : Value does not fall within the expected range.
      Stack Trace:
           at System.Runtime.CompilerServices.RuntimeHelpers.GetSpanDataFrom(RuntimeFieldHandle fldHandle, RuntimeTypeHandle targetTypeHandle, Int32& count)
        /_/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs(100,0): at System.Runtime.CompilerServices.RuntimeHelpers.CreateSpan[T](RuntimeFieldHandle fldHandle)
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs(140,0): at System.Decimal.DecCalc.get_DoublePowers10()
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs(1732,0): at System.Decimal.DecCalc.VarDecFromR8(Double input, DecCalc& result)
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.cs(195,0): at System.Decimal..ctor(Double value)
        /_/src/libraries/System.Private.CoreLib/src/System/Decimal.cs(935,0): at System.Decimal.op_Explicit(Double value)

I've attached the System.Private.CoreLib.zip binary from the failing Libraries Test Run checked coreclr windows x86 Debug leg (one of two checked legs hitting this widespread). The relevant field is:

.field /* 040021ED */ assembly static initonly valuetype '<PrivateImplementationDetails>'/'__StaticArrayInitTypeSize=648_Align=8' '67856A16DB0550FDAB4D1A9B208B0C155C4679CA116BF867B74ED2A0AA4D29558' at I_001B881C

Note the address 001B881C, which is not 8-byte aligned. But the associated type does have a .pack of 8:

.class /* 0200099D */ nested private explicit ansi sealed '__StaticArrayInitTypeSize=648_Align=8'
	extends System.ValueType
{
	.pack 8
	.size 648

} // end of class __StaticArrayInitTypeSize=648_Align=8

I can repro this locally. With the https://github.com/stephentoub/runtime/tree/usecreatespan branch, if I build with:

.\build.cmd clr.corelib -c checked -arch x86

I get the same erroneously-aligned address. But if I then edit the .\src\coreclr\System.Private.CoreLib\System.Private.CoreLib.csproj file to change the:

<ILLinkTrimAssembly>true</ILLinkTrimAssembly>

to:

<ILLinkTrimAssembly>false</ILLinkTrimAssembly>

I then get an 8-byte-aligned address of 004602C0, which makes sense given Roslyn 8-byte aligns every RVA field.

Can you take a look?

cc: @davidwrighton, @jcouv

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

dotnet/roslyn#61414 updated Roslyn with the ability to target RuntimeHelpers.CreateSpan. #79461 updates dotnet/runtime to pull in a newer version of Roslyn that contains the improvements and updates a bunch of code to take advantage of CreateSpan. But CI for that is failing due to ArgumentExceptions coming from CreateSpan due to what appears to be alignment issues. @davidwrighton is investigating, and I'm opening this to track, as we may need to back the changes out of Roslyn if it's an issue with the runtime or linker...

Author:	stephentoub
Assignees:	stephentoub
Labels:	area-Tools-ILLink
Milestone:	8.0.0

[vitek-karas]

This is a bug in Cecil. David's change to Cecil correctly sets the alignment for each field data blob and the minimum alignment necessary for the entire data blob. But when Cecil computes positions of each of the blobs, it incorrectly applies the alignment to the length of the blob, not to its position.
Since this is x86 the default alignment for data blobs in Cecil is 4, and in this case it just so happens that the position ends up being aligned to only 4, not 8.

I think the bug is basically here: https://github.com/dotnet/cecil/blob/49215988d4404e2c0befe697ce37a3f38e99953e/Mono.Cecil.PE/TextMap.cs#L54

It applies the alignment to the length, not position - it should probably do both.

This affects only x86, on x64 the position will be aligned to 8 at least.
BTW: This theoretically affects code (IL) as well - on x64 the code alignment should be 16, but it's not applied to the start of the blob. I actually think that the position will end up being at offset 72, which is not 16 aligned, but I guess nobody really cares about the alignment of IL for this to break anything.

[stephentoub]

Thanks!

This affects only x86, on x64 the position will be aligned to 8 at least.

Tests are failing in a similar way on arm64, e.g.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=111650&view=ms.vss-test-web.build-test-results-tab

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-79461-merge-a9e1343cc9d343999c/Microsoft.Extensions.Configuration.CommandLine.Tests/1/console.1205db0d.log?helixlogtype=result

[jkotas]

This theoretically affects code (IL) as well - on x64 the code alignment should be 16

IL blobs do not need to be aligned to more than 4.

[jkotas]

This is a bug in Cecil

We may need to push this bug fix to servicing. This bug fix needs to be present in all places where the new Roslyn or packages built with new Roslyn can meet with ILLinker.

[vitek-karas]

IL blobs do not need to be aligned to more than 4.

It's hard to tell if the 16 align is because of the code blob, or the following Resources blob. But honestly given the behavior of the alignment I would not be surprised if it was broken in subtle ways in more than one place.

[stephentoub]

We may need to push this bug fix to servicing.

sbomer closed this as completed in https://github.com/dotnet/cecil/pull/55[last week](#79477 (comment))

Can we backport the fix for this to release/7.0? RuntimeHelpers.CreateSpan shipped in .NET 7, and a Roslyn compiler that lights up when it sees CreateSpan will be shipping supported soon, which means anyone who has the combination of conditions that can lead to this may immediately start seeing failures upon recompilation. I believe those conditions are the combination of:

• Using a new enough C# compiler to have the CreateSpan-based support. The C# compiler ships much more frequently than does dotnet/runtime.
• Using illink.
• Targeting coreclr.
• Casting (implicit or explicit) a new T[] { const, ... } to a ReadOnlySpan<T>, where the element type is long or ulong.
• Unluckiness with how things end up aligning

[sbomer]

The fix was backported to 7.0 in dotnet/cecil#61.