From 948d73b7a785cd952fc02124917fa383695c0d76 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Sat, 18 Jan 2025 21:15:26 -0800 Subject: [PATCH 01/16] Fix explicit offset of ByRefLike fields. A ByRefLike type was previously forced to be pointer aligned so ref fields were implicitly aligned. Using Explicit layout one can create valid aligned ByRefLike types with no object pointers. This change propagates the base offset of the ByRefLike type and includes that for determining offsets of fields in the ByRefLike type. --- .../Common/MetadataFieldLayoutAlgorithm.cs | 2 - src/coreclr/vm/methodtablebuilder.cpp | 38 +++++----- src/coreclr/vm/methodtablebuilder.h | 6 +- .../objrefandnonobjrefoverlap/case13.cs | 71 +++++++++++++++++++ 4 files changed, 95 insertions(+), 22 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 4bd6a3cb7a952b..b76c850e1544d2 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -329,13 +329,11 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty LayoutInt computedOffset = fieldAndOffset.Offset + cumulativeInstanceFieldPos + offsetBias; // GC pointers MUST be aligned. - // We treat byref-like structs as GC pointers too. bool needsToBeAligned = !computedOffset.IsIndeterminate && ( fieldType.IsGCPointer - || fieldType.IsByRefLike || (fieldType.IsValueType && ((DefType)fieldType).ContainsGCPointers) ); if (needsToBeAligned) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index 60110263e4a274..c520d4adc8e14c 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8677,30 +8677,28 @@ MethodTableBuilder::HandleExplicitLayout( else { MethodTable *pByValueMT = pByValueClassCache[valueClassCacheIndex]; - if (pByValueMT->IsByRefLike() || pByValueMT->ContainsGCPointers()) + BOOL hasGcPointers = pByValueMT->ContainsGCPointers(); + if (hasGcPointers || pByValueMT->IsByRefLike()) { - if ((pFD->GetOffset() & ((ULONG)TARGET_POINTER_SIZE - 1)) != 0) + if (hasGcPointers && ((pFD->GetOffset() & ((ULONG)TARGET_POINTER_SIZE - 1)) != 0)) { - // If we got here, then a ByRefLike valuetype or a valuetype containing an OREF was misaligned. + // If we got here, then a valuetype containing an OREF was misaligned. badOffset = pFD->GetOffset(); fieldTrust.SetTrust(ExplicitFieldTrust::kNone); break; } - ExplicitFieldTrust::TrustLevel trust = CheckValueClassLayout(pByValueMT, &pFieldLayout[pFD->GetOffset()]); + ExplicitFieldTrust::TrustLevel trust = CheckValueClassLayout(pByValueMT, &pFieldLayout[pFD->GetOffset()], pFD->GetOffset()); fieldTrust.SetTrust(trust); - if (trust != ExplicitFieldTrust::kNone) - { - continue; - } - else + if (trust == ExplicitFieldTrust::kNone) { // If we got here, then an OREF/BYREF inside the valuetype illegally overlapped a non-OREF field. THROW. badOffset = pFD->GetOffset(); break; } - break; + + continue; } // no pointers so fall through to do standard checking fieldSize = pByValueMT->GetNumInstanceFieldBytes(); @@ -8855,13 +8853,13 @@ MethodTableBuilder::HandleExplicitLayout( //******************************************************************************* // make sure that no object fields are overlapped incorrectly, returns the trust level -/*static*/ ExplicitFieldTrust::TrustLevel MethodTableBuilder::CheckValueClassLayout(MethodTable * pMT, bmtFieldLayoutTag *pFieldLayout) +/*static*/ ExplicitFieldTrust::TrustLevel MethodTableBuilder::CheckValueClassLayout(MethodTable * pMT, bmtFieldLayoutTag *pFieldLayout, UINT fieldBaseOffset) { STANDARD_VM_CONTRACT; // ByRefLike types need to be checked for ByRef fields. if (pMT->IsByRefLike()) - return CheckByRefLikeValueClassLayout(pMT, pFieldLayout); + return CheckByRefLikeValueClassLayout(pMT, pFieldLayout, fieldBaseOffset); // Build a layout of the value class (vc). Don't know the sizes of all the fields easily, but // do know (a) vc is already consistent so don't need to check it's overlaps and @@ -8947,7 +8945,7 @@ MethodTableBuilder::HandleExplicitLayout( //******************************************************************************* // make sure that no byref/object fields are overlapped, returns the trust level -/*static*/ ExplicitFieldTrust::TrustLevel MethodTableBuilder::CheckByRefLikeValueClassLayout(MethodTable * pMT, bmtFieldLayoutTag *pFieldLayout) +/*static*/ ExplicitFieldTrust::TrustLevel MethodTableBuilder::CheckByRefLikeValueClassLayout(MethodTable * pMT, bmtFieldLayoutTag *pFieldLayout, UINT fieldBaseOffset) { STANDARD_VM_CONTRACT; _ASSERTE(pMT->IsByRefLike()); @@ -8964,17 +8962,21 @@ MethodTableBuilder::HandleExplicitLayout( if (pFD->GetFieldType() == ELEMENT_TYPE_VALUETYPE) { MethodTable *pFieldMT = pFD->GetApproxFieldTypeHandleThrowing().AsMethodTable(); - trust = CheckValueClassLayout(pFieldMT, &pFieldLayout[fieldStartIndex]); + trust = CheckValueClassLayout(pFieldMT, &pFieldLayout[fieldStartIndex], fieldBaseOffset + fieldStartIndex); } else if (pFD->IsObjRef()) { - _ASSERTE(fieldStartIndex % TARGET_POINTER_SIZE == 0); - trust = MarkTagType(&pFieldLayout[fieldStartIndex], TARGET_POINTER_SIZE, oref); + UINT absOffset = fieldBaseOffset + fieldStartIndex; + trust = (absOffset % TARGET_POINTER_SIZE == 0) + ? MarkTagType(&pFieldLayout[fieldStartIndex], TARGET_POINTER_SIZE, oref) + : ExplicitFieldTrust::kNone; } else if (pFD->IsByRef()) { - _ASSERTE(fieldStartIndex % TARGET_POINTER_SIZE == 0); - trust = MarkTagType(&pFieldLayout[fieldStartIndex], TARGET_POINTER_SIZE, byref); + UINT absOffset = fieldBaseOffset + fieldStartIndex; + trust = (absOffset % TARGET_POINTER_SIZE == 0) + ? MarkTagType(&pFieldLayout[fieldStartIndex], TARGET_POINTER_SIZE, byref) + : ExplicitFieldTrust::kNone; } else { diff --git a/src/coreclr/vm/methodtablebuilder.h b/src/coreclr/vm/methodtablebuilder.h index 485161300eb465..a361608bd66f5d 100644 --- a/src/coreclr/vm/methodtablebuilder.h +++ b/src/coreclr/vm/methodtablebuilder.h @@ -2878,11 +2878,13 @@ class MethodTableBuilder static ExplicitFieldTrust::TrustLevel CheckValueClassLayout( MethodTable * pMT, - bmtFieldLayoutTag* pFieldLayout); + bmtFieldLayoutTag* pFieldLayout, + UINT fieldBaseOffset); static ExplicitFieldTrust::TrustLevel CheckByRefLikeValueClassLayout( MethodTable * pMT, - bmtFieldLayoutTag* pFieldLayout); + bmtFieldLayoutTag* pFieldLayout, + UINT fieldBaseOffset); static ExplicitFieldTrust::TrustLevel MarkTagType( bmtFieldLayoutTag* field, diff --git a/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs b/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs index 99ca59fdd47e7a..cb8dbcd92fbc88 100644 --- a/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs +++ b/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs @@ -54,6 +54,20 @@ public ref struct Inner3 } } + [StructLayout(LayoutKind.Explicit)] + public ref struct Explicit4 + { + [FieldOffset(0)] + public Size4Bytes Field1; + [FieldOffset(4)] + public Size4Bytes Field2; + + public ref struct Size4Bytes + { + public uint Value; + } + } + [Fact] public static void Validate_Explicit1() { @@ -89,4 +103,61 @@ static string Load3() return typeof(Explicit3).ToString(); } } + + [Fact] + public static void Validate_Explicit4() + { + Load4(); + + [MethodImpl(MethodImplOptions.NoInlining)] + static string Load4() + { + return typeof(Explicit4).ToString(); + } + } + + // Invalid. Explicit offset on second field is invalid + // since first field on type will be misaligned. + [StructLayout(LayoutKind.Explicit)] + public ref struct Explicit_Invalid32bit + { + [FieldOffset(0)] + public WithByRefs Field1; + [FieldOffset(2)] + public WithByRefs Field2; + } + + [StructLayout(LayoutKind.Explicit)] + public ref struct Explicit_Invalid64bit + { + [FieldOffset(0)] + public WithByRefs Field1; + [FieldOffset(4)] + public WithByRefs Field2; + } + + [Fact] + public static void Validate_Throws_Explicit() + { + if (Environment.Is64BitProcess) + { + Assert.Throws(() => Load64()); + } + else + { + Assert.Throws(() => Load32()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static string Load32() + { + return typeof(Explicit_Invalid32bit).ToString(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static string Load64() + { + return typeof(Explicit_Invalid64bit).ToString(); + } + } } From b0cda63e77d8e3b696127e03dfb63e6627312f9f Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Sat, 18 Jan 2025 21:37:06 -0800 Subject: [PATCH 02/16] Improve tests coverage --- .../objrefandnonobjrefoverlap/case13.cs | 62 ++++++++++++++----- 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs b/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs index cb8dbcd92fbc88..b45e3a72d1e0d3 100644 --- a/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs +++ b/src/tests/Loader/classloader/explicitlayout/objrefandnonobjrefoverlap/case13.cs @@ -54,17 +54,17 @@ public ref struct Inner3 } } - [StructLayout(LayoutKind.Explicit)] + [StructLayout(LayoutKind.Explicit, Size = 2)] public ref struct Explicit4 { [FieldOffset(0)] - public Size4Bytes Field1; - [FieldOffset(4)] - public Size4Bytes Field2; + public Size1Byte Field1; + [FieldOffset(1)] + public Size1Byte Field2; - public ref struct Size4Bytes + public ref struct Size1Byte { - public uint Value; + public byte Value; } } @@ -119,7 +119,7 @@ static string Load4() // Invalid. Explicit offset on second field is invalid // since first field on type will be misaligned. [StructLayout(LayoutKind.Explicit)] - public ref struct Explicit_Invalid32bit + public ref struct Explicit5a_Invalid32 { [FieldOffset(0)] public WithByRefs Field1; @@ -128,7 +128,16 @@ public ref struct Explicit_Invalid32bit } [StructLayout(LayoutKind.Explicit)] - public ref struct Explicit_Invalid64bit + public ref struct Explicit5b_Invalid32 + { + [FieldOffset(0)] + public WithORefs Field1; + [FieldOffset(2)] + public WithORefs Field2; + } + + [StructLayout(LayoutKind.Explicit)] + public ref struct Explicit5a_Invalid64 { [FieldOffset(0)] public WithByRefs Field1; @@ -136,28 +145,51 @@ public ref struct Explicit_Invalid64bit public WithByRefs Field2; } + [StructLayout(LayoutKind.Explicit)] + public ref struct Explicit5b_Invalid64 + { + [FieldOffset(0)] + public WithORefs Field1; + [FieldOffset(4)] + public WithORefs Field2; + } + [Fact] - public static void Validate_Throws_Explicit() + public static void Validate_Explicit5_Invalid() { if (Environment.Is64BitProcess) { - Assert.Throws(() => Load64()); + Assert.Throws(() => LoadA64()); + Assert.Throws(() => LoadB64()); } else { - Assert.Throws(() => Load32()); + Assert.Throws(() => LoadA32()); + Assert.Throws(() => LoadB32()); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static string LoadA32() + { + return typeof(Explicit5a_Invalid32).ToString(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static string LoadB32() + { + return typeof(Explicit5b_Invalid32).ToString(); } [MethodImpl(MethodImplOptions.NoInlining)] - static string Load32() + static string LoadA64() { - return typeof(Explicit_Invalid32bit).ToString(); + return typeof(Explicit5a_Invalid64).ToString(); } [MethodImpl(MethodImplOptions.NoInlining)] - static string Load64() + static string LoadB64() { - return typeof(Explicit_Invalid64bit).ToString(); + return typeof(Explicit5b_Invalid64).ToString(); } } } From f87bb6054cd77c28e5e0d41bd59ef5cdea86af25 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 27 Jan 2025 08:10:24 -0800 Subject: [PATCH 03/16] Fix mono error checking --- src/mono/mono/metadata/class-init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c index 59920cccd1aedc..6a43869672e572 100644 --- a/src/mono/mono/metadata/class-init.c +++ b/src/mono/mono/metadata/class-init.c @@ -2381,7 +2381,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_ } ftype = mono_type_get_underlying_type (field->type); ftype = mono_type_get_basic_type_from_generic (ftype); - if (type_has_references (klass, ftype) || m_type_is_byref (ftype)) { + if (type_has_references (klass, ftype) || type_has_ref_fields (ftype) || m_type_is_byref (ftype)) { if (field_offsets [i] % TARGET_SIZEOF_VOID_P) { mono_class_set_type_load_failure (klass, "Reference typed field '%s' has explicit offset that is not pointer-size aligned.", field->name); } From 405b24e4eeb2d83d3e642627726dad98357cb8de Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Mon, 27 Jan 2025 14:15:16 -0800 Subject: [PATCH 04/16] Update native AOT. --- .../Common/ExplicitLayoutValidator.cs | 2 +- .../Common/MetadataFieldLayoutAlgorithm.cs | 78 +++++++++++-------- 2 files changed, 46 insertions(+), 34 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index b176d0a1c50601..b06243b1e2a940 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -81,7 +81,7 @@ protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType) return false; } - if (offset % PointerSize != 0) + if (!fieldType.IsByRefLike && offset % PointerSize != 0) { // Misaligned struct with GC pointers or ByRef ThrowFieldLayoutError(offset); diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index b76c850e1544d2..fe9a2fe1870a7d 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -207,7 +207,7 @@ public override ComputedStaticFieldLayout ComputeStaticFieldLayout(DefType defTy } ref StaticsBlock block = ref GetStaticsBlockForField(ref result, field); - SizeAndAlignment sizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout: false, context.Target.DefaultPackingSize, out bool _, out bool _, out bool _, out bool _); + SizeAndAlignment sizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout: false, context.Target.DefaultPackingSize, out ComputedFieldData _); block.Size = LayoutInt.AlignUp(block.Size, sizeAndAlignment.Alignment, context.Target); result.Offsets[index] = new FieldAndOffset(field, block.Size); @@ -291,7 +291,7 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty LayoutInt cumulativeInstanceFieldPos = CalculateFieldBaseOffset(type, requiresAlign8: false, requiresAlignedBase: false) - offsetBias; LayoutInt instanceSize = cumulativeInstanceFieldPos + offsetBias; - var layoutMetadata = type.GetClassLayout(); + ClassLayoutMetadata layoutMetadata = type.GetClassLayout(); int packingSize = ComputePackingSize(type, layoutMetadata); LayoutInt largestAlignmentRequired = LayoutInt.One; @@ -308,17 +308,17 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty hasVectorTField = type.BaseType.IsVectorTOrHasVectorTFields; } - foreach (var fieldAndOffset in layoutMetadata.Offsets) + foreach (FieldAndOffset fieldAndOffset in layoutMetadata.Offsets) { TypeDesc fieldType = fieldAndOffset.Field.FieldType; - var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType.UnderlyingType, hasLayout: true, packingSize, out bool fieldLayoutAbiStable, out bool fieldHasAutoLayout, out bool fieldHasInt128Field, out bool fieldHasVectorTField); - if (!fieldLayoutAbiStable) + var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType.UnderlyingType, hasLayout: true, packingSize, out ComputedFieldData fieldData); + if (!fieldData.LayoutAbiStable) layoutAbiStable = false; - if (fieldHasAutoLayout) + if (fieldData.HasAutoLayout) hasAutoLayoutField = true; - if (fieldHasInt128Field) + if (fieldData.HasInt128Field) hasInt128Field = true; - if (fieldHasVectorTField) + if (fieldData.HasVectorTField) hasVectorTField = true; largestAlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequired); @@ -420,14 +420,14 @@ protected ComputedInstanceFieldLayout ComputeSequentialFieldLayout(MetadataType if (field.IsStatic) continue; - var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType.UnderlyingType, hasLayout: true, packingSize, out bool fieldLayoutAbiStable, out bool fieldHasAutoLayout, out bool fieldHasInt128Field, out bool fieldHasVectorTField); - if (!fieldLayoutAbiStable) + var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType.UnderlyingType, hasLayout: true, packingSize, out ComputedFieldData fieldData); + if (!fieldData.LayoutAbiStable) layoutAbiStable = false; - if (fieldHasAutoLayout) + if (fieldData.HasAutoLayout) hasAutoLayoutField = true; - if (fieldHasInt128Field) + if (fieldData.HasInt128Field) hasInt128Field = true; - if (fieldHasVectorTField) + if (fieldData.HasVectorTField) hasVectorTField = true; largestAlignmentRequirement = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequirement); @@ -557,7 +557,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, { Debug.Assert(fieldType.IsPrimitive || fieldType.IsPointer || fieldType.IsFunctionPointer || fieldType.IsEnum || fieldType.IsByRef); - var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out bool _, out bool _, out bool _, out bool _); + var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out ComputedFieldData _); instanceNonGCPointerFieldsCount[CalculateLog2(fieldSizeAndAlignment.Size.AsInt)]++; } } @@ -594,8 +594,8 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, TypeDesc fieldType = field.FieldType; - var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable, out bool _, out bool _, out bool _); - if (!fieldLayoutAbiStable) + var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(fieldType, hasLayout, packingSize, out ComputedFieldData fieldData); + if (!fieldData.LayoutAbiStable) layoutAbiStable = false; if (IsByValueClass(fieldType)) @@ -680,7 +680,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, int j; // Check if the position is aligned such that we could place a larger type - int offsetModulo = cumulativeInstanceFieldPos.AsInt % (1 << (i+1)); + int offsetModulo = cumulativeInstanceFieldPos.AsInt % (1 << (i + 1)); if (offsetModulo == 0) { continue; @@ -764,8 +764,8 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, for (int i = 0; i < instanceValueClassFieldsArr.Length; i++) { // Align the cumulative field offset to the indeterminate value - var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(instanceValueClassFieldsArr[i].FieldType, hasLayout, packingSize, out bool fieldLayoutAbiStable, out bool _, out bool _, out bool _); - if (!fieldLayoutAbiStable) + var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(instanceValueClassFieldsArr[i].FieldType, hasLayout, packingSize, out ComputedFieldData fieldData); + if (!fieldData.LayoutAbiStable) layoutAbiStable = false; cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(cumulativeInstanceFieldPos, fieldSizeAndAlignment.Alignment, context.Target); @@ -835,7 +835,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, private static void PlaceInstanceField(FieldDesc field, bool hasLayout, int packingSize, FieldAndOffset[] offsets, ref LayoutInt instanceFieldPos, ref int fieldOrdinal, LayoutInt offsetBias) { - var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType, hasLayout, packingSize, out bool _, out bool _, out bool _, out bool _); + var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType, hasLayout, packingSize, out ComputedFieldData _); instanceFieldPos = AlignUpInstanceFieldOffset(instanceFieldPos, fieldSizeAndAlignment.Alignment, field.Context.Target); offsets[fieldOrdinal] = new FieldAndOffset(field, instanceFieldPos + offsetBias); @@ -848,9 +848,9 @@ private static void PlaceInstanceField(FieldDesc field, bool hasLayout, int pack // This will calculate the next multiple of 4 that is greater than or equal to the instance size private static LayoutInt GetAlignedNumInstanceFieldBytes(LayoutInt instanceSize) { - uint inputSize = (uint) instanceSize.AsInt; + uint inputSize = (uint)instanceSize.AsInt; uint result = (uint)(((inputSize + 3) & (~3))); - return new LayoutInt((int) result); + return new LayoutInt((int)result); } private static int CalculateLog2(int size) @@ -859,7 +859,7 @@ private static int CalculateLog2(int size) Debug.Assert(size > 0); // Size must be a power of 2 - Debug.Assert( 0 == (size & (size - 1))); + Debug.Assert(0 == (size & (size - 1))); int log2size; for (log2size = 0; size > 1; log2size++) @@ -895,13 +895,25 @@ public LayoutInt CalculateFieldBaseOffset(MetadataType type, bool requiresAlign8 return cumulativeInstanceFieldPos; } - private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType, bool hasLayout, int packingSize, out bool layoutAbiStable, out bool fieldTypeHasAutoLayout, out bool fieldTypeHasInt128Field, out bool fieldTypeHasVectorTField) + private struct ComputedFieldData + { + public bool LayoutAbiStable; + public bool HasAutoLayout; + public bool HasInt128Field; + public bool HasVectorTField; + } + + private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType, bool hasLayout, int packingSize, out ComputedFieldData fieldData) { SizeAndAlignment result; - layoutAbiStable = true; - fieldTypeHasAutoLayout = true; - fieldTypeHasInt128Field = false; - fieldTypeHasVectorTField = false; + fieldData = new() + { + // Initialize to expected default value + LayoutAbiStable = true, + HasAutoLayout = true, + HasInt128Field = false, + HasVectorTField = false + }; if (fieldType.IsDefType) { @@ -910,10 +922,10 @@ private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType, DefType defType = (DefType)fieldType; result.Size = defType.InstanceFieldSize; result.Alignment = defType.InstanceFieldAlignment; - layoutAbiStable = defType.LayoutAbiStable; - fieldTypeHasAutoLayout = defType.IsAutoLayoutOrHasAutoLayoutFields; - fieldTypeHasInt128Field = defType.IsInt128OrHasInt128Fields; - fieldTypeHasVectorTField = defType.IsVectorTOrHasVectorTFields; + fieldData.LayoutAbiStable = defType.LayoutAbiStable; + fieldData.HasAutoLayout = defType.IsAutoLayoutOrHasAutoLayoutFields; + fieldData.HasInt128Field = defType.IsInt128OrHasInt128Fields; + fieldData.HasVectorTField = defType.IsVectorTOrHasVectorTFields; } else { @@ -934,7 +946,7 @@ private static SizeAndAlignment ComputeFieldSizeAndAlignment(TypeDesc fieldType, Debug.Assert(fieldType.IsPointer || fieldType.IsFunctionPointer || fieldType.IsByRef); result.Size = fieldType.Context.Target.LayoutPointerSize; result.Alignment = fieldType.Context.Target.LayoutPointerSize; - fieldTypeHasAutoLayout = fieldType.IsByRef; + fieldData.HasAutoLayout = fieldType.IsByRef; } // For non-auto layouts, we need to respect tighter packing requests for alignment. From d0fbad15149f4373d9f7795ac78b642a9d2a6f64 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 28 Jan 2025 14:27:36 -0800 Subject: [PATCH 05/16] Feedback --- .../Common/ExplicitLayoutValidator.cs | 11 +++----- src/coreclr/vm/methodtablebuilder.cpp | 25 +++++++++---------- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index b06243b1e2a940..9816723ad959bc 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -76,18 +76,13 @@ protected override FieldLayoutTag GetIntervalDataForType(int offset, TypeDesc fi protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType) { - if (!fieldType.IsValueType || !((MetadataType)fieldType).ContainsGCPointers && !fieldType.IsByRefLike) + if (!fieldType.IsValueType) { return false; } - if (!fieldType.IsByRefLike && offset % PointerSize != 0) - { - // Misaligned struct with GC pointers or ByRef - ThrowFieldLayoutError(offset); - } - - return true; + // Valuetypes with GC references and byref-like types need to be checked for overlaps and alignment + return ((MetadataType)fieldType).ContainsGCPointers || fieldType.IsByRefLike; } private void ThrowFieldLayoutError(int offset) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index c520d4adc8e14c..c14f8ca2b08e4c 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8677,17 +8677,8 @@ MethodTableBuilder::HandleExplicitLayout( else { MethodTable *pByValueMT = pByValueClassCache[valueClassCacheIndex]; - BOOL hasGcPointers = pByValueMT->ContainsGCPointers(); - if (hasGcPointers || pByValueMT->IsByRefLike()) + if (pByValueMT->ContainsGCPointers() || pByValueMT->IsByRefLike()) { - if (hasGcPointers && ((pFD->GetOffset() & ((ULONG)TARGET_POINTER_SIZE - 1)) != 0)) - { - // If we got here, then a valuetype containing an OREF was misaligned. - badOffset = pFD->GetOffset(); - fieldTrust.SetTrust(ExplicitFieldTrust::kNone); - break; - } - ExplicitFieldTrust::TrustLevel trust = CheckValueClassLayout(pByValueMT, &pFieldLayout[pFD->GetOffset()], pFD->GetOffset()); fieldTrust.SetTrust(trust); @@ -8871,10 +8862,10 @@ MethodTableBuilder::HandleExplicitLayout( bmtFieldLayoutTag *vcLayout = (bmtFieldLayoutTag*) qb.AllocThrows(fieldSize * sizeof(bmtFieldLayoutTag)); memset((void*)vcLayout, nonoref, fieldSize); - // If the type contains pointers fill it out from the GC data + // If the type contains pointers fill it out from the GC data and validate OREF alignment. if (pMT->ContainsGCPointers()) { - // use pointer series to locate the orefs + // Use pointer series to locate the OREFs CGCDesc* map = CGCDesc::GetCGCDescFromMT(pMT); CGCDescSeries *pSeries = map->GetLowestSeries(); @@ -8882,7 +8873,15 @@ MethodTableBuilder::HandleExplicitLayout( { CONSISTENCY_CHECK(pSeries <= map->GetHighestSeries()); - memset((void*)&vcLayout[pSeries->GetSeriesOffset() - OBJECT_SIZE], oref, pSeries->GetSeriesSize() + pMT->GetBaseSize()); + // Get offset into the value class of the first pointer field (includes a +Object) + size_t offset = pSeries->GetSeriesOffset() - OBJECT_SIZE; + if ((fieldBaseOffset + offset) % TARGET_POINTER_SIZE != 0) + { + // If we got here, then an OREF is misaligned. + return ExplicitFieldTrust::kNone; + } + + memset((void*)&vcLayout[offset], oref, pSeries->GetSeriesSize() + pMT->GetBaseSize()); pSeries++; } } From f762b1dacbdf4673d42ac5e4a377aa4d5b20fc2e Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 28 Jan 2025 15:18:29 -0800 Subject: [PATCH 06/16] Update native AOT tests --- .../CoreTestAssembly/InstanceFieldLayout.cs | 1 + 1 file changed, 1 insertion(+) 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 a67197c0a1901b..4262dc92072aa8 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs @@ -118,6 +118,7 @@ ref struct MisalignedByRef ref struct ByRefStruct { + public ref int R; } } From 3c86eb9e1180f7288e92fe07e4de241714476697 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 28 Jan 2025 15:32:14 -0800 Subject: [PATCH 07/16] Feedback --- src/coreclr/vm/methodtablebuilder.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/methodtablebuilder.cpp b/src/coreclr/vm/methodtablebuilder.cpp index c14f8ca2b08e4c..0f3c65c12981b6 100644 --- a/src/coreclr/vm/methodtablebuilder.cpp +++ b/src/coreclr/vm/methodtablebuilder.cpp @@ -8865,6 +8865,14 @@ MethodTableBuilder::HandleExplicitLayout( // If the type contains pointers fill it out from the GC data and validate OREF alignment. if (pMT->ContainsGCPointers()) { + // If the type contains pointers, it has to start at an aligned offset. + // The alignment of the pointer series itself was checked earlier. + if (fieldBaseOffset % TARGET_POINTER_SIZE != 0) + { + // If we got here, then an OREF is misaligned. + return ExplicitFieldTrust::kNone; + } + // Use pointer series to locate the OREFs CGCDesc* map = CGCDesc::GetCGCDescFromMT(pMT); CGCDescSeries *pSeries = map->GetLowestSeries(); @@ -8874,14 +8882,7 @@ MethodTableBuilder::HandleExplicitLayout( CONSISTENCY_CHECK(pSeries <= map->GetHighestSeries()); // Get offset into the value class of the first pointer field (includes a +Object) - size_t offset = pSeries->GetSeriesOffset() - OBJECT_SIZE; - if ((fieldBaseOffset + offset) % TARGET_POINTER_SIZE != 0) - { - // If we got here, then an OREF is misaligned. - return ExplicitFieldTrust::kNone; - } - - memset((void*)&vcLayout[offset], oref, pSeries->GetSeriesSize() + pMT->GetBaseSize()); + memset((void*)&vcLayout[pSeries->GetSeriesOffset() - OBJECT_SIZE], oref, pSeries->GetSeriesSize() + pMT->GetBaseSize()); pSeries++; } } From c8285e9a6d3c294ba2342de6deb61defc8fa2f95 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Tue, 28 Jan 2025 21:58:48 -0800 Subject: [PATCH 08/16] Fix test build warning --- .../CoreTestAssembly/InstanceFieldLayout.cs | 2 ++ 1 file changed, 2 insertions(+) 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 4262dc92072aa8..d385dec97fcfe8 100644 --- a/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs +++ b/src/coreclr/tools/aot/ILCompiler.TypeSystem.Tests/CoreTestAssembly/InstanceFieldLayout.cs @@ -118,7 +118,9 @@ ref struct MisalignedByRef ref struct ByRefStruct { +#pragma warning disable CS9265 // Field is never ref-assigned to, and will always have its default value (null reference) public ref int R; +#pragma warning restore CS9265 } } From c458d7436cbc6d3fb5a79e97683174c2faae1560 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 29 Jan 2025 01:05:03 -0800 Subject: [PATCH 09/16] AOT should needs to pass along the base offset. --- .../tools/Common/JitInterface/SwiftPhysicalLowering.cs | 2 +- .../Common/TypeSystem/Common/ExplicitLayoutValidator.cs | 2 +- .../TypeSystem/Common/FieldLayoutIntervalCalculator.cs | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/tools/Common/JitInterface/SwiftPhysicalLowering.cs b/src/coreclr/tools/Common/JitInterface/SwiftPhysicalLowering.cs index f5d8afc33d3105..0d6cb3561a6b6a 100644 --- a/src/coreclr/tools/Common/JitInterface/SwiftPhysicalLowering.cs +++ b/src/coreclr/tools/Common/JitInterface/SwiftPhysicalLowering.cs @@ -101,7 +101,7 @@ protected override LoweredType GetIntervalDataForType(int offset, TypeDesc field return LoweredType.Opaque; } - protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType) => fieldType.IsValueType && !fieldType.IsPrimitiveNumeric; + protected override bool NeedsRecursiveLayout(TypeDesc fieldType) => fieldType.IsValueType && !fieldType.IsPrimitiveNumeric; private List CreateConsolidatedIntervals() { diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index 9816723ad959bc..24b75926c79416 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -74,7 +74,7 @@ protected override FieldLayoutTag GetIntervalDataForType(int offset, TypeDesc fi } } - protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType) + protected override bool NeedsRecursiveLayout(TypeDesc fieldType) { if (!fieldType.IsValueType) { diff --git a/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs b/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs index 73fa31073598a0..dffeb0e12a8a4e 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs @@ -55,7 +55,7 @@ public FieldLayoutIntervalCalculator(int pointerSize) PointerSize = pointerSize; } - protected abstract bool NeedsRecursiveLayout(int offset, TypeDesc fieldType); + protected abstract bool NeedsRecursiveLayout(TypeDesc fieldType); protected abstract TIntervalTag GetIntervalDataForType(int offset, TypeDesc fieldType); @@ -84,7 +84,7 @@ private int GetFieldSize(TypeDesc fieldType) public void AddToFieldLayout(int offset, TypeDesc fieldType, bool addTrailingEmptyInterval) { - if (NeedsRecursiveLayout(offset, fieldType)) + if (NeedsRecursiveLayout(fieldType)) { if (fieldType is MetadataType { IsInlineArray: true } mdType) { @@ -97,7 +97,7 @@ public void AddToFieldLayout(int offset, TypeDesc fieldType, bool addTrailingEmp if (field.IsStatic) continue; - int fieldOffset = field.Offset.AsInt; + int fieldOffset = offset + field.Offset.AsInt; AddToFieldLayout(nestedIntervals, fieldOffset, field.FieldType); } @@ -126,7 +126,7 @@ public void AddToFieldLayout(int offset, TypeDesc fieldType, bool addTrailingEmp private void AddToFieldLayout(List fieldLayout, int offset, TypeDesc fieldType) { - if (NeedsRecursiveLayout(offset, fieldType)) + if (NeedsRecursiveLayout(fieldType)) { if (fieldType is MetadataType { IsInlineArray: true } mdType) { From 3b789f28b3d11929122eefd06ea597af675e3a87 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 29 Jan 2025 08:16:07 -0800 Subject: [PATCH 10/16] Add property for type containing any byrefs. --- .../Compiler/Int128FieldLayoutAlgorithm.cs | 6 +++ .../Compiler/VectorFieldLayoutAlgorithm.cs | 6 +++ .../TypeSystem/Common/DefType.FieldLayout.cs | 39 +++++++++++++++++++ .../TypeSystem/Common/FieldLayoutAlgorithm.cs | 5 +++ .../Common/FieldLayoutIntervalCalculator.cs | 2 +- .../Common/MetadataFieldLayoutAlgorithm.cs | 19 +++++++++ ...eWithRepeatedFieldsFieldLayoutAlgorithm.cs | 5 +++ .../Common/UniversalCanonLayoutAlgorithm.cs | 6 +++ .../RuntimeDeterminedFieldLayoutAlgorithm.cs | 8 ++++ .../Compiler/VectorOfTFieldLayoutAlgorithm.cs | 5 +++ 10 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/Common/Compiler/Int128FieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/Compiler/Int128FieldLayoutAlgorithm.cs index 18f794a6f354e8..a78446d5967ae4 100644 --- a/src/coreclr/tools/Common/Compiler/Int128FieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/Compiler/Int128FieldLayoutAlgorithm.cs @@ -61,6 +61,12 @@ public override bool ComputeContainsGCPointers(DefType type) return false; } + public override bool ComputeContainsByRefs(DefType type) + { + Debug.Assert(!_fallbackAlgorithm.ComputeContainsByRefs(type)); + return false; + } + public override bool ComputeIsUnsafeValueType(DefType type) { Debug.Assert(!_fallbackAlgorithm.ComputeIsUnsafeValueType(type)); diff --git a/src/coreclr/tools/Common/Compiler/VectorFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/Compiler/VectorFieldLayoutAlgorithm.cs index 1f3722ab019bcc..15d22e79fe8f3b 100644 --- a/src/coreclr/tools/Common/Compiler/VectorFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/Compiler/VectorFieldLayoutAlgorithm.cs @@ -133,6 +133,12 @@ public override bool ComputeContainsGCPointers(DefType type) return false; } + public override bool ComputeContainsByRefs(DefType type) + { + Debug.Assert(!_fallbackAlgorithm.ComputeContainsByRefs(type)); + return false; + } + public override bool ComputeIsUnsafeValueType(DefType type) { Debug.Assert(!_fallbackAlgorithm.ComputeIsUnsafeValueType(type)); diff --git a/src/coreclr/tools/Common/TypeSystem/Common/DefType.FieldLayout.cs b/src/coreclr/tools/Common/TypeSystem/Common/DefType.FieldLayout.cs index fb4cd6bf17157f..073b118f7e8947 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/DefType.FieldLayout.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/DefType.FieldLayout.cs @@ -78,6 +78,16 @@ private static class FieldLayoutFlags /// True if the type transitively has a Vector in it or is Vector /// public const int IsVectorTOrHasVectorTFields = 0x1000; + + /// + /// True if ContainsByRefs has been computed + /// + public const int ComputedContainsByRefs = 0x2000; + + /// + /// True if the type contains byrefs + /// + public const int ContainsByRefs = 0x4000; } private sealed class StaticBlockInfo @@ -113,6 +123,21 @@ public bool ContainsGCPointers } } + /// + /// Does a type transitively have any fields which are byrefs + /// + public bool ContainsByRefs + { + get + { + if (!_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedContainsByRefs)) + { + ComputeTypeContainsByRefs(); + } + return _fieldLayoutFlags.HasFlags(FieldLayoutFlags.ContainsByRefs); + } + } + /// /// Does a type transitively have any fields which are marked with System.Runtime.CompilerServices.UnsafeValueTypeAttribute /// @@ -545,6 +570,20 @@ public void ComputeTypeContainsGCPointers() _fieldLayoutFlags.AddFlags(flagsToAdd); } + public void ComputeTypeContainsByRefs() + { + if (_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedContainsByRefs)) + return; + + int flagsToAdd = FieldLayoutFlags.ComputedContainsByRefs; + if (this.Context.GetLayoutAlgorithmForType(this).ComputeContainsByRefs(this)) + { + flagsToAdd |= FieldLayoutFlags.ContainsByRefs; + } + + _fieldLayoutFlags.AddFlags(flagsToAdd); + } + public void ComputeIsUnsafeValueType() { if (_fieldLayoutFlags.HasFlags(FieldLayoutFlags.ComputedIsUnsafeValueType)) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutAlgorithm.cs index 31a46ec47f6416..09818b42cf7de2 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutAlgorithm.cs @@ -31,6 +31,11 @@ public abstract class FieldLayoutAlgorithm /// public abstract bool ComputeContainsGCPointers(DefType type); + /// + /// Compute whether the fields of the specified type contains a byref. + /// + public abstract bool ComputeContainsByRefs(DefType type); + /// /// Compute whether the specified type is a value type that transitively has UnsafeValueTypeAttribute /// diff --git a/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs b/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs index dffeb0e12a8a4e..51683d223db00d 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/FieldLayoutIntervalCalculator.cs @@ -97,7 +97,7 @@ public void AddToFieldLayout(int offset, TypeDesc fieldType, bool addTrailingEmp if (field.IsStatic) continue; - int fieldOffset = offset + field.Offset.AsInt; + int fieldOffset = field.Offset.AsInt; AddToFieldLayout(nestedIntervals, fieldOffset, field.FieldType); } diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index fe9a2fe1870a7d..e70b22be9dd746 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -269,6 +269,24 @@ public override bool ComputeContainsGCPointers(DefType type) return someFieldContainsPointers; } + public override bool ComputeContainsByRefs(DefType type) + { + if (!type.IsByRefLike) + return false; + + foreach (var field in type.GetFields()) + { + if (field.IsStatic) + continue; + + TypeDesc fieldType = field.FieldType; + if (fieldType.IsByRef) + return true; + } + + return false; + } + /// /// Called during static field layout to setup initial contents of statics blocks /// @@ -335,6 +353,7 @@ protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType ty ( fieldType.IsGCPointer || (fieldType.IsValueType && ((DefType)fieldType).ContainsGCPointers) + || (fieldType.IsByRefLike && ((DefType)fieldType).ContainsByRefs) ); if (needsToBeAligned) { diff --git a/src/coreclr/tools/Common/TypeSystem/Common/TypeWithRepeatedFieldsFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/TypeWithRepeatedFieldsFieldLayoutAlgorithm.cs index b9537b9af8baf0..c2df1dce3425a0 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/TypeWithRepeatedFieldsFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/TypeWithRepeatedFieldsFieldLayoutAlgorithm.cs @@ -65,6 +65,11 @@ public override bool ComputeContainsGCPointers(DefType type) return _fallbackAlgorithm.ComputeContainsGCPointers(((TypeWithRepeatedFields)type).MetadataType); } + public override bool ComputeContainsByRefs(DefType type) + { + return _fallbackAlgorithm.ComputeContainsByRefs(((TypeWithRepeatedFields)type).MetadataType); + } + public override bool ComputeIsUnsafeValueType(DefType type) { return _fallbackAlgorithm.ComputeIsUnsafeValueType(((TypeWithRepeatedFields)type).MetadataType); diff --git a/src/coreclr/tools/Common/TypeSystem/Common/UniversalCanonLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/UniversalCanonLayoutAlgorithm.cs index 36987f950946fd..f917d1f854d028 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/UniversalCanonLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/UniversalCanonLayoutAlgorithm.cs @@ -17,6 +17,12 @@ public override bool ComputeContainsGCPointers(DefType type) throw new NotSupportedException(); } + public override bool ComputeContainsByRefs(DefType type) + { + // This should never be called + throw new NotSupportedException(); + } + public override ComputedInstanceFieldLayout ComputeInstanceLayout(DefType type, InstanceLayoutKind layoutKind) { return new ComputedInstanceFieldLayout() diff --git a/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs index 55c3bfbd85f029..aa71102fad1e93 100644 --- a/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/RuntimeDetermined/RuntimeDeterminedFieldLayoutAlgorithm.cs @@ -48,6 +48,14 @@ public override bool ComputeContainsGCPointers(DefType type) return canonicalType.ContainsGCPointers; } + public override bool ComputeContainsByRefs(DefType type) + { + RuntimeDeterminedType runtimeDeterminedType = (RuntimeDeterminedType)type; + DefType canonicalType = runtimeDeterminedType.CanonicalType; + + return canonicalType.ContainsByRefs; + } + public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristics(DefType type) { RuntimeDeterminedType runtimeDeterminedType = (RuntimeDeterminedType)type; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VectorOfTFieldLayoutAlgorithm.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VectorOfTFieldLayoutAlgorithm.cs index cfa4dc2524815b..808ce747d9b144 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VectorOfTFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VectorOfTFieldLayoutAlgorithm.cs @@ -68,6 +68,11 @@ public override bool ComputeContainsGCPointers(DefType type) return false; } + public override bool ComputeContainsByRefs(DefType type) + { + return false; + } + public override ValueTypeShapeCharacteristics ComputeValueTypeShapeCharacteristics(DefType type) { if (type.Context.Target.Architecture == TargetArchitecture.ARM64 && From 247e5e547e615734f2e5ae139b95ea1813826dd1 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 29 Jan 2025 15:24:43 -0800 Subject: [PATCH 11/16] Feedback --- .../Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs | 5 ++++- .../Compiler/ReadyToRunCompilerContext.cs | 5 +++++ .../Compiler/SystemObjectFieldLayoutAlgorithm.cs | 5 +++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index e70b22be9dd746..3e2c6fd2536121 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -280,8 +280,11 @@ public override bool ComputeContainsByRefs(DefType type) continue; TypeDesc fieldType = field.FieldType; - if (fieldType.IsByRef) + if (fieldType.IsByRef + || ((fieldType is DefType df) && df.ContainsByRefs)) + { return true; + } } return false; diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs index 3a2c90da849bf5..2878289e33996c 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs @@ -226,6 +226,11 @@ public override bool ComputeContainsGCPointers(DefType type) return false; } + public override bool ComputeContainsByRefs(DefType type) + { + return false; + } + public override bool ComputeIsUnsafeValueType(DefType type) { return false; diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/SystemObjectFieldLayoutAlgorithm.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/SystemObjectFieldLayoutAlgorithm.cs index d7d8c047d3c057..97cd02b8c9fe84 100644 --- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/SystemObjectFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/SystemObjectFieldLayoutAlgorithm.cs @@ -50,6 +50,11 @@ public override bool ComputeContainsGCPointers(DefType type) return false; } + public override bool ComputeContainsByRefs(DefType type) + { + return false; + } + public override bool ComputeIsUnsafeValueType(DefType type) { return false; From 8cfee36b61e3d5aa0a726abb967fbb7b344ddb2f Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 30 Jan 2025 00:43:09 -0800 Subject: [PATCH 12/16] Feedback --- docs/coding-guidelines/breaking-change-rules.md | 2 ++ .../Common/TypeSystem/Common/ExplicitLayoutValidator.cs | 5 +++-- .../Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/coding-guidelines/breaking-change-rules.md b/docs/coding-guidelines/breaking-change-rules.md index e8a5e5b7d0afda..131d0f0ec708b7 100644 --- a/docs/coding-guidelines/breaking-change-rules.md +++ b/docs/coding-guidelines/breaking-change-rules.md @@ -169,6 +169,8 @@ Breaking Change Rules * Changing a `struct` type to a `ref struct` type and vice versa +* Adding a new `ref` field to a type + * Changing the underlying type of an enum This is a compile-time and behavioral breaking change as well as a binary breaking change which can make attribute arguments unparsable. diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index 24b75926c79416..f980dfc4f315a0 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -81,8 +81,9 @@ protected override bool NeedsRecursiveLayout(TypeDesc fieldType) return false; } - // Valuetypes with GC references and byref-like types need to be checked for overlaps and alignment - return ((MetadataType)fieldType).ContainsGCPointers || fieldType.IsByRefLike; + // Valuetypes with GC references or byrefs need to be checked for overlaps and alignment + var metadataType = ((MetadataType)fieldType); + return metadataType.ContainsGCPointers || metadataType.ContainsByRefs; } private void ThrowFieldLayoutError(int offset) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs index 3e2c6fd2536121..7a1a05ad26488b 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs @@ -281,7 +281,7 @@ public override bool ComputeContainsByRefs(DefType type) TypeDesc fieldType = field.FieldType; if (fieldType.IsByRef - || ((fieldType is DefType df) && df.ContainsByRefs)) + || (fieldType.IsByRefLike && ((DefType)fieldType).ContainsByRefs)) { return true; } From 59f7ec4b517378b97dbc55d5d4bfdeac5f10d323 Mon Sep 17 00:00:00 2001 From: Aaron Robinson Date: Thu, 30 Jan 2025 04:54:44 -0800 Subject: [PATCH 13/16] Update docs/coding-guidelines/breaking-change-rules.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Michal Strehovský --- docs/coding-guidelines/breaking-change-rules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/coding-guidelines/breaking-change-rules.md b/docs/coding-guidelines/breaking-change-rules.md index 131d0f0ec708b7..f5161b9d9c0a2f 100644 --- a/docs/coding-guidelines/breaking-change-rules.md +++ b/docs/coding-guidelines/breaking-change-rules.md @@ -169,7 +169,7 @@ Breaking Change Rules * Changing a `struct` type to a `ref struct` type and vice versa -* Adding a new `ref` field to a type +* Adding a new `ref` field to a type that didn't have any `ref` field before (recursively) * Changing the underlying type of an enum From ac4e3d28f6bf47df9a5a2cdff4df4fbe5ca4ae06 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 30 Jan 2025 05:22:39 -0800 Subject: [PATCH 14/16] Fix native AOT predicate --- .../Common/TypeSystem/Common/ExplicitLayoutValidator.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index f980dfc4f315a0..e39b84ae81e48f 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -60,12 +60,12 @@ protected override FieldLayoutTag GetIntervalDataForType(int offset, TypeDesc fi else if (fieldType.IsValueType) { MetadataType mdType = (MetadataType)fieldType; - if (!mdType.ContainsGCPointers && !mdType.IsByRefLike) + if (!mdType.ContainsGCPointers && !mdType.ContainsByRefs) { // Plain value type, mark the entire range as NonORef return FieldLayoutTag.NonORef; } - Debug.Fail("We should recurse on value types with GC pointers or ByRefLike types"); + Debug.Fail("We should recurse on value types with GC pointers or byrefs"); return FieldLayoutTag.Empty; } else @@ -82,8 +82,8 @@ protected override bool NeedsRecursiveLayout(TypeDesc fieldType) } // Valuetypes with GC references or byrefs need to be checked for overlaps and alignment - var metadataType = ((MetadataType)fieldType); - return metadataType.ContainsGCPointers || metadataType.ContainsByRefs; + MetadataType mdType = ((MetadataType)fieldType); + return mdType.ContainsGCPointers || mdType.ContainsByRefs; } private void ThrowFieldLayoutError(int offset) From 47a7a1345ff53023bd00593c5f4ff53b1c228ff2 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 30 Jan 2025 05:26:59 -0800 Subject: [PATCH 15/16] Fix style --- .../tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs index e39b84ae81e48f..6c4cb3f9b4149b 100644 --- a/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs +++ b/src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs @@ -82,7 +82,7 @@ protected override bool NeedsRecursiveLayout(TypeDesc fieldType) } // Valuetypes with GC references or byrefs need to be checked for overlaps and alignment - MetadataType mdType = ((MetadataType)fieldType); + MetadataType mdType = (MetadataType)fieldType; return mdType.ContainsGCPointers || mdType.ContainsByRefs; } From 7f864881279d8316929b9ecd0ccd41c69656b280 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 30 Jan 2025 09:29:21 -0800 Subject: [PATCH 16/16] Revert changes to breaking-change-rules.md --- docs/coding-guidelines/breaking-change-rules.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/coding-guidelines/breaking-change-rules.md b/docs/coding-guidelines/breaking-change-rules.md index f5161b9d9c0a2f..e8a5e5b7d0afda 100644 --- a/docs/coding-guidelines/breaking-change-rules.md +++ b/docs/coding-guidelines/breaking-change-rules.md @@ -169,8 +169,6 @@ Breaking Change Rules * Changing a `struct` type to a `ref struct` type and vice versa -* Adding a new `ref` field to a type that didn't have any `ref` field before (recursively) - * Changing the underlying type of an enum This is a compile-time and behavioral breaking change as well as a binary breaking change which can make attribute arguments unparsable.