Skip to content

Commit

Permalink
Fix for field layout verification across version bubble boundary (#50364
Browse files Browse the repository at this point in the history 
)

* Fix for field layout verification across version bubble boundary

When verifying field offset consistency, we shouldn't be checking
base class size when the base class doesn't belong to the current
version bubble. I have fixed this by adding a special case for
the existing fixup encoding indicated by base class size being
zero. I have also created a simple regression test that was previously
failing when run against the framework compiled with CG2 assembly
by assembly.

I have fixed several other corner field layout cases - we shouldn't
be 8-aligning the base class on x86, zero-sized base classes with
explicit layout are treated as if they are 1 byte long, improved
offset bias handling based on parallel corerun / crossgen2 debugging.

Thanks

Tomas
  • Loading branch information
@trylek
trylek authored Apr 7, 2021
1 parent 89ea4c4 commit a7ed1fd
Show file tree
Hide file tree
Showing 10 changed files with 153 additions and 75 deletions.
85 changes: 39 additions & 46 deletions src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,11 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
{
// Instance slice size is the total size of instance not including the base type.
// It is calculated as the field whose offset and size add to the greatest value.
LayoutInt offsetBias = !type.IsValueType ? new LayoutInt(type.Context.Target.PointerSize) : LayoutInt.Zero;
LayoutInt cumulativeInstanceFieldPos =
type.HasBaseType && !type.IsValueType ? type.BaseType.InstanceByteCount : LayoutInt.Zero;
LayoutInt instanceSize = cumulativeInstanceFieldPos;
cumulativeInstanceFieldPos -= offsetBias;

var layoutMetadata = type.GetClassLayout();

Expand All @@ -333,7 +335,7 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
if (fieldAndOffset.Offset == FieldAndOffset.InvalidOffset)
ThrowHelper.ThrowTypeLoadException(ExceptionStringID.ClassLoadBadFormat, type);

LayoutInt computedOffset = fieldAndOffset.Offset + cumulativeInstanceFieldPos;
LayoutInt computedOffset = fieldAndOffset.Offset + cumulativeInstanceFieldPos + offsetBias;

// GC pointers MUST be aligned.
// We treat byref-like structs as GC pointers too.
Expand Down Expand Up @@ -379,29 +381,20 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
return computedLayout;
}

private static LayoutInt AlignUpInstanceFieldOffset(TypeDesc typeWithField, LayoutInt cumulativeInstanceFieldPos, LayoutInt alignment, TargetDetails target, bool armAlignFromStartOfFields = false)
private static LayoutInt AlignUpInstanceFieldOffset(TypeDesc typeWithField, LayoutInt cumulativeInstanceFieldPos, LayoutInt alignment, TargetDetails target)
{
if (!typeWithField.IsValueType && (target.Architecture == TargetArchitecture.X86 || (armAlignFromStartOfFields && target.Architecture == TargetArchitecture.ARM)) && cumulativeInstanceFieldPos != new LayoutInt(0))
{
// Alignment of fields is relative to the start of the field list, not the start of the object
//
// The code in the VM is written as if this is the rule for all architectures, but for ARM 32bit platforms
// there is an additional adjustment via dwOffsetBias that aligns fields based on the start of the object
cumulativeInstanceFieldPos = cumulativeInstanceFieldPos - new LayoutInt(target.PointerSize);
return LayoutInt.AlignUp(cumulativeInstanceFieldPos, alignment, target) + new LayoutInt(target.PointerSize);
}
else
{
return LayoutInt.AlignUp(cumulativeInstanceFieldPos, alignment, target);
}
return LayoutInt.AlignUp(cumulativeInstanceFieldPos, alignment, target);
}

protected static ComputedInstanceFieldLayout ComputeSequentialFieldLayout(MetadataType type, int numInstanceFields)
protected ComputedInstanceFieldLayout ComputeSequentialFieldLayout(MetadataType type, int numInstanceFields)
{
var offsets = new FieldAndOffset[numInstanceFields];

// For types inheriting from another type, field offsets continue on from where they left off
LayoutInt cumulativeInstanceFieldPos = ComputeBytesUsedInParentType(type);
// For reference types, we calculate field alignment as if the address after the method table pointer
// has offset 0 (on 32-bit platforms, this location is guaranteed to be 8-aligned).
LayoutInt offsetBias = !type.IsValueType ? new LayoutInt(type.Context.Target.PointerSize) : LayoutInt.Zero;
LayoutInt cumulativeInstanceFieldPos = CalculateFieldBaseOffset(type, requiresAlign8: false, requiresAlignedBase: false) - offsetBias;

var layoutMetadata = type.GetClassLayout();

Expand All @@ -421,18 +414,15 @@ protected static ComputedInstanceFieldLayout ComputeSequentialFieldLayout(Metada

largestAlignmentRequirement = LayoutInt.Max(fieldSizeAndAlignment.Alignment, largestAlignmentRequirement);

cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(type, cumulativeInstanceFieldPos, fieldSizeAndAlignment.Alignment, type.Context.Target,
armAlignFromStartOfFields: true // In what appears to have been a bug in the design of the arm32 type layout code
// this portion of the layout algorithm does not layout from the start of the object
);
offsets[fieldOrdinal] = new FieldAndOffset(field, cumulativeInstanceFieldPos);
cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(type, cumulativeInstanceFieldPos, fieldSizeAndAlignment.Alignment, type.Context.Target);
offsets[fieldOrdinal] = new FieldAndOffset(field, cumulativeInstanceFieldPos + offsetBias);
cumulativeInstanceFieldPos = checked(cumulativeInstanceFieldPos + fieldSizeAndAlignment.Size);

fieldOrdinal++;
}

SizeAndAlignment instanceByteSizeAndAlignment;
var instanceSizeAndAlignment = ComputeInstanceSize(type, cumulativeInstanceFieldPos, largestAlignmentRequirement, layoutMetadata.Size, out instanceByteSizeAndAlignment);
var instanceSizeAndAlignment = ComputeInstanceSize(type, cumulativeInstanceFieldPos + offsetBias, largestAlignmentRequirement, layoutMetadata.Size, out instanceByteSizeAndAlignment);

ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout();
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
Expand All @@ -445,14 +435,12 @@ protected static ComputedInstanceFieldLayout ComputeSequentialFieldLayout(Metada
return computedLayout;
}

protected virtual void AlignBaseOffsetIfNecessary(MetadataType type, ref LayoutInt baseOffset, bool requiresAlign8)
protected virtual void AlignBaseOffsetIfNecessary(MetadataType type, ref LayoutInt baseOffset, bool requiresAlign8, bool requiresAlignedBase)
{
}

protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type, int numInstanceFields)
{
// For types inheriting from another type, field offsets continue on from where they left off
LayoutInt cumulativeInstanceFieldPos = ComputeBytesUsedInParentType(type);
TypeSystemContext context = type.Context;

var layoutMetadata = type.GetClassLayout();
Expand All @@ -463,7 +451,6 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
var offsets = new FieldAndOffset[numInstanceFields];
int fieldOrdinal = 0;


// Iterate over the instance fields and keep track of the number of fields of each category
// For the non-GC Pointer fields, we will keep track of the number of fields by log2(size)
int maxLog2Size = CalculateLog2(TargetDetails.MaximumPrimitiveSize);
Expand Down Expand Up @@ -552,18 +539,15 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
largestAlignmentRequired = context.Target.GetObjectAlignment(largestAlignmentRequired);
bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && largestAlignmentRequired.AsInt > 4;

if (!type.IsValueType)
// 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
// 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)
{
DefType baseType = type.BaseType;
if (baseType != null && !baseType.IsObject)
{
if (!requiresAlign8 && baseType.RequiresAlign8())
{
requiresAlign8 = true;
}

AlignBaseOffsetIfNecessary(type, ref cumulativeInstanceFieldPos, requiresAlign8);
}
offsetBias = new LayoutInt(type.Context.Target.PointerSize);
cumulativeInstanceFieldPos -= offsetBias;
}

// We've finished placing the fields into their appropriate arrays
Expand Down Expand Up @@ -638,7 +622,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
// Place the field
j = instanceNonGCPointerFieldsCount[i];
FieldDesc field = instanceNonGCPointerFieldsArr[i][j];
PlaceInstanceField(field, packingSize, offsets, ref cumulativeInstanceFieldPos, ref fieldOrdinal);
PlaceInstanceField(field, packingSize, offsets, ref cumulativeInstanceFieldPos, ref fieldOrdinal, offsetBias);

instanceNonGCPointerFieldsCount[i]++;
}
Expand All @@ -655,14 +639,14 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
{
for (int j = 0; j < instanceGCPointerFieldsArr.Length; j++)
{
PlaceInstanceField(instanceGCPointerFieldsArr[j], packingSize, offsets, ref cumulativeInstanceFieldPos, ref fieldOrdinal);
PlaceInstanceField(instanceGCPointerFieldsArr[j], packingSize, offsets, ref cumulativeInstanceFieldPos, ref fieldOrdinal, offsetBias);
}
}

// The start index will be the index that may have been increased in the previous optimization
for (int j = instanceNonGCPointerFieldsCount[i]; j < instanceNonGCPointerFieldsArr[i].Length; j++)
{
PlaceInstanceField(instanceNonGCPointerFieldsArr[i][j], packingSize, offsets, ref cumulativeInstanceFieldPos, ref fieldOrdinal);
PlaceInstanceField(instanceNonGCPointerFieldsArr[i][j], packingSize, offsets, ref cumulativeInstanceFieldPos, ref fieldOrdinal, offsetBias);
}
}

Expand All @@ -685,7 +669,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
LayoutInt AlignmentRequired = LayoutInt.Max(fieldSizeAndAlignment.Alignment, context.Target.LayoutPointerSize);
cumulativeInstanceFieldPos = AlignUpInstanceFieldOffset(type, cumulativeInstanceFieldPos, AlignmentRequired, context.Target);
}
offsets[fieldOrdinal] = new FieldAndOffset(instanceValueClassFieldsArr[i], cumulativeInstanceFieldPos);
offsets[fieldOrdinal] = new FieldAndOffset(instanceValueClassFieldsArr[i], cumulativeInstanceFieldPos + offsetBias);

// If the field has an indeterminate size, align the cumulative field offset to the indeterminate value
// Otherwise, align the cumulative field offset to the aligned-instance field size
Expand Down Expand Up @@ -720,7 +704,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
}

SizeAndAlignment instanceByteSizeAndAlignment;
var instanceSizeAndAlignment = ComputeInstanceSize(type, cumulativeInstanceFieldPos, minAlign, 0/* specified field size unused */, out instanceByteSizeAndAlignment);
var instanceSizeAndAlignment = ComputeInstanceSize(type, cumulativeInstanceFieldPos + offsetBias, minAlign, 0/* specified field size unused */, out instanceByteSizeAndAlignment);

ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout();
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
Expand All @@ -733,12 +717,12 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
return computedLayout;
}

private static void PlaceInstanceField(FieldDesc field, int packingSize, FieldAndOffset[] offsets, ref LayoutInt instanceFieldPos, ref int fieldOrdinal)
private static void PlaceInstanceField(FieldDesc field, int packingSize, FieldAndOffset[] offsets, ref LayoutInt instanceFieldPos, ref int fieldOrdinal, LayoutInt offsetBias)
{
var fieldSizeAndAlignment = ComputeFieldSizeAndAlignment(field.FieldType, packingSize, out bool _);

instanceFieldPos = AlignUpInstanceFieldOffset(field.OwningType, instanceFieldPos, fieldSizeAndAlignment.Alignment, field.Context.Target);
offsets[fieldOrdinal] = new FieldAndOffset(field, instanceFieldPos);
offsets[fieldOrdinal] = new FieldAndOffset(field, instanceFieldPos + offsetBias);
instanceFieldPos = checked(instanceFieldPos + fieldSizeAndAlignment.Size);

fieldOrdinal++;
Expand Down Expand Up @@ -775,13 +759,22 @@ private static bool IsByValueClass(TypeDesc type)
return type.IsValueType && !type.IsPrimitive && !type.IsEnum;
}

private static LayoutInt ComputeBytesUsedInParentType(DefType type)
public LayoutInt CalculateFieldBaseOffset(MetadataType type, bool requiresAlign8, bool requiresAlignedBase)
{
LayoutInt cumulativeInstanceFieldPos = LayoutInt.Zero;

if (!type.IsValueType && type.HasBaseType)
{
cumulativeInstanceFieldPos = type.BaseType.InstanceByteCountUnaligned;
if (!type.BaseType.InstanceByteCountUnaligned.IsIndeterminate)
{
cumulativeInstanceFieldPos = type.BaseType.InstanceByteCountUnaligned;
if (type.BaseType.IsZeroSizedReferenceType && ((MetadataType)type.BaseType).HasLayout())
{
cumulativeInstanceFieldPos += LayoutInt.One;
}
AlignBaseOffsetIfNecessary(type, ref cumulativeInstanceFieldPos, requiresAlign8, requiresAlignedBase);
}
}

return cumulativeInstanceFieldPos;
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/CompilationModuleGroup.ReadyToRun.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,12 @@ public bool EnforceOwningType(EcmaModule module)
/// </summary>
public abstract bool IsInputBubble { get; }

/// <summary>
/// Returns true when the base type and derived type don't reside in the same version bubble
/// in which case the runtime aligns the field base offset.
/// </summary>
public abstract bool NeedsAlignmentBetweenBaseTypeAndDerived(MetadataType baseType, MetadataType derivedType);

/// <summary>
/// List of input modules to use for the compilation.
/// </summary>
Expand Down
19 changes: 14 additions & 5 deletions ...s/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/FieldFixupSignature.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,22 +41,31 @@ public override ObjectData GetData(NodeFactory factory, bool relocsOnly = false)

EcmaModule targetModule = factory.SignatureContext.GetTargetModule(_fieldDesc);
SignatureContext innerContext = dataBuilder.EmitFixup(factory, _fixupKind, targetModule, factory.SignatureContext);
uint baseOffset = 0;
uint fieldOffset = (uint)_fieldDesc.Offset.AsInt;

if (_fixupKind == ReadyToRunFixupKind.Verify_FieldOffset)
{
TypeDesc baseType = _fieldDesc.OwningType.BaseType;
if ((_fieldDesc.OwningType.BaseType != null) && !_fieldDesc.IsStatic && !_fieldDesc.OwningType.IsValueType)
if ((_fieldDesc.OwningType.BaseType != null)
&& !_fieldDesc.IsStatic
&& !_fieldDesc.OwningType.IsValueType)
{
dataBuilder.EmitUInt((uint)_fieldDesc.OwningType.FieldBaseOffset().AsInt);
MetadataType owningType = (MetadataType)_fieldDesc.OwningType;
baseOffset = (uint)owningType.FieldBaseOffset().AsInt;
if (factory.CompilationModuleGroup.NeedsAlignmentBetweenBaseTypeAndDerived((MetadataType)baseType, owningType))
{
fieldOffset -= baseOffset;
baseOffset = 0;
}
}
else
dataBuilder.EmitUInt(0);
dataBuilder.EmitUInt(baseOffset);
}

if ((_fixupKind == ReadyToRunFixupKind.Check_FieldOffset) ||
(_fixupKind == ReadyToRunFixupKind.Verify_FieldOffset))
{
dataBuilder.EmitUInt((uint)_fieldDesc.Offset.AsInt);
dataBuilder.EmitUInt(fieldOffset);
}

dataBuilder.EmitFieldSignature(_fieldDesc, innerContext);
Expand Down
5 changes: 4 additions & 1 deletion src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,11 @@ private bool ModuleMatchesCompilationUnitIndex(ModuleDesc module1, ModuleDesc mo
return ModuleToCompilationUnitIndex(module1) == ModuleToCompilationUnitIndex(module2);
}

public bool NeedsAlignmentBetweenBaseTypeAndDerived(MetadataType baseType, MetadataType derivedType)
public override bool NeedsAlignmentBetweenBaseTypeAndDerived(MetadataType baseType, MetadataType derivedType)
{
if (baseType.IsObject)
return false;

if (!ModuleMatchesCompilationUnitIndex(derivedType.Module, baseType.Module) ||
TypeLayoutCompilationUnits(baseType).HasMultipleCompilationUnits)
{
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ public override FieldLayoutAlgorithm GetLayoutAlgorithmForType(DefType type)
}
}

/// <summary>
/// This is a rough equivalent of the CoreCLR runtime method ReadyToRunInfo::GetFieldBaseOffset.
/// In contrast to the auto field layout algorithm, this method unconditionally applies alignment
/// between base and derived class (even when they reside in the same version bubble).
/// </summary>
public LayoutInt CalculateFieldBaseOffset(MetadataType type) => _r2rFieldLayoutAlgorithm.CalculateFieldBaseOffset(type, type.RequiresAlign8(), requiresAlignedBase: true);

public void SetCompilationGroup(ReadyToRunCompilationModuleGroupBase compilationModuleGroup)
{
_r2rFieldLayoutAlgorithm.SetCompilationGroup(compilationModuleGroup);
Expand Down
Loading

0 comments on commit a7ed1fd

Please sign in to comment.