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

[release/7.0-rc1] Fix auto layout algorithm to compute structure alignment correctly #74091

Merged
merged 6 commits into from
Aug 18, 2022

Conversation

github-actions[bot]
Copy link
Contributor

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

Backport of #73738 to release/7.0-rc1

/cc @davidwrighton

Customer Impact

This can cause incorrect layout of structure. If that happens there can be either data corruption, or if there are object fields on the type, there may be GC holes that cause the system to crash.

Testing

Standard PR runs + additional targeted test cases.

Risk

Moderate.

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.

davidwrighton and others added 6 commits August 17, 2022 18:52
…In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8.

- The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8)

In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime.
 - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now.

Fixes #65281
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. Will merge once we have a green ci

@carlossanlop
Copy link
Member

CI passed. Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit a969995 into release/7.0-rc1 Aug 18, 2022
@carlossanlop carlossanlop deleted the backport/pr-73738-to-release/7.0-rc1 branch August 18, 2022 02:41
@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 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.

4 participants