From 29b5d98fc0c810b30086229d9a1d3eb91d8f5778 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Tue, 16 Aug 2022 01:31:32 +0200 Subject: [PATCH 1/3] Fix one more Crossgen2 field layout mismatch with runtime The field offset mismatch in the test JIT/Regression/JitBlue/Runtime_60035/Runtime_60035.csproj specific to Crossgen2 composite mode is due to the recent proliferation of 16-byte alignment specific to Vector across the runtime repo. Previously, the largest supported alignment was 8 and X86 was the only architecture not respecting it. In general the problem is caused by the fact that the native CoreCLR runtime method table builder calculates field offsets without taking the method table pointer into account; in the particular case of the Runtime_60035 test, the class System.Text.Encodings.Web.OptimizedInboxTextEncoder has a field named _allowedAsciiCodePoints that is 16-aligned that exposes this inconsistency. Thanks Tomas --- .../Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 9d291d8c372d69..5a76d71bc204f3 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -580,7 +580,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, // between base type and the current type. LayoutInt cumulativeInstanceFieldPos = CalculateFieldBaseOffset(type, requiresAlign8, requiresAlignedBase: false); LayoutInt offsetBias = LayoutInt.Zero; - if (!type.IsValueType && cumulativeInstanceFieldPos != LayoutInt.Zero && type.Context.Target.Architecture == TargetArchitecture.X86) + if (!type.IsValueType && cumulativeInstanceFieldPos != LayoutInt.Zero && type.Context.Target.Architecture != TargetArchitecture.ARM) { offsetBias = type.Context.Target.LayoutPointerSize; cumulativeInstanceFieldPos -= offsetBias; From 21049f4a899202e0d0c91eed71676a95261899a8 Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Fri, 19 Aug 2022 23:35:45 +0200 Subject: [PATCH 2/3] Address David's PR feedback --- .../Common/MetadataFieldLayoutAlgorithm.cs | 9 ++++ .../CoreTestAssembly/InstanceFieldLayout.cs | 13 ++++++ .../InstanceFieldLayoutTests.cs | 44 +++++++++++++++++++ .../UniversalGenericFieldLayoutTests.cs | 2 + 4 files changed, 68 insertions(+) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 5a76d71bc204f3..5ff6417e52e411 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -580,6 +580,15 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, // between base type and the current type. LayoutInt cumulativeInstanceFieldPos = CalculateFieldBaseOffset(type, requiresAlign8, requiresAlignedBase: false); LayoutInt offsetBias = LayoutInt.Zero; + + // The following conditional statement mimics the behavior of MethodTableBuilder::PlaceInstanceFields; + // the fundamental difference between CoreCLR native runtime and Crossgen2 regarding field placement is + // that the native runtime doesn't count the method table pointer at the beginning of reference types as a 'field' + // so that the first field in a class has offset 0 while its 'real' offset from the 'this' pointer is LayoutPointerSize. + // On ARM32, native runtime employs a special logic internally calculating the field offsets relative to the 'this' + // pointer (the Crossgen2 way) to ensure 8-alignment for longs and doubles as required by the ARM32 ISA. Please note + // that for 16-alignment used by Vector128 this logic actually ensures that the fields are 16-misaligned + // (they are 16-aligned after the 4-byte or 8-byte method table pointer). if (!type.IsValueType && cumulativeInstanceFieldPos != LayoutInt.Zero && type.Context.Target.Architecture != TargetArchitecture.ARM) { offsetBias = type.Context.Target.LayoutPointerSize; diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs index 92446b5770ef94..abf093055dc309 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs @@ -224,6 +224,12 @@ public struct StructStructByte_UInt128StructAuto public StructByte fld1; public Auto.UInt128Struct fld2; } + + [StructLayout(LayoutKind.Sequential)] + public class Class16Align + { + Vector128 vector16Align; + } } namespace Auto @@ -430,6 +436,7 @@ public struct Struct9Bytes public byte fld9; } +<<<<<<< HEAD [StructLayout(LayoutKind.Auto)] public struct UInt128Struct { @@ -440,6 +447,12 @@ public struct UInt128Struct public struct Int128Struct { Int128 fld1; +======= + [StructLayout(LayoutKind.Sequential)] + public class Class16Align + { + Vector128 vector16Align; +>>>>>>> 7f70d973821 (Address David's PR feedback) } } diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/InstanceFieldLayoutTests.cs b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/InstanceFieldLayoutTests.cs index 4b02f805f3535b..083a4e65af3844 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/InstanceFieldLayoutTests.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/InstanceFieldLayoutTests.cs @@ -281,6 +281,28 @@ public void TestSequentialTypeLayoutStructEmbedded() } } + [Fact] + public void TestSequentialTypeLayoutClass16Align() + { + MetadataType classType = _testModule.GetType("Sequential", "Class16Align"); + Assert.Equal(0x18, classType.InstanceByteCount.AsInt); + foreach (var f in classType.GetFields()) + { + if (f.IsStatic) + continue; + + switch (f.Name) + { + case "vector16Align": + Assert.Equal(0x8, f.Offset.AsInt); + break; + default: + Assert.True(false); + break; + } + } + } + [Fact] public void TestAutoLayoutStruct() { @@ -782,6 +804,28 @@ public void TestAutoTypeLayoutMinPacking(WellKnownType type, int expectedSize) Assert.Equal(expectedSize, inst.InstanceFieldSize.AsInt); } + [Fact] + public void TestAutoTypeLayoutClass16Align() + { + MetadataType classType = _testModule.GetType("Auto", "Class16Align"); + Assert.Equal(0x18, classType.InstanceByteCount.AsInt); + foreach (var f in classType.GetFields()) + { + if (f.IsStatic) + continue; + + switch (f.Name) + { + case "vector16Align": + Assert.Equal(0x8, f.Offset.AsInt); + break; + default: + Assert.True(false); + break; + } + } + } + [Fact] public void TestTypeContainsGCPointers() { diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/UniversalGenericFieldLayoutTests.cs b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/UniversalGenericFieldLayoutTests.cs index 541361160f2b58..6b81314b9f9765 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/UniversalGenericFieldLayoutTests.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/UniversalGenericFieldLayoutTests.cs @@ -372,6 +372,7 @@ private void CommonClassLayoutTestBits(ModuleDesc testModule, AssertClassIndeterminateSize(context, genOfUL, expectedIndeterminateByteAlignment); } + /* This test exercises universal shared generic layout that is currently unsupported and known to be buggy. [Fact] public void TestClassLayout() { @@ -511,5 +512,6 @@ public void TestClassLayout() Assert.Equal(LayoutInt.Indeterminate, genOfUI.GetFields().First().Offset); Assert.Equal(LayoutInt.Indeterminate, genOfUL.GetFields().First().Offset); } + */ } } From c70454c38aa8ce5a90a1070b21c7fa937a18fb4b Mon Sep 17 00:00:00 2001 From: Tomas Rylek Date: Sat, 20 Aug 2022 01:42:00 +0200 Subject: [PATCH 3/3] Resolve previously missed merge conflict --- .../CoreTestAssembly/InstanceFieldLayout.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs index abf093055dc309..249090e38eae51 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs @@ -436,7 +436,6 @@ public struct Struct9Bytes public byte fld9; } -<<<<<<< HEAD [StructLayout(LayoutKind.Auto)] public struct UInt128Struct { @@ -447,12 +446,12 @@ public struct UInt128Struct public struct Int128Struct { Int128 fld1; -======= + } + [StructLayout(LayoutKind.Sequential)] public class Class16Align { Vector128 vector16Align; ->>>>>>> 7f70d973821 (Address David's PR feedback) } }