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

Change all "unmanaged" (no GC fields) sequential types to have sequential layout. #61759

Merged
merged 36 commits into from
Dec 15, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Nov 18, 2021

This is a prereq for #60639

This enables us to use the managed layout as the native layout for all LayoutKind.Sequential "C# unmanaged" structs, which is required for reconciling the "unmanaged" and "blittable" concepts.

Fixes #44579
Fixes #12977

jkoritzinsky and others added 3 commits November 29, 2021 16:21
…nMetadataFieldLayoutAlgorithm.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…all other scenarios (align-up to strictest alignment of members)
…Algorithm.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkoritzinsky
Copy link
Member Author

@jkotas I've updated this PR to now fix #12977. This is ready for another round of review.

@jkoritzinsky
Copy link
Member Author

/azp run runtime-coreclr crossgen2 outerloop,runtime-coreclr crossgen2-composite

src/coreclr/vm/class.h Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Dec 13, 2021

It looks good to me (once it builds on all platforms).

Are you happy with the test coverage we have for this, or do you plan to add more tests?

It would be useful to also get a review from @trylek or @davidwrighton .

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

I love this change, thanks so much Jeremy for finally putting some order to all this mess!

src/coreclr/vm/classlayoutinfo.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/classlayoutinfo.cpp Show resolved Hide resolved
@jkoritzinsky
Copy link
Member Author

/azp run runtime-coreclr crossgen2 outerloop,runtime-coreclr crossgen2-composite

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jkoritzinsky
Copy link
Member Author

/azp run runtime-coreclr crossgen2 outerloop,runtime-coreclr crossgen2-composite

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jkoritzinsky
Copy link
Member Author

It looks like the macOS ARM64 test failures are #62747 due to the following multidimensional array:

private static readonly string[,] s_fgbgAndColorStrings = new string[2, 16]; // 2 == fg vs bg, 16 == ConsoleColor values

@jkoritzinsky
Copy link
Member Author

I've confirmed these failures are also in main now that #61938 has been merged long enough to have outerloop runs triggered against it.

I think the test coverage already have makes me comfortable enough to merge this PR. I'll add more test coverage when I implement #60639.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants