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 layout and GCInfo for ByRef fields. #64422

Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jan 28, 2022

Update tests to use recursion and validate GCStress scenarios.

Related NativeAOT/CrossGen2 change - #64366

Fixes #64465
Fixes #64466

/cc @MichalStrehovsky @dotnet/interop-contrib

@MichalStrehovsky
Copy link
Member

Tangentially related to the topic of GC holes, is there a similar problem in FindByRefPointerOffsetsInByRefLikeObject? That one is used in stack walking. Not sure how to hit that one... might be something along the lines of an interface call that takes a struct with ref fields as a parameter? It might be a good strategy to search for all occurrences of g_pByReferenceClass.

Found that one by accident when I wanted to point you to #12842 (that we should also do for byref fields due to GC tracking).

Sorry for dumping tangentially related stuff on this pull request - it should have probably gone to #63985.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Jan 28, 2022

Tangentially related to the topic of GC holes, is there a similar problem in FindByRefPointerOffsetsInByRefLikeObject?
It might be a good strategy to search for all occurrences of g_pByReferenceClass.

@MichalStrehovsky That is exactly what I've been doing for the past 40 minutes. I agree there are some issues here. Based on my searching, I think FindByRefPointerOffsetsInByRefLikeObject is the only other thing that needs to be changed. I was going to defer until we remove ByReference<T> but since you brought it up, I will apply the fixes to the other places.

it should have probably gone to #63985.

Agree.

@AaronRobinsonMSFT AaronRobinsonMSFT force-pushed the fix_gcinfo_for_ref_fields branch from f155671 to 6911ce6 Compare January 28, 2022 19:34
 - This was missed in the initial support for ref fields.
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Fix the GCInfo for types containing ByRef fields. Fix layout and GCInfo for ByRef fields. Jan 28, 2022
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM

@AaronRobinsonMSFT
Copy link
Member Author

@MichalStrehovsky I am checking this in now since it is impacting the outer loop tests. Please still give it a once over in case there is something else I'm not considering.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 6d21c93 into dotnet:main Jan 30, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the fix_gcinfo_for_ref_fields branch January 30, 2022 00:18
@MichalStrehovsky
Copy link
Member

Looks good! I filed #64520 for one of the tangentially related issues.

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