Disable Int128 use in by value ABI scenarios, and fix field layout behavior #74123

davidwrighton · 2022-08-18T02:50:06Z

Unfortunately, attempting to actually fix all the ABI issues is probably too complex for this time in the release cycle.

Mark Int128 as being ABI unstable in Crossgen2. This will prevent it from appearing in function signatures
Adjust layout so that crossgen2 and the runtime agree about layout
- Arm32 - Alignment of 8 bytes
- Everywhere else - Alignment of 16 bytes
Disable use of Int128 in pinvokes as a by value parameter or return value. (Match behavior of Vector128<T>) (Unlike Vector128 handle scenarios such as having fields of Int128 types.)
Disable tests that will fail now that pinvokes are disabled
Build a test that will succeed now that pinvokes are disabled
Add test that alignment matches OS behavior
Add unit tests for alignment behavior
Update R2R version

Fixes #72206

- On Unix platforms (64 bit) use 16 byte alignment - On Arm32 use 8 byte alignment matching the 128 byte vector type - On other Windows platforms the 128 bit integer type isn't defined by the C compiler, but match the behavior of other 128 bit types (16 byte alignment) Add tests to call down to native that should pass with these rules - Disable use of Int128 as p/invoke parameter passed by value

- This disables use of these types for parameter passing in R2R images

jkoritzinsky · 2022-08-18T03:55:27Z

If you explicitly mark Int128 as non-blittable, you can then block it in the struct marshalling stubs (and as a result block usage of any struct containing an Int128 in a P/Invoke scenario)

MichalPetryka · 2022-08-18T17:09:31Z

src/tests/Interop/PInvoke/Int128/Int128Test.cs

@@ -25,6 +32,47 @@ unsafe partial class Int128Native
    [DllImport(nameof(Int128Native))]
    public static extern Int128 AddInt128(Int128 lhs, Int128 rhs);

+
+    [DllImport(nameof(Int128Native))]
+    public static extern void AddStructWithInt128_ByRef(ref StructWithInt128 lhs, ref StructWithInt128 rhs);


Should pointers also be checked to make sure the marshaler doesn't change the by-ref marshalling?

I've done some mild testing of pointers. It should work acceptably well.

davidwrighton · 2022-08-19T18:21:44Z

All failures caused by the unix arm queue getting completely overwhelmed

tannergooding · 2022-08-19T18:33:25Z

src/coreclr/vm/methodtablebuilder.cpp

+            // On Windows, no standard for Int128 has been established yet, 
+            // although applying 16 byte alignment is consistent with treatment of 128 bit SSE types
+            // even on X86


Worth noting that the MSVC STL does have a private __int128 that is 16-byte aligned. Clang also uses 16-byte alignment for its own Int128 when targeting the Windows ABI.

Nothing official/documented of course, but it does help justify using 16 on Windows here for managed.

jkoritzinsky

LGTM with a few nits

src/coreclr/vm/classlayoutinfo.cpp

jkoritzinsky · 2022-08-19T18:47:10Z

src/tests/Interop/PInvoke/Int128/Int128TestFieldLayout.csproj

+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <DebugType>embedded</DebugType>


Is this necessary?

The separate test? Yes, as the other test fails, and I'd like to not get rid of it. The DebugType flag, no, but it was just a copy/paste from the other project.

The question was around the DebugType in particular.

Co-authored-by: Jeremy Koritzinsky <[email protected]>

MichalPetryka · 2022-08-19T19:49:50Z

src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/Platform.cs

+    [Intrinsic]
+    [StructLayout(LayoutKind.Sequential)]
+    public readonly struct Int128
+    {
+
+        private readonly ulong _lower;
+        private readonly ulong _upper;
+    }
+
+    [Intrinsic]
+    [StructLayout(LayoutKind.Sequential)]
+    public readonly struct UInt128
+    {
+
+        private readonly ulong _lower;
+        private readonly ulong _upper;
+    }


Suggested change

[Intrinsic]

[StructLayout(LayoutKind.Sequential)]

public readonly struct Int128

{

private readonly ulong _lower;

private readonly ulong _upper;

}

[Intrinsic]

[StructLayout(LayoutKind.Sequential)]

public readonly struct UInt128

{

private readonly ulong _lower;

private readonly ulong _upper;

}

[Intrinsic]

[StructLayout(LayoutKind.Sequential)]

public readonly struct Int128

{

private readonly ulong _lower;

private readonly ulong _upper;

}

[Intrinsic]

[StructLayout(LayoutKind.Sequential)]

public readonly struct UInt128

{

private readonly ulong _lower;

private readonly ulong _upper;

}

davidwrighton · 2022-08-19T22:41:55Z

The failures here are all arm64 issues hit by the arm helix queues falling over today.

davidwrighton · 2022-08-19T22:43:07Z

/backport to release/7.0-rc1

github-actions · 2022-08-19T22:43:23Z

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2892554223

dotnet#74123 broke compiling TechEmpower benchmarks with NativeAOT. We now crash with: ``` > ILCompiler.TypeSystem.dll!Internal.TypeSystem.RuntimeDeterminedFieldLayoutAlgorithm.ComputeInstanceLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 19 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeInstanceLayout(Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 436 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.IsInt128OrHasInt128Fields.get() Line 150 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeAutoFieldLayout(Internal.TypeSystem.MetadataType type, int numInstanceFields) Line 483 C# ILCompiler.Compiler.dll!ILCompiler.CompilerMetadataFieldLayoutAlgorithm.ComputeInstanceFieldLayout(Internal.TypeSystem.MetadataType type, int numInstanceFields) Line 56 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeInstanceLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 164 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeInstanceLayout(Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 436 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.InstanceFieldSize.get() Line 165 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeFieldSizeAndAlignment(Internal.TypeSystem.TypeDesc fieldType, bool hasLayout, int packingSize, out bool layoutAbiStable, out bool fieldTypeHasAutoLayout, out bool fieldTypeHasInt128Field) Line 838 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeStaticFieldLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.StaticLayoutKind layoutKind) Line 215 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeStaticFieldLayout(Internal.TypeSystem.StaticLayoutKind layoutKind) Line 473 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.GCStaticFieldSize.get() Line 302 C# ILCompiler.Compiler.dll!ILCompiler.DependencyAnalysis.NativeLayoutTemplateTypeLayoutVertexNode.GetStaticDependencies(ILCompiler.DependencyAnalysis.NodeFactory context) Line 997 C# ``` https://github.com/dotnet/runtime/blob/4cf1383c8458945b7eb27ae5f57338c10ed25d54/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs#L17-L19 I was not able to come up with a standalone repro case, but this fixes compiling the benchmark. I'm not clear why native layout operates on runtime determined forms, but what I'm doing here should have equivalent behaviors, except avoiding the problem that we can no longer compute layouts of runtime determined things in some obscure scenarios due to the new code.

#74123 broke compiling TechEmpower benchmarks with NativeAOT. We now crash with: ``` > ILCompiler.TypeSystem.dll!Internal.TypeSystem.RuntimeDeterminedFieldLayoutAlgorithm.ComputeInstanceLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 19 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeInstanceLayout(Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 436 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.IsInt128OrHasInt128Fields.get() Line 150 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeAutoFieldLayout(Internal.TypeSystem.MetadataType type, int numInstanceFields) Line 483 C# ILCompiler.Compiler.dll!ILCompiler.CompilerMetadataFieldLayoutAlgorithm.ComputeInstanceFieldLayout(Internal.TypeSystem.MetadataType type, int numInstanceFields) Line 56 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeInstanceLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 164 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeInstanceLayout(Internal.TypeSystem.InstanceLayoutKind layoutKind) Line 436 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.InstanceFieldSize.get() Line 165 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeFieldSizeAndAlignment(Internal.TypeSystem.TypeDesc fieldType, bool hasLayout, int packingSize, out bool layoutAbiStable, out bool fieldTypeHasAutoLayout, out bool fieldTypeHasInt128Field) Line 838 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataFieldLayoutAlgorithm.ComputeStaticFieldLayout(Internal.TypeSystem.DefType defType, Internal.TypeSystem.StaticLayoutKind layoutKind) Line 215 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.ComputeStaticFieldLayout(Internal.TypeSystem.StaticLayoutKind layoutKind) Line 473 C# ILCompiler.TypeSystem.dll!Internal.TypeSystem.DefType.GCStaticFieldSize.get() Line 302 C# ILCompiler.Compiler.dll!ILCompiler.DependencyAnalysis.NativeLayoutTemplateTypeLayoutVertexNode.GetStaticDependencies(ILCompiler.DependencyAnalysis.NodeFactory context) Line 997 C# ``` https://github.com/dotnet/runtime/blob/4cf1383c8458945b7eb27ae5f57338c10ed25d54/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs#L17-L19 I was not able to come up with a standalone repro case, but this fixes compiling the benchmark. I'm not clear why native layout operates on runtime determined forms, but what I'm doing here should have equivalent behaviors, except avoiding the problem that we can no longer compute layouts of runtime determined things in some obscure scenarios due to the new code.

davidwrighton added 7 commits August 17, 2022 19:44

First stab at support for proper 128bit integer layout and abi

29ea888

Add ABI tests for Int128 covering interesting scenarios

ca40e03

Fix bugs so that at least Windows Arm64 works

183a36e

Add more types to the ABI tester, so that we cover the Int128 scenarios

823090b

Revert changes which attempted to enable by value passing for Int128

22adce8

Mark Int128 types as not having a stable abi

867ab2e

- This disables use of these types for parameter passing in R2R images

dotnet-issue-labeler bot added the area-VM-coreclr label Aug 18, 2022

ghost assigned davidwrighton Aug 18, 2022

MichalPetryka reviewed Aug 18, 2022

View reviewed changes

davidwrighton mentioned this pull request Aug 19, 2022

ABI for Int128 and UInt128 is incorrect with regards to native ABI for by value parameter passing #74209

Open

davidwrighton added 3 commits August 18, 2022 18:46

Address all known issues

8585002

Merge branch 'main' of github.com:dotnet/runtime into fix_72206_2

fdba562

Try to fix PR job

4fab4d3

davidwrighton mentioned this pull request Aug 19, 2022

Mono does not align Int128 types on correct boundaries when p/invoking or passing pointers to them to unmanaged code #74223

Closed

Should fix the test issues

1170afe

davidwrighton marked this pull request as ready for review August 19, 2022 07:50

davidwrighton requested a review from MichalStrehovsky as a code owner August 19, 2022 07:50

davidwrighton requested review from jkoritzinsky and tannergooding August 19, 2022 08:08

tannergooding reviewed Aug 19, 2022

View reviewed changes

tannergooding approved these changes Aug 19, 2022

View reviewed changes

tannergooding mentioned this pull request Aug 19, 2022

Ensure Vector64/128/256 are blocked for pass-by-value to P/Invoke in all scenarios #74269

Open

2 tasks

jkoritzinsky approved these changes Aug 19, 2022

View reviewed changes

Apply suggestions from code review

81c4439

Co-authored-by: Jeremy Koritzinsky <[email protected]>

MichalPetryka reviewed Aug 19, 2022

View reviewed changes

davidwrighton merged commit e6b5e67 into dotnet:main Aug 19, 2022

github-actions bot mentioned this pull request Aug 19, 2022

[release/7.0-rc1] Disable Int128 use in by value ABI scenarios, and fix field layout behavior #74279

Merged

MichalStrehovsky mentioned this pull request Aug 25, 2022

Int128TestFieldLayout test failing on NativeAOT #74549

Closed

MichalStrehovsky mentioned this pull request Sep 16, 2022

Do not request type layout for RuntimeDetermined types #75746

Merged

ghost locked as resolved and limited conversation to collaborators Sep 19, 2022

davidwrighton deleted the fix_72206_2 branch April 13, 2023 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable Int128 use in by value ABI scenarios, and fix field layout behavior #74123

Disable Int128 use in by value ABI scenarios, and fix field layout behavior #74123

davidwrighton commented Aug 18, 2022 •

edited

Loading

jkoritzinsky commented Aug 18, 2022

MichalPetryka Aug 18, 2022

davidwrighton Aug 19, 2022

davidwrighton commented Aug 19, 2022

tannergooding Aug 19, 2022

jkoritzinsky left a comment

jkoritzinsky Aug 19, 2022

davidwrighton Aug 19, 2022

jkoritzinsky Aug 19, 2022

MichalPetryka Aug 19, 2022

davidwrighton commented Aug 19, 2022

davidwrighton commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

Disable Int128 use in by value ABI scenarios, and fix field layout behavior #74123

Disable Int128 use in by value ABI scenarios, and fix field layout behavior #74123

Conversation

davidwrighton commented Aug 18, 2022 • edited Loading

jkoritzinsky commented Aug 18, 2022

MichalPetryka Aug 18, 2022

Choose a reason for hiding this comment

davidwrighton Aug 19, 2022

Choose a reason for hiding this comment

davidwrighton commented Aug 19, 2022

tannergooding Aug 19, 2022

Choose a reason for hiding this comment

jkoritzinsky left a comment

Choose a reason for hiding this comment

jkoritzinsky Aug 19, 2022

Choose a reason for hiding this comment

davidwrighton Aug 19, 2022

Choose a reason for hiding this comment

jkoritzinsky Aug 19, 2022

Choose a reason for hiding this comment

MichalPetryka Aug 19, 2022

Choose a reason for hiding this comment

davidwrighton commented Aug 19, 2022

davidwrighton commented Aug 19, 2022

github-actions bot commented Aug 19, 2022

davidwrighton commented Aug 18, 2022 •

edited

Loading