-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Disable R2R code for all associated components of a composite image #65482
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
Conversation
- When loading a composite image, capture the detail that the composite image code cannot be used into the NativeImage structure, and flow that data into all associated ReadyToRunInfo structures. Fixes dotnet#61471
|
@tannergooding fyi |
| if (compositeNativeImage->EagerFixupsHaveRun()) | ||
| { | ||
| if (compositeNativeImage->ReadyToRunCodeDisabled()) | ||
| GetReadyToRunInfo()->DisableAllR2RCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is basically just propagating up the information from compositeNativeImage, is that right? The issue being that we weren't before and so compositeNativeImage might have flagged itself as not being R2R compatible but the corresponding module loaded in the VM wouldn't track that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue was that the first module in the composite image would trigger the code to be disabled, but the second module and subsequent modules would just use the code in the composite image.
tannergooding
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
Will this be backported to 6.0.x servicing? Also, does it fix the issue where |
|
@rickbrew, yes this will fix the issue for |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1861907208 |
trylek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks David for fixing this so quickly!
| DWORD m_nImportSections; | ||
|
|
||
| bool m_readyToRunCodeDisabled; | ||
| bool m_readyToRunCodeDisabled; // Is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit - looks like an unfinished comment. If the purpose of the one-word comment is just to indicate that the flag states whether ready to run code is disabled, would it be perhaps better to directly rename the identifier to something like m_isReadyToRunCodeDisabled?
Fixes #61471