Skip to content

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.

Sign up for GitHub

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

Merged
davidwrighton merged 1 commit into from
Feb 17, 2022
Merged

Disable R2R code for all associated components of a composite image #65482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/coreclr/vm/ceeload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4615,16 +4615,16 @@ void Module::RunEagerFixups()
{
// For composite images, multiple modules may request initializing eager fixups
// from multiple threads so we need to lock their resolution.
if (compositeNativeImage->EagerFixupsHaveRun())
{
return;
}
CrstHolder compositeEagerFixups(compositeNativeImage->EagerFixupsLock());
if (compositeNativeImage->EagerFixupsHaveRun())
{
if (compositeNativeImage->ReadyToRunCodeDisabled())
GetReadyToRunInfo()->DisableAllR2RCode();
Copy link
Member

@tannergooding tannergooding Feb 17, 2022

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

@davidwrighton davidwrighton Feb 17, 2022

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.

return;
}
RunEagerFixupsUnlocked();
if (GetReadyToRunInfo()->ReadyToRunCodeDisabled())
compositeNativeImage->DisableAllR2RCode();
compositeNativeImage->SetEagerFixupsHaveRun();
}
else
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/vm/nativeimage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ NativeImage::NativeImage(AssemblyBinder *pAssemblyBinder, PEImageLayout *pImageL
m_pImageLayout = pImageLayout;
m_fileName = imageFileName;
m_eagerFixupsHaveRun = false;
m_readyToRunCodeDisabled = false;
}

void NativeImage::Initialize(READYTORUN_HEADER *pHeader, LoaderAllocator *pLoaderAllocator, AllocMemTracker *pamTracker)
Expand Down
13 changes: 13 additions & 0 deletions src/coreclr/vm/nativeimage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class NativeImage

Crst m_eagerFixupsLock;
bool m_eagerFixupsHaveRun;
bool m_readyToRunCodeDisabled;

private:
NativeImage(AssemblyBinder *pAssemblyBinder, PEImageLayout *peImageLayout, LPCUTF8 imageFileName);
Expand Down Expand Up @@ -126,6 +127,18 @@ class NativeImage

void CheckAssemblyMvid(Assembly *assembly) const;

void DisableAllR2RCode()
{
LIMITED_METHOD_CONTRACT;
m_readyToRunCodeDisabled = true;
}

bool ReadyToRunCodeDisabled()
{
LIMITED_METHOD_CONTRACT;
return m_readyToRunCodeDisabled;
}

private:
IMDInternalImport *LoadManifestMetadata();
};
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/vm/readytoruninfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ bool ReadyToRunInfo::GetPgoInstrumentationData(MethodDesc * pMD, BYTE** pAllocat
return false;

// If R2R code is disabled for this module, simply behave as if it is never found
if (m_readyToRunCodeDisabled)
if (ReadyToRunCodeDisabled())
return false;

if (m_pgoInstrumentationDataHashtable.IsNull())
Expand Down Expand Up @@ -890,7 +890,7 @@ PCODE ReadyToRunInfo::GetEntryPoint(MethodDesc * pMD, PrepareCodeConfig* pConfig
goto done;

// If R2R code is disabled for this module, simply behave as if it is never found
if (m_readyToRunCodeDisabled)
if (ReadyToRunCodeDisabled())
goto done;

ETW::MethodLog::GetR2RGetEntryPointStart(pMD);
Expand Down Expand Up @@ -1077,7 +1077,7 @@ BOOL ReadyToRunInfo::MethodIterator::Next()
}
CONTRACTL_END;

if (m_pInfo->m_readyToRunCodeDisabled)
if (m_pInfo->ReadyToRunCodeDisabled())
return FALSE;

// Enumerate non-generic methods
Expand Down
10 changes: 8 additions & 2 deletions src/coreclr/vm/readytoruninfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class ReadyToRunInfo
PTR_CORCOMPILE_IMPORT_SECTION m_pImportSections;
DWORD m_nImportSections;

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

@trylek trylek Feb 18, 2022

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?


NativeFormat::NativeReader m_nativeReader;
NativeFormat::NativeArray m_methodDefEntryPoints;
Expand Down Expand Up @@ -125,7 +125,13 @@ class ReadyToRunInfo
void DisableAllR2RCode()
{
LIMITED_METHOD_CONTRACT;
m_readyToRunCodeDisabled = TRUE;
m_readyToRunCodeDisabled = true;
}

bool ReadyToRunCodeDisabled()
{
LIMITED_METHOD_CONTRACT;
return m_readyToRunCodeDisabled;
}

BOOL HasNonShareablePInvokeStubs()
Expand Down