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.

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 one more Crossgen2 field layout mismatch with runtime #73978

Merged
merged 3 commits into from
Aug 20, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Aug 15, 2022

The field offset mismatch in the test

JIT/Regression/JitBlue/Runtime_60035/Runtime_60035.csproj

specific to Crossgen2 composite mode is due to the recent
proliferation of 16-byte alignment specific to Vector across
the runtime repo. Previously, the largest supported alignment
was 8 and X86 was the only architecture not respecting it.

In general the problem is caused by the fact that the native
CoreCLR runtime method table builder calculates field offsets
without taking the method table pointer into account; in the
particular case of the Runtime_60035 test, the class
System.Text.Encodings.Web.OptimizedInboxTextEncoder
has a field named _allowedAsciiCodePoints that is 16-aligned
that exposes this inconsistency.

Thanks

Tomas

/cc @dotnet/crossgen-contrib

Fixes #73728

@trylek
Copy link
Member Author

trylek commented Aug 15, 2022

/azp run runtime-coreclr crossgen2-composite

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -545,7 +545,7 @@ protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(MetadataType type,
// 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)
if (!type.IsValueType && cumulativeInstanceFieldPos != LayoutInt.Zero && type.Context.Target.Architecture != TargetArchitecture.ARM)
Copy link
Member

Choose a reason for hiding this comment

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

This function needs a comment describing what's happening here, and we need a test where we have Sequential field layout used for classes as well as just the standard auto layout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks David for your feedback. I have improved the code comment and I added a pair of tests exercising the 16-layout with classes using both Auto and Sequential layout. Could you please take another look when you have a chance?

@mangod9
Copy link
Member

mangod9 commented Aug 16, 2022

There is one test failing in the IL.ClassLayout tests. Given this error looks like the test is expecting a nondeterministic layout:

Assert.Equal() Failure\nExpected: Indeterminate\nActual: 8

@davidwrighton
Copy link
Member

@mangod9, @trylek, the failing test is testing universal generic layout behavior, which I don't believe has been actually correct since we implemented auto layout. We can comment that test out.

@mangod9
Copy link
Member

mangod9 commented Aug 18, 2022

@mangod9, @trylek, the failing test is testing universal generic layout behavior, which I don't believe has been actually correct since we implemented auto layout. We can comment that test out.

or perhaps just update it to expect 8. Or is that non-deterministic still?

The field offset mismatch in the test

JIT/Regression/JitBlue/Runtime_60035/Runtime_60035.csproj

specific to Crossgen2 composite mode is due to the recent
proliferation of 16-byte alignment specific to Vector across
the runtime repo. Previously, the largest supported alignment
was 8 and X86 was the only architecture not respecting it.

In general the problem is caused by the fact that the native
CoreCLR runtime method table builder calculates field offsets
without taking the method table pointer into account; in the
particular case of the Runtime_60035 test, the class
System.Text.Encodings.Web.OptimizedInboxTextEncoder
has a field named _allowedAsciiCodePoints that is 16-aligned
that exposes this inconsistency.

Thanks

Tomas
@trylek trylek merged commit fad309d into dotnet:main Aug 20, 2022
@trylek trylek deleted the 60035 branch August 20, 2022 12:55
@trylek
Copy link
Member Author

trylek commented Aug 20, 2022

/backport to release/7.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/7.0-rc1: https://github.com/dotnet/runtime/actions/runs/2894813266

@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crossgen2/VM layout mismatch in CI
3 participants