[release/7.0-rc1] Fix auto layout algorithm to compute structure alignment correctly (#74091)

* New test

* Fix auto layout algorithm to compute structure alignment correctly - In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8.

- The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8)

In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime.
 - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now.

Fixes #65281

* Re-enable disabled test

* Remove file that shouldn't be added as part of the new test

* Make a few test types public to silence unassigned field errors

* Update comments and add more testing

Co-authored-by: David Wrighton <[email protected]>
Co-authored-by: Tomas Rylek <[email protected]>

Author: 3 people

src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

@@ -469,6 +469,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
 
                 if (IsByValueClass(fieldType))
                 {
+                    // Valuetypes which are not primitives or enums
                     instanceValueClassFieldCount++;
                 }
                 else if (fieldType.IsGCPointer)
@@ -520,25 +521,47 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
                 if (!fieldLayoutAbiStable)
                     layoutAbiStable = false;
 
-                largestAlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequired);
-
                 if (IsByValueClass(fieldType))
                 {
+                    // This block handles valuetypes which are not primitives or enums, it only has a meaningful effect, if the
+                    // type has an alignment greater than pointer size.
+                    largestAlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequired);
                     instanceValueClassFieldsArr[instanceValueClassFieldCount++] = field;
                 }
-                else if (fieldType.IsGCPointer)
-                {
-                    instanceGCPointerFieldsArr[instanceGCPointerFieldsCount++] = field;
-                }
                 else
                 {
-                    int log2size = CalculateLog2(fieldSizeAndAlignment.Size.AsInt);
-                    instanceNonGCPointerFieldsArr[log2size][instanceNonGCPointerFieldsCount[log2size]++] = field;
+                    // non-value-type (and primitive type) fields will add an alignment requirement of pointer size
+                    // This alignment requirement will not be significant in the final alignment calculation unlesss the
+                    // type is greater than the size of a single pointer.
+                    //
+                    // This does not account for types that are marked IsAlign8Candidate due to 8-byte fields
+                    // but that is explicitly handled when we calculate the final alignment for the type.
+
+                    // This behavior is extremely strange for primitive types, as it makes a struct with a single byte in it
+                    // have 8 byte alignment, but that is the current implementation.
+
+                    largestAlignmentRequired = LayoutInt.Max(new LayoutInt(context.Target.PointerSize), largestAlignmentRequired);
+
+                    if (fieldType.IsGCPointer)
+                    {
+                        instanceGCPointerFieldsArr[instanceGCPointerFieldsCount++] = field;
+                    }
+                    else
+                    {
+                        Debug.Assert(fieldType.IsPrimitive || fieldType.IsPointer || fieldType.IsFunctionPointer || fieldType.IsEnum || fieldType.IsByRef);
+                        int log2size = CalculateLog2(fieldSizeAndAlignment.Size.AsInt);
+                        instanceNonGCPointerFieldsArr[log2size][instanceNonGCPointerFieldsCount[log2size]++] = field;
+
+                        if (fieldType.IsPrimitive || fieldType.IsEnum)
+                        {
+                            // Handle alignment of long/ulong/double on ARM32
+                            largestAlignmentRequired = LayoutInt.Max(context.Target.GetObjectAlignment(fieldSizeAndAlignment.Size), largestAlignmentRequired);
+                        }
+                    }
                 }
             }
 
-            largestAlignmentRequired = context.Target.GetObjectAlignment(largestAlignmentRequired);
-            bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && largestAlignmentRequired.AsInt > 4;
+            bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && context.Target.PointerSize == 4 && context.Target.GetObjectAlignment(largestAlignmentRequired).AsInt > 4 && context.Target.PointerSize == 4;
 
             // For types inheriting from another type, field offsets continue on from where they left off
             // Base alignment is not always required, it's only applied when there's a version bubble boundary

src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/ArchitectureSpecificFieldLayoutTests.cs

@@ -414,5 +414,67 @@ public void TestAlignmentBehavior_ShortByteEnumStructAuto()
             Assert.Equal(0x4, tX86FieldStruct.GetField("_struct").Offset.AsInt);
             Assert.Equal(0x4, tARMFieldStruct.GetField("_struct").Offset.AsInt);
         }
+
+        [Fact]
+        public void TestAlignmentBehavior_StructStructByte_StructByteAuto()
+        {
+            string _namespace = "Sequential";
+            string _type = "StructStructByte_StructByteAuto";
+
+            MetadataType tX64 = _testModuleX64.GetType(_namespace, _type);
+            MetadataType tX86 = _testModuleX86.GetType(_namespace, _type);
+            MetadataType tARM = _testModuleARM.GetType(_namespace, _type);
+
+            Assert.Equal(0x1, tX64.InstanceFieldAlignment.AsInt);
+            Assert.Equal(0x1, tARM.InstanceFieldAlignment.AsInt);
+            Assert.Equal(0x1, tX86.InstanceFieldAlignment.AsInt);
+
+            Assert.Equal(0x2, tX64.InstanceFieldSize.AsInt);
+            Assert.Equal(0x2, tARM.InstanceFieldSize.AsInt);
+            Assert.Equal(0x2, tX86.InstanceFieldSize.AsInt);
+
+            Assert.Equal(0x0, tX64.GetField("fld1").Offset.AsInt);
+            Assert.Equal(0x0, tARM.GetField("fld1").Offset.AsInt);
+            Assert.Equal(0x0, tX86.GetField("fld1").Offset.AsInt);
+
+            Assert.Equal(0x1, tX64.GetField("fld2").Offset.AsInt);
+            Assert.Equal(0x1, tARM.GetField("fld2").Offset.AsInt);
+            Assert.Equal(0x1, tX86.GetField("fld2").Offset.AsInt);
+        }
+
+        [Theory]
+        [InlineData("StructStructByte_StructByteAuto", new int[]{1,1,1}, new int[]{2,2,2})]
+        [InlineData("StructStructByte_Struct2BytesAuto", new int[]{2,2,2}, new int[]{4,4,4})]
+        [InlineData("StructStructByte_Struct3BytesAuto", new int[]{4,4,4}, new int[]{8,8,8})]
+        [InlineData("StructStructByte_Struct4BytesAuto", new int[]{4,4,4}, new int[]{8,8,8})]
+        [InlineData("StructStructByte_Struct5BytesAuto", new int[]{8,4,4}, new int[]{16,12,12})]
+        [InlineData("StructStructByte_Struct8BytesAuto", new int[]{8,4,4}, new int[]{16,12,12})]
+        [InlineData("StructStructByte_Struct9BytesAuto", new int[]{8,4,4}, new int[]{24,16,16})]
+        public void TestAlignmentBehavior_AutoAlignmentRules(string wrapperType, int[] alignment, int[] size)
+        {
+            string _namespace = "Sequential";
+            string _type = wrapperType;
+
+            MetadataType tX64 = _testModuleX64.GetType(_namespace, _type);
+            MetadataType tX86 = _testModuleX86.GetType(_namespace, _type);
+            MetadataType tARM = _testModuleARM.GetType(_namespace, _type);
+
+            Assert.Equal(alignment[0], tX64.InstanceFieldAlignment.AsInt);
+            Assert.Equal(alignment[1], tARM.InstanceFieldAlignment.AsInt);
+            Assert.Equal(alignment[2], tX86.InstanceFieldAlignment.AsInt);
+
+            Assert.Equal(size[0], tX64.InstanceFieldSize.AsInt);
+            Assert.Equal(size[1], tARM.InstanceFieldSize.AsInt);
+            Assert.Equal(size[2], tX86.InstanceFieldSize.AsInt);
+
+            Assert.Equal(0x0, tX64.GetField("fld1").Offset.AsInt);
+            Assert.Equal(0x0, tARM.GetField("fld1").Offset.AsInt);
+            Assert.Equal(0x0, tX86.GetField("fld1").Offset.AsInt);
+
+            Assert.Equal(alignment[0], tX64.GetField("fld2").Offset.AsInt);
+            Assert.Equal(alignment[1], tARM.GetField("fld2").Offset.AsInt);
+            Assert.Equal(alignment[2], tX86.GetField("fld2").Offset.AsInt);
+        }
+
     }
 }