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

[Mono] Fix support for nested structs with explicit layout #61467

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Nov 11, 2021

The current implementation of explicit struct layout check doesn't consider the layout of embedded value types. This new implementation recursively checks the inner structure of all embedded structs.

Fixes #61385

@ghost
Copy link

ghost commented Nov 11, 2021

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

WIP

Fixes #61385

Author: simonrozsival
Assignees: -
Labels:

area-VM-meta-mono

Milestone: -

@simonrozsival simonrozsival marked this pull request as ready for review November 25, 2021 07:40
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, Simon!

I suggested to use the m_class_XYZ getters for accessing MonoClass fields in a couple of places. Technically class-init.c is allowed to use the fields directly, it's probably a good idea to only do it when you're also initializing the MonoClass.

src/mono/mono/metadata/class-init.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/class-init.c Outdated Show resolved Hide resolved
src/mono/mono/metadata/class-init.c Outdated Show resolved Hide resolved
eng/pipelines/runtime.yml Outdated Show resolved Hide resolved
@simonrozsival
Copy link
Member Author

Thanks for the feedback @lambdageek! I replaced the direct accesses with those getters as you suggested.

@simonrozsival simonrozsival merged commit 64d1276 into dotnet:main Dec 2, 2021
@simonrozsival simonrozsival deleted the simonrozsival/61385-nested-struct-with-explicit-layout branch December 2, 2021 13:08
agocke added a commit to agocke/runtime that referenced this pull request Dec 3, 2021
lambdageek added a commit to lambdageek/runtime that referenced this pull request Dec 13, 2021
lambdageek added a commit that referenced this pull request Dec 14, 2021
* [tests] Re-enable some explicit layout tests on Mono

   These were fixed by #61467

   Related to #36112

* objref and non-objref overlap tests expected to fail AOT compilation

   As well as the NestedStructs tests.

   Resolves #62567
@ghost ghost locked as resolved and limited conversation to collaborators Jan 1, 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.

[mono] class initializaiton overlap check does not support nested structs with explicit layout
3 participants