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

Jump to bottom

Fix regression test 46239 with Crossgen2 and improve runtime logging #51416

Closed
wants to merge 16 commits into from
Closed

Fix regression test 46239 with Crossgen2 and improve runtime logging #51416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
1ce126a
Fix regression test 46239 and improve runtime logging
trylek Apr 16, 2021
54389f7
One more fix for explicit layout with zero fields
trylek Apr 18, 2021
1d21a98
Switch over the runtime outerloop pipeline to use Crossgen2
trylek Apr 17, 2021
9e4b351
Remove no longer needed R2RMFLA override of IsBlittableOrManagedSeque…
trylek May 7, 2021
fc6d7e1
Start transforming the change based on JanK's PR feedback
trylek May 7, 2021
84980df
Refactor runtime explicit / sequential behavior per JanK's feedback
trylek May 11, 2021
ef85fac
Fix Crossgen2 calculation of explicit layout size based on runtime
trylek May 13, 2021
783b02f
One more fix for the explicit layout tests
trylek May 13, 2021
a2fff2b
Put back static modifier on ComputeExplicitFieldLayout per David's PR…
trylek May 13, 2021
464c9b1
Simplify sequential layout check per JanK's suggestion
trylek May 21, 2021
46b95f0
Make changes internally consistent and address DavidWr's offline feed…
trylek May 22, 2021
11cc7df
Additional consistency fixes for x86
trylek May 22, 2021
531a460
Fixes for x86 corner cases including a pre-existing blittability bug
trylek May 24, 2021
c9297dd
Fix pre-existing solution consistency mismatch in Crossgen2.sln
trylek May 24, 2021
d4da238
Revert change to ClassLoader::CreateTypeHandleForTypeDefThrowing
trylek May 24, 2021
3df7a59
Fix architecture-specific tests to cater for x86 Windows vs. Linux
trylek May 27, 2021
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
Next Next commit
Fix regression test 46239 and improve runtime logging 
The regression test

<code>src\tests\JIT\Regressions\JitBlue\Runtime_46239</code>

exercises various interesting corner cases of type layout that
weren't handled properly in Crossgen2 on x86 and ARM[32]. This
change fixes the remaining deficiencies and it also adds
provisions for better runtime logging upon type layout mismatches.

Thanks

Tomas
  • Loading branch information
@trylek
trylek committed May 24, 2021
commit 1ce126ab027f47db7a6147662d1fd4f8b756d593
37 changes: 29 additions & 8 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 @@ -103,6 +103,7 @@ public override ComputedInstanceFieldLayout ComputeInstanceLayout(DefType defTyp
type.Context.Target.GetWellKnownTypeSize(type),
type.Context.Target.GetWellKnownTypeAlignment(type),
0,
alignUpInstanceByteSize: true,
out instanceByteSizeAndAlignment
);

Expand Down Expand Up @@ -304,17 +305,19 @@ protected virtual void FinalizeRuntimeSpecificStaticFieldLayout(TypeSystemContex
{
}

protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType type, int numInstanceFields)
protected virtual bool IsBlittableOrManagedSequential(TypeDesc type) => false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the default here should be true. Blittable types have a requirement that they are handled in the same way on all runtimes as they describe native data structures, and by defaulting to false, this leads for a way for the algorithm to misbehave on Native AOT.

I'd prefer to see this either be default true, or make it an abstract method and force the NativeAOT compiler to deal with this when it gets there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichalStrehovsky could you comment here on what you'd like to see.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's just move IsManagedSequentialType to here and then this doesn't even need to be virtual and can just do the same thing?

We didn't have trouble with this in .NET Native (where this is missing) so I don't think it matters, but we ported over so many CoreCLR weirdness's to this algorithm over time that I no longer understand how layout is done and would prefer all bugs in it to be also surfaced in crossgen2 (and not have NativeAOT specific bugs that we need to troubleshoot there).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichalStrehovsky - Thanks for your feedback. I'm trying to modify the change based on your suggestion but I'm hitting a layering problem - IsBlittable is part of MarshalUtils and that's not in ILCompiler.TypeSystem.ReadyToRun and it seems to me that moving it over brings various bits of interop code into ILCompiler.TypeSystem.ReadyToRun which I suspect to be undesirable as my current understanding is that NativeAOT doesn't use exactly identical interop as Crossgen2 / CoreCLR. What do you think would be the ideal way to resolve this discrepancy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MarshalUtils are part of ILCompiler.TypeSystem on the NativeAOT side, so I'm not opposed to moving it (and things it depends on) into ILCompiler.TypeSystem.ReadyToRun as well. The split is rather arbitrary and interop is getting tangled into the type system the same way as is on CoreCLR. It was a nice dream to have a separation of concerns there.

https://github.com/dotnet/runtimelab/blob/c8e3158b0981419527b18f3c0259bcf02103e054/src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj#L528-L530

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The split is rather arbitrary and interop is getting tangled into the type system the same way as is on CoreCLR. It was a nice dream to have a separation of concerns there.

Would there be a way to adjust the runtime side of the things to avoid this mess?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for now I think I have at least managed to make something like an inventory of the various runtime behaviors by implementing them in Crossgen2. In theory we may be able to simplify some of the stuff, I however suspect that some of the subtle distinctions amount to tiny memory use optimizations on the runtime side i.e. something that's very tricky to undo as it may incur working set regressions in arbitrary scenarios.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however suspect that some of the subtle distinctions amount to tiny memory use optimizations on the runtime side

I doubt that it would show up on the radar. The behavior of the field layout algorithm is completely accidental in number of cases, and in fact there are many known situations where it is inefficient for no good reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're probably the biggest expert in this field so I have no desire to doubt your assessment. My only point is that further independent development of runtime vs. Crossgen2 side of this equation is giving me goosebumps; my current expectation is that we'll ultimately go down the path you yourself suggested in the past - having a way to communicate the field layouts from the compiler to the runtime, that will give us room for experimentation with potential packing optimizations and rid us of the burdensome need to keep both codebases in 100% sync.


protected ComputedInstanceFieldLayout ComputeExplicitFieldLayout(MetadataType type, int numInstanceFields)
{
// 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();
LayoutInt instanceSize = cumulativeInstanceFieldPos + new LayoutInt(layoutMetadata.Size);

int packingSize = ComputePackingSize(type, layoutMetadata);
LayoutInt largestAlignmentRequired = LayoutInt.One;
Expand Down Expand Up @@ -349,6 +352,7 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
);
if (needsToBeAligned)
{
largestAlignmentRequired = LayoutInt.Max(largestAlignmentRequired, type.Context.Target.LayoutPointerSize);
int offsetModulo = computedOffset.AsInt % type.Context.Target.PointerSize;
if (offsetModulo != 0)
{
Expand All @@ -365,7 +369,12 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
}

SizeAndAlignment instanceByteSizeAndAlignment;
var instanceSizeAndAlignment = ComputeInstanceSize(type, instanceSize, largestAlignmentRequired, layoutMetadata.Size, out instanceByteSizeAndAlignment);
var instanceSizeAndAlignment = ComputeInstanceSize(type,
instanceSize,
largestAlignmentRequired,
layoutMetadata.Size,
alignUpInstanceByteSize: IsBlittableOrManagedSequential(type),
out instanceByteSizeAndAlignment);

ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout();
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
Expand All @@ -375,7 +384,6 @@ protected static ComputedInstanceFieldLayout ComputeExplicitFieldLayout(Metadata
computedLayout.Offsets = offsets;
computedLayout.LayoutAbiStable = layoutAbiStable;


ExplicitLayoutValidator.Validate(type, computedLayout);

return computedLayout;
Expand Down Expand Up @@ -422,7 +430,13 @@ protected ComputedInstanceFieldLayout ComputeSequentialFieldLayout(MetadataType
}

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

ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout();
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
Expand Down Expand Up @@ -704,7 +718,12 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
}

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

ComputedInstanceFieldLayout computedLayout = new ComputedInstanceFieldLayout();
computedLayout.FieldAlignment = instanceSizeAndAlignment.Alignment;
Expand Down Expand Up @@ -829,7 +848,7 @@ private static int ComputePackingSize(MetadataType type, ClassLayoutMetadata lay
return layoutMetadata.PackingSize;
}

private static SizeAndAlignment ComputeInstanceSize(MetadataType type, LayoutInt instanceSize, LayoutInt alignment, int classLayoutSize, out SizeAndAlignment byteCount)
private static SizeAndAlignment ComputeInstanceSize(MetadataType type, LayoutInt instanceSize, LayoutInt alignment, int classLayoutSize, bool alignUpInstanceByteSize, out SizeAndAlignment byteCount)
{
SizeAndAlignment result;

Expand Down Expand Up @@ -857,7 +876,9 @@ private static SizeAndAlignment ComputeInstanceSize(MetadataType type, LayoutInt
{
if (type.IsValueType)
{
instanceSize = LayoutInt.AlignUp(instanceSize, alignment, target);
instanceSize = LayoutInt.AlignUp(instanceSize,
alignUpInstanceByteSize ? alignment : LayoutInt.Min(alignment, target.LayoutPointerSize),
Copy link
Member

@jkotas jkotas Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that the alignUpInstanceByteSize = false case will kick in for a struct like this on ARM32:

    [StructLayout(LayoutKind.Explicit)]
    internal struct S3
    {
        [FieldOffset(0)]
        public ulong tmp1;
        [FieldOffset(8)]
        public Object tmp2;
    }

The algorithm will compute the instanceSize of this struct as 12. Is that correct?

It looks like a bug in the type loader. The instance size of this struct on ARM should be 16, so that the long field is aligned at 8 bytes, so that potential atomic 64-bit operations work fine on it. It would be better to fix the type loader instead of replicating the bug here.

target);
}
}

Expand Down
2 changes: 2 additions & 0 deletions ...oreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunMetadataFieldLayoutAlgorithm.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,8 @@ protected override ComputedInstanceFieldLayout ComputeInstanceFieldLayout(Metada
}
}

protected override bool IsBlittableOrManagedSequential(TypeDesc type) => MarshalUtils.IsBlittableType(type) || IsManagedSequentialType(type);

/// <summary>
/// This method decides whether the type needs aligned base offset in order to have layout resilient to
/// base class layout changes.
Expand Down
100 changes: 90 additions & 10 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13524,7 +13524,7 @@ void ComputeGCRefMap(MethodTable * pMT, BYTE * pGCRefMap, size_t cbGCRefMap)
// - Alignment
// - Position of GC references
//
BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob)
BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob, BOOL printDiff)
{
STANDARD_VM_CONTRACT;

Expand All @@ -13534,13 +13534,28 @@ BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob)
uint32_t dwFlags;
IfFailThrow(p.GetData(&dwFlags));

BOOL result = TRUE;

// Size is checked unconditionally
uint32_t dwExpectedSize;
IfFailThrow(p.GetData(&dwExpectedSize));

DWORD dwActualSize = pMT->GetNumInstanceFieldBytes();
if (dwExpectedSize != dwActualSize)
return FALSE;
{
if (printDiff)
{
result = FALSE;

DefineFullyQualifiedNameForClassW();
wprintf(W("Type %s: expected size 0x%08x, actual size 0x%08x\n"),
GetFullyQualifiedNameForClassW(pMT), dwExpectedSize, dwActualSize);
}
else
{
return FALSE;
}
}

#ifdef FEATURE_HFA
if (dwFlags & READYTORUN_LAYOUT_HFA)
Expand All @@ -13550,12 +13565,38 @@ BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob)

DWORD dwActualHFAType = pMT->GetHFAType();
if (dwExpectedHFAType != dwActualHFAType)
return FALSE;
{
if (printDiff)
{
result = FALSE;

DefineFullyQualifiedNameForClassW();
wprintf(W("Type %s: expected HFA type %08x, actual %08x\n"),
GetFullyQualifiedNameForClassW(pMT), dwExpectedHFAType, dwActualHFAType);
}
else
{
return FALSE;
}
}
}
else
{
if (pMT->IsHFA())
return FALSE;
{
if (printDiff)
{
result = FALSE;

DefineFullyQualifiedNameForClassW();
wprintf(W("Type %s: type is HFA but READYTORUN_LAYOUT_HFA flag is not set\n"),
GetFullyQualifiedNameForClassW(pMT));
}
else
{
return FALSE;
}
}
}
#else
_ASSERTE(!(dwFlags & READYTORUN_LAYOUT_HFA));
Expand All @@ -13571,7 +13612,20 @@ BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob)

DWORD dwActualAlignment = CEEInfo::getClassAlignmentRequirementStatic(pMT);
if (dwExpectedAlignment != dwActualAlignment)
return FALSE;
{
if (printDiff)
{
result = FALSE;

DefineFullyQualifiedNameForClassW();
wprintf(W("Type %s: expected alignment 0x%08x, actual 0x%08x\n"),
GetFullyQualifiedNameForClassW(pMT), dwExpectedAlignment, dwActualAlignment);
}
else
{
return FALSE;
}
}

}

Expand All @@ -13580,7 +13634,20 @@ BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob)
if (dwFlags & READYTORUN_LAYOUT_GCLayout_Empty)
{
if (pMT->ContainsPointers())
return FALSE;
{
if (printDiff)
{
result = FALSE;

DefineFullyQualifiedNameForClassW();
wprintf(W("Type %s contains pointers but READYTORUN_LAYOUT_GCLayout_Empty is set\n"),
GetFullyQualifiedNameForClassW(pMT));
}
else
{
return FALSE;
}
}
}
else
{
Expand All @@ -13592,11 +13659,24 @@ BOOL TypeLayoutCheck(MethodTable * pMT, PCCOR_SIGNATURE pBlob)
ComputeGCRefMap(pMT, pGCRefMap, cbGCRefMap);

if (memcmp(pGCRefMap, p.GetPtr(), cbGCRefMap) != 0)
return FALSE;
{
if (printDiff)
{
result = FALSE;

DefineFullyQualifiedNameForClassW();
wprintf(W("Type %s: GC refmap content doesn't match\n"),
GetFullyQualifiedNameForClassW(pMT));
}
else
{
return FALSE;
}
}
}
}

return TRUE;
return result;
}

#endif // FEATURE_READYTORUN
Expand Down Expand Up @@ -14049,7 +14129,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,
MethodTable * pMT = th.AsMethodTable();
_ASSERTE(pMT->IsValueType());

if (!TypeLayoutCheck(pMT, pBlob))
if (!TypeLayoutCheck(pMT, pBlob, /* printDiff */ kind == ENCODE_VERIFY_TYPE_LAYOUT))
{
if (kind == ENCODE_CHECK_TYPE_LAYOUT)
{
Expand All @@ -14068,7 +14148,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule,
StackScratchBuffer buf;
_ASSERTE_MSG(false, fatalErrorString.GetUTF8(buf));
// Run through the type layout logic again, after the assert, makes debugging easy
TypeLayoutCheck(pMT, pBlob);
TypeLayoutCheck(pMT, pBlob, /* printDiff */ TRUE);
}
#endif

Expand Down