Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Aug 20, 2022

Backport of #73978 to release/7.0-rc1

/cc @trylek

Customer Impact

This bug was only found recently due to more widespread use of Vector128 in framework codebase. In classes containing 16-aligned fields, this causes a layout mismatch between Crossgen2 and native runtime with the potential to cause data corruptions, crashes or GC holes.

Testing

Innerloop & Crossgen2 composite testing (the bug was originally observed in Crossgen2 composite runs), as part of the change I wrote two more dedicated Crossgen2 typesystem tests to exercise this behavior.

Risk

Relatively low - most of the actual code logic is pre-existing, the fix only changes how it's applied to the individual architectures.

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

trylek added 3 commits August 20, 2022 12:58
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 requested review from davidwrighton and jeffschwMSFT and removed request for MichalStrehovsky August 20, 2022 13:09
@carlossanlop carlossanlop requested a review from mangod9 August 20, 2022 17:59
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved.

@jeffschwMSFT jeffschwMSFT merged commit 4823884 into release/7.0-rc1 Aug 21, 2022
@jkotas jkotas deleted the backport/pr-73978-to-release/7.0-rc1 branch August 30, 2022 21:22
@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 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.

3 participants