Skip to content

Conversation

@davidwrighton
Copy link
Member

  • 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 #61471

- 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
@davidwrighton
Copy link
Member Author

@tannergooding fyi

if (compositeNativeImage->EagerFixupsHaveRun())
{
if (compositeNativeImage->ReadyToRunCodeDisabled())
GetReadyToRunInfo()->DisableAllR2RCode();
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM.

@davidwrighton davidwrighton merged commit b20c355 into dotnet:main Feb 17, 2022
@rickbrew
Copy link
Contributor

rickbrew commented Feb 17, 2022

Will this be backported to 6.0.x servicing? Also, does it fix the issue where --instruction-set:sse42 crashes on a non-SSE4.2 capable CPU? Or is this just for AVX2? #61471 (comment)

@davidwrighton
Copy link
Member Author

@rickbrew, yes this will fix the issue for --instruction-set:sse42. I'll put in a request for this to be considered in servicing. It may not be approved, as I'm not certain that we've declared that use of the --instruction-set switch is supported. (There are a number of crossgen2 switches that are not reliable and are used for engineering purposes on our team, and we only documented those which we were extremely confident were well tested.)

@davidwrighton
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

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.

LGTM, thanks David for fixing this so quickly!

DWORD m_nImportSections;

bool m_readyToRunCodeDisabled;
bool m_readyToRunCodeDisabled; // Is
Copy link
Member

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?

@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
@davidwrighton davidwrighton deleted the fix_61471 branch April 13, 2023 18:50
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.

ReadyToRun images crash if compiled for AVX2 but run on non-AVX2 CPU

4 participants