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

GCCoverageInfo support 32-bit Tiered compilation #37116

Merged
merged 2 commits into from
May 28, 2020

Conversation

AaronRobinsonMSFT
Copy link
Member

Update the GC coverage info code path to respect Tiered compilation on a 32-bit platform.

Fixes #36366

/cc @AndyAyersMS @kouvel

Update the GC coverage info code path to respect Tiered compilation
  on a 32-bit platform.
Copy link
Member

@kouvel kouvel 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!

@AndyAyersMS
Copy link
Member

Seems a little odd to me that native code versions aren't tied in more closely to the jit manager and so can be obtained from a code address with less ceremony.

@AaronRobinsonMSFT have you checked that this fixes the test case?

@AaronRobinsonMSFT
Copy link
Member Author

@AndyAyersMS Yep. I locally validated this fixes the issue reported in #36366. I also ran through several other GC stress scenarios in other tests.

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as ready for review May 28, 2020 19:15
@AaronRobinsonMSFT
Copy link
Member Author

Seems a little odd to me that native code versions aren't tied in more closely to the jit manager and so can be obtained from a code address with less ceremony.

@AndyAyersMS I agree. I wish I knew the history behind this design. It makes bugs like this feel even more like the tip of the iceberg rather than one offs.

Does my change here align with your approach?

@AndyAyersMS
Copy link
Member

Yes, it's the same approach that I started working up. I hit the lock level assert and didn't go further.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit fd62c7f into dotnet:master May 28, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_36366 branch May 28, 2020 20:35
@kouvel
Copy link
Member

kouvel commented May 28, 2020

Seems a little odd to me that native code versions aren't tied in more closely to the jit manager and so can be obtained from a code address with less ceremony.

Yea maybe the native code version could be stored instead of the method desc or something like that. Not sure about fragile ngen images and its jit manager, maybe it had something to do with that, or the size since it's a bit larger. Would be nice to be able to get the code version without a lock.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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.

Assert in DoubleAlign scenario with GCStress and TieredCompilation
3 participants