Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix explicit offset of ByRefLike fields. #111584

Merged
merged 16 commits into from
Jan 31, 2025
Merged

Fix explicit offset of ByRefLike fields. #111584

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
948d73b
Fix explicit offset of ByRefLike fields.
AaronRobinsonMSFT Jan 19, 2025
b0cda63
Improve tests coverage
AaronRobinsonMSFT Jan 19, 2025
f87bb60
Fix mono error checking
AaronRobinsonMSFT Jan 27, 2025
405b24e
Update native AOT.
AaronRobinsonMSFT Jan 27, 2025
d0fbad1
Feedback
AaronRobinsonMSFT Jan 28, 2025
f762b1d
Update native AOT tests
AaronRobinsonMSFT Jan 28, 2025
3c86eb9
Feedback
AaronRobinsonMSFT Jan 28, 2025
c8285e9
Fix test build warning
AaronRobinsonMSFT Jan 29, 2025
c458d74
AOT should needs to pass along the base offset.
AaronRobinsonMSFT Jan 29, 2025
3b789f2
Add property for type containing any byrefs.
AaronRobinsonMSFT Jan 29, 2025
247e5e5
Feedback
AaronRobinsonMSFT Jan 29, 2025
8cfee36
Feedback
AaronRobinsonMSFT Jan 30, 2025
59f7ec4
Update docs/coding-guidelines/breaking-change-rules.md
AaronRobinsonMSFT Jan 30, 2025
ac4e3d2
Fix native AOT predicate
AaronRobinsonMSFT Jan 30, 2025
47a7a13
Fix style
AaronRobinsonMSFT Jan 30, 2025
7f86488
Revert changes to breaking-change-rules.md
AaronRobinsonMSFT Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/TypeSystem/Common/ExplicitLayoutValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ protected override bool NeedsRecursiveLayout(int offset, TypeDesc fieldType)
return false;
}

if (offset % PointerSize != 0)
if (!fieldType.IsByRefLike && offset % PointerSize != 0)
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
// Misaligned struct with GC pointers or ByRef
ThrowFieldLayoutError(offset);
Expand Down
80 changes: 45 additions & 35 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;

Expand All @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -422,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);
Expand Down Expand Up @@ -559,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)]++;
}
}
Expand Down Expand Up @@ -596,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))
Expand Down Expand Up @@ -682,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;
Expand Down Expand Up @@ -766,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);
Expand Down Expand Up @@ -837,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);
Expand All @@ -850,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)
Expand All @@ -861,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++)
Expand Down Expand Up @@ -897,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)
{
Expand All @@ -912,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
{
Expand All @@ -936,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.
Expand Down
38 changes: 20 additions & 18 deletions src/coreclr/vm/methodtablebuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
AaronRobinsonMSFT marked this conversation as resolved.
Show resolved Hide resolved
{
// 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();
Expand Down Expand Up @@ -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)
jkotas marked this conversation as resolved.
Show resolved Hide resolved
{
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
Expand Down Expand Up @@ -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());
Expand All @@ -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
{
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/vm/methodtablebuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/class-init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
Loading
Loading