Skip to content

Keep using RO metadata after creating IMetaDataImporter for PDB reading #93760

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

Closed
wants to merge 1 commit into from
Closed

Keep using RO metadata after creating IMetaDataImporter for PDB reading #93760

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
10 changes: 5 additions & 5 deletions src/coreclr/vm/ceeload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2102,7 +2102,7 @@ BOOL Module::IsInSameVersionBubble(Module *target)

//---------------------------------------------------------------------------------------
//
// Wrapper for Module::GetRWImporter + QI when writing is not needed.
// Wrapper for Module::GetImporter + QI when writing is not needed.
//
// Arguments:
// * dwOpenFlags - Combo from CorOpenFlags. Better not contain ofWrite!
Expand All @@ -2113,7 +2113,7 @@ BOOL Module::IsInSameVersionBubble(Module *target)
// Return Value:
// HRESULT indicating success or failure.
//
HRESULT Module::GetReadablePublicMetaDataInterface(DWORD dwOpenFlags, REFIID riid, LPVOID * ppvInterface)
HRESULT Module::GetReadablePublicMetaDataInterface(DWORD dwOpenFlags, REFIID riid, bool swapForRWMDImport, LPVOID * ppvInterface)
{
CONTRACTL
{
Expand All @@ -2137,7 +2137,7 @@ HRESULT Module::GetReadablePublicMetaDataInterface(DWORD dwOpenFlags, REFIID rii
// Normally, we just get an RWImporter to do the QI on, and we're on our way.
EX_TRY
{
pIUnk = GetRWImporter();
pIUnk = GetRWImporter(swapForRWMDImport);
}
EX_CATCH_HRESULT_NO_ERRORINFO(hr);

Expand Down Expand Up @@ -2306,7 +2306,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void)
}
if (SUCCEEDED(hr))
{
hr = pBinder->GetReaderFromStream(GetRWImporter(), pIStream, &pReader);
hr = pBinder->GetReaderFromStream(GetRWImporter(/* swapForRWMDImport */ false), pIStream, &pReader);
}
}
else
Expand All @@ -2318,7 +2318,7 @@ ISymUnmanagedReader *Module::GetISymUnmanagedReader(void)
// trying to get a symbol reader. This has to be done once per
// Assembly.
ReleaseHolder<IUnknown> pUnk = NULL;
hr = GetReadablePublicMetaDataInterface(ofReadOnly, IID_IMetaDataImport, &pUnk);
hr = GetReadablePublicMetaDataInterface(ofReadOnly, IID_IMetaDataImport, /* swapForRWMDImport */ false, &pUnk);
if (SUCCEEDED(hr))
hr = pBinder->GetReaderForFile(pUnk, path, NULL, &pReader);
}
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/vm/ceeload.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,14 +1042,14 @@ class Module : public ModuleBase
return m_pPEAssembly->GetEmitter();
}

IMetaDataImport2 *GetRWImporter()
IMetaDataImport2 *GetRWImporter(bool swapForRWMDImport = true)
{
WRAPPER_NO_CONTRACT;

return m_pPEAssembly->GetRWImporter();
return m_pPEAssembly->GetRWImporter(swapForRWMDImport);
}

HRESULT GetReadablePublicMetaDataInterface(DWORD dwOpenFlags, REFIID riid, LPVOID * ppvInterface);
HRESULT GetReadablePublicMetaDataInterface(DWORD dwOpenFlags, REFIID riid, bool swapForRWMDImport, LPVOID * ppvInterface);
#endif // !DACCESS_COMPILE

#if defined(FEATURE_READYTORUN)
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/vm/method.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,14 +836,6 @@ class MethodDesc
PREFIX_ASSUME(pModule != NULL);
return pModule->GetEmitter();
}

IMetaDataImport* GetRWImporter()
{
WRAPPER_NO_CONTRACT;
Module *pModule = GetModule();
PREFIX_ASSUME(pModule != NULL);
return pModule->GetRWImporter();
}
#endif // !DACCESS_COMPILE

#ifdef FEATURE_COMINTEROP
Expand Down
90 changes: 68 additions & 22 deletions src/coreclr/vm/peassembly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ TADDR PEAssembly::GetIL(RVA il)

#ifndef DACCESS_COMPILE

void PEAssembly::OpenImporter()
void PEAssembly::OpenImporter(bool swapForRWMDImport)
{
CONTRACTL
{
Expand All @@ -296,10 +296,10 @@ void PEAssembly::OpenImporter()
CONTRACTL_END;

// Make sure internal MD is in RW format.
ConvertMDInternalToReadWrite();
ConvertMDInternalToReadWrite(swapForRWMDImport);

IMetaDataImport2 *pIMDImport = NULL;
IfFailThrow(GetMetaDataPublicInterfaceFromInternal((void*)GetMDImport(),
IfFailThrow(GetMetaDataPublicInterfaceFromInternal((void*)m_pConvertedMDImport,
IID_IMetaDataImport2,
(void **)&pIMDImport));

Expand All @@ -308,7 +308,7 @@ void PEAssembly::OpenImporter()
pIMDImport->Release();
}

void PEAssembly::ConvertMDInternalToReadWrite()
void PEAssembly::ConvertMDInternalToReadWrite(bool swapForRWMDImport)
{
CONTRACTL
{
Expand All @@ -320,12 +320,37 @@ void PEAssembly::ConvertMDInternalToReadWrite()
}
CONTRACTL_END;

IMDInternalImport *pOld; // Old (current RO) value of internal import.
IMDInternalImport *pNew = NULL; // New (RW) value of internal import.

// Take a local copy of *ppImport. This may be a pointer to an RO
// Take a local copy of the assembly's MD import. This may be a pointer to an RO
// or to an RW MDInternalXX.
pOld = m_pMDImport;
IMDInternalImport *pNew = NULL; // New (RW) value of internal import.
IMDInternalImport *pOld = m_pMDImport; // Old (current RO) value of internal import.
IMDInternalImport *pConvertedImport = m_pConvertedMDImport; // Pointer to a RW interna metadata that might not be actively in use.


if (pConvertedImport != NULL)
{
// TODO: Is this a good check if metadata is read write.
if (swapForRWMDImport && pOld != pConvertedImport)
{
// We had converted the metadata before, now we are just requested
// to replace the RO view we had with the converted one.
if (InterlockedCompareExchangeT(&m_pMDImport, pConvertedImport, pOld) == pOld)
{
m_pMDImport->AddRef();
HRESULT hr=m_pConvertedMDImport->SetUserContextData(pOld);
_ASSERTE(SUCCEEDED(hr)||!"Leaking old MDImport");
IfFailThrow(hr);
}
else
{
// Even if we lost the race, the fields should now be the same.
_ASSERTE(pConvertedImport == m_pMDImport);
}
}

return;
}

IMetaDataImport *pIMDImport = m_pImporter;
if (pIMDImport != NULL)
{
Expand All @@ -334,8 +359,10 @@ void PEAssembly::ConvertMDInternalToReadWrite()
{
EX_THROW(EEMessageException, (hr));
}
if (pNew == pOld)

if ((m_pMDImport == m_pConvertedMDImport && pNew == pOld) || !swapForRWMDImport)
{
_ASSERTE(m_pConvertedMDImport != NULL);
pNew->Release();
return;
}
Expand All @@ -359,19 +386,31 @@ void PEAssembly::ConvertMDInternalToReadWrite()
// Swap the pointers in a thread safe manner. If the contents of *ppImport
// equals pOld then no other thread got here first, and the old contents are
// replaced with pNew. The old contents are returned.
if (InterlockedCompareExchangeT(&m_pMDImport, pNew, pOld) == pOld)
{
//if the debugger queries, it will now see that we have RW metadata
m_MDImportIsRW_Debugger_Use_Only = TRUE;

// Swapped -- get the metadata to hang onto the old Internal import.
HRESULT hr=m_pMDImport->SetUserContextData(pOld);
_ASSERTE(SUCCEEDED(hr)||!"Leaking old MDImport");
IfFailThrow(hr);
}
else
{ // Some other thread finished first. Just free the results of this conversion.
if (InterlockedCompareExchangeT(&m_pConvertedMDImport, pNew, NULL) != NULL)
{
// If we lost the race, free our copy of the RW MD. However, we might still
// be the only requesting swapping and our caller would expect m_pMDImport to
// be RW. Try to set it to the converted import.
pNew->Release();
pNew = m_pConvertedMDImport;
}

// TODO: this can still be torn? Struct with pointers and swapping might be the only good option.
// need to see what pointers the struct should have.
if (swapForRWMDImport)
{
if (InterlockedCompareExchangeT(&m_pMDImport, pNew, pOld) == pOld)
{
//if the debugger queries, it will now see that we have RW metadata
m_MDImportIsRW_Debugger_Use_Only = TRUE;

// Swapped -- get the metadata to hang onto the old Internal import.
m_pMDImport->AddRef();
HRESULT hr=m_pMDImport->SetUserContextData(pOld);
_ASSERTE(SUCCEEDED(hr)||!"Leaking old MDImport");
IfFailThrow(hr);
}
}
}

Expand Down Expand Up @@ -417,7 +456,7 @@ void PEAssembly::OpenEmitter()
CONTRACTL_END;

// Make sure internal MD is in RW format.
ConvertMDInternalToReadWrite();
ConvertMDInternalToReadWrite(/* swapForRWMDImport */ true);

IMetaDataEmit *pIMDEmit = NULL;
IfFailThrow(GetMetaDataPublicInterfaceFromInternal((void*)GetMDImport(),
Expand Down Expand Up @@ -673,6 +712,7 @@ PEAssembly::PEAssembly(
#endif // LOGGING
m_PEImage = NULL;
m_MDImportIsRW_Debugger_Use_Only = FALSE;
m_pConvertedMDImport = NULL;
m_pMDImport = NULL;
m_pImporter = NULL;
m_pEmitter = NULL;
Expand Down Expand Up @@ -787,6 +827,12 @@ PEAssembly::~PEAssembly()
m_pMDImport = NULL;
}

if (m_pConvertedMDImport != NULL)
{
m_pConvertedMDImport->Release();
m_pConvertedMDImport = NULL;
}

if (m_PEImage != NULL)
m_PEImage->Release();

Expand Down
8 changes: 5 additions & 3 deletions src/coreclr/vm/peassembly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,12 @@ class PEAssembly final

#ifndef DACCESS_COMPILE
IMetaDataEmit *GetEmitter();
IMetaDataImport2 *GetRWImporter();
IMetaDataImport2 *GetRWImporter(bool swapForRWMDImport = true);
#else
TADDR GetMDInternalRWAddress();
#endif // DACCESS_COMPILE

void ConvertMDInternalToReadWrite();
void ConvertMDInternalToReadWrite(bool swapForRWMDImport = true);

void GetMVID(GUID* pMvid);
ULONG GetHashAlgId();
Expand Down Expand Up @@ -382,7 +382,7 @@ class PEAssembly final
#endif

void OpenMDImport();
void OpenImporter();
void OpenImporter(bool swapForRWMDImport = true);
void OpenEmitter();

private:
Expand Down Expand Up @@ -420,6 +420,8 @@ class PEAssembly final
#endif
};

IMDInternalImport* m_pConvertedMDImport;

IMetaDataImport2* m_pImporter;
IMetaDataEmit* m_pEmitter;

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/peassembly.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ inline IMDInternalImport* PEAssembly::GetMDImport()

#ifndef DACCESS_COMPILE

inline IMetaDataImport2 *PEAssembly::GetRWImporter()
inline IMetaDataImport2 *PEAssembly::GetRWImporter(bool swapForRWMDImport)
{
CONTRACT(IMetaDataImport2 *)
{
Expand All @@ -288,7 +288,7 @@ inline IMetaDataImport2 *PEAssembly::GetRWImporter()
CONTRACT_END;

if (m_pImporter == NULL)
OpenImporter();
OpenImporter(swapForRWMDImport);

RETURN m_pImporter;
}
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/proftoeeinterfaceimpl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2172,7 +2172,7 @@ HRESULT ProfToEEInterfaceImpl::GetTokenAndMetaDataFromFunction(
if (ppOut)
{
Module * pMod = pMD->GetModule();
hr = pMod->GetReadablePublicMetaDataInterface(ofRead, riid, (LPVOID *) ppOut);
hr = pMod->GetReadablePublicMetaDataInterface(ofRead, riid, /* swapForRWMDImport */ true, (LPVOID *) ppOut);
}

return hr;
Expand Down Expand Up @@ -4254,7 +4254,7 @@ HRESULT ProfToEEInterfaceImpl::GetModuleMetaData(ModuleID moduleId,
if ((dwOpenFlags & ofWrite) == 0)
{
// Readable interface
return pModule->GetReadablePublicMetaDataInterface(dwOpenFlags, riid, (LPVOID *) ppOut);
return pModule->GetReadablePublicMetaDataInterface(dwOpenFlags, riid, /* swapForRWMDImport */ true, (LPVOID *) ppOut);
}

// Writeable interface
Expand Down