From 0953757c1edd732b0cdbd6e67839965218de52b1 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Fri, 9 Aug 2024 16:51:48 -0700 Subject: [PATCH] Cache the path on `Module` for easier diagnostics (#106103) - Make `PEImage` take a path in its constructor and make its member const - Store the path when initializing `Module` This will let us avoid having to expose PEAssembly/PEImage in data descriptors to the cDAC just to get the module path. --- src/coreclr/inc/sstring.inl | 6 ++++++ src/coreclr/vm/ceeload.cpp | 2 ++ src/coreclr/vm/ceeload.h | 13 ++++++++----- src/coreclr/vm/peimage.cpp | 7 +++---- src/coreclr/vm/peimage.h | 8 ++++---- src/coreclr/vm/peimage.inl | 16 +++++++--------- 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/coreclr/inc/sstring.inl b/src/coreclr/inc/sstring.inl index 60ec60511922c..0b78ec3bc3aaf 100644 --- a/src/coreclr/inc/sstring.inl +++ b/src/coreclr/inc/sstring.inl @@ -231,6 +231,9 @@ inline SString::SString(const WCHAR *string) Set(string); + _ASSERTE(IsRepresentation(REPRESENTATION_UNICODE)); + SetNormalized(); + SS_RETURN; } @@ -249,6 +252,9 @@ inline SString::SString(const WCHAR *string, COUNT_T count) Set(string, count); + _ASSERTE(IsRepresentation(REPRESENTATION_UNICODE)); + SetNormalized(); + SS_RETURN; } diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 088ee89a07f56..eb426cd668f82 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -414,6 +414,8 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) m_loaderAllocator = GetAssembly()->GetLoaderAllocator(); m_pSimpleName = m_pPEAssembly->GetSimpleName(); + m_path = m_pPEAssembly->GetPath().GetUnicode(); + _ASSERTE(m_path != NULL); m_baseAddress = m_pPEAssembly->HasLoadedPEImage() ? m_pPEAssembly->GetLoadedLayout()->GetBase() : NULL; if (m_pPEAssembly->IsReflectionEmit()) m_dwTransientFlags |= IS_REFLECTION_EMIT; diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 36cb346cb0ec3..609f5a884c302 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -43,11 +43,9 @@ #include "ilinstrumentation.h" #include "codeversion.h" -class Stub; class MethodDesc; class FieldDesc; class Crst; -class ClassConverter; class RefClassWriter; class ReflectionModule; class EEStringData; @@ -56,11 +54,9 @@ class SigTypeContext; class Assembly; class BaseDomain; class AppDomain; -class DomainModule; class SystemDomain; class Module; class SString; -class Pending; class MethodTable; class DynamicMethodTable; class TieredCompilationManager; @@ -615,6 +611,7 @@ class Module : public ModuleBase private: PTR_CUTF8 m_pSimpleName; // Cached simple name for better performance and easier diagnostics + const WCHAR* m_path; // Cached path for easier diagnostics PTR_PEAssembly m_pPEAssembly; PTR_VOID m_baseAddress; // Cached base address for easier diagnostics @@ -1384,7 +1381,13 @@ class Module : public ModuleBase } HRESULT GetScopeName(LPCUTF8 * pszName) { WRAPPER_NO_CONTRACT; return m_pPEAssembly->GetScopeName(pszName); } - const SString &GetPath() { WRAPPER_NO_CONTRACT; return m_pPEAssembly->GetPath(); } + const SString &GetPath() + { + WRAPPER_NO_CONTRACT; + // Validate the pointers are the same to ensure the lifetime of m_path is handled. + _ASSERTE(m_path == m_pPEAssembly->GetPath().GetUnicode()); + return m_pPEAssembly->GetPath(); + } #ifdef LOGGING LPCUTF8 GetDebugName() { WRAPPER_NO_CONTRACT; return m_pPEAssembly->GetDebugName(); } diff --git a/src/coreclr/vm/peimage.cpp b/src/coreclr/vm/peimage.cpp index 84d31686d49ce..5de04622a73e3 100644 --- a/src/coreclr/vm/peimage.cpp +++ b/src/coreclr/vm/peimage.cpp @@ -615,9 +615,8 @@ void PEImage::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) #endif // #ifdef DACCESS_COMPILE - -PEImage::PEImage(): - m_path(), +PEImage::PEImage(const WCHAR* path): + m_path{path}, m_pathHash(0), m_refCount(1), m_bInHashMap(FALSE), @@ -788,7 +787,7 @@ PTR_PEImage PEImage::CreateFromByteArray(const BYTE* array, COUNT_T size) } CONTRACT_END; - PEImageHolder pImage(new PEImage()); + PEImageHolder pImage(new PEImage(NULL /*path*/)); PTR_PEImageLayout pLayout = PEImageLayout::CreateFromByteArray(pImage, array, size); _ASSERTE(!pLayout->IsMapped()); diff --git a/src/coreclr/vm/peimage.h b/src/coreclr/vm/peimage.h index e7bfc11d319f1..0d4a0fb8bb549 100644 --- a/src/coreclr/vm/peimage.h +++ b/src/coreclr/vm/peimage.h @@ -104,7 +104,7 @@ class PEImage final static void Startup(); ~PEImage(); - PEImage(); + explicit PEImage(const WCHAR* path); BOOL Equals(PEImage* pImage); @@ -121,7 +121,7 @@ class PEImage final MDInternalImportFlags flags = MDInternalImport_Default, BundleFileLocation bundleFileLocation = BundleFileLocation::Invalid()); - static PTR_PEImage FindByPath(LPCWSTR pPath, BOOL isInBundle = TRUE); + static PTR_PEImage FindByPath(LPCWSTR pPath, BOOL isInBundle); void AddToHashMap(); #endif @@ -206,7 +206,7 @@ class PEImage final // Private routines // ------------------------------------------------------------ - void Init(LPCWSTR pPath, BundleFileLocation bundleFileLocation); + void Init(BundleFileLocation bundleFileLocation); struct PEImageLocator { @@ -285,7 +285,7 @@ class PEImage final // Instance fields // ------------------------------------------------------------ - SString m_path; + const SString m_path; ULONG m_pathHash; LONG m_refCount; diff --git a/src/coreclr/vm/peimage.inl b/src/coreclr/vm/peimage.inl index 726c0d65b6bf5..f04078fe6455a 100644 --- a/src/coreclr/vm/peimage.inl +++ b/src/coreclr/vm/peimage.inl @@ -276,7 +276,7 @@ inline CHECK PEImage::CheckFormat() CHECK_OK; } -inline void PEImage::Init(LPCWSTR pPath, BundleFileLocation bundleFileLocation) +inline void PEImage::Init(BundleFileLocation bundleFileLocation) { CONTRACTL { @@ -286,8 +286,6 @@ inline void PEImage::Init(LPCWSTR pPath, BundleFileLocation bundleFileLocation) } CONTRACTL_END; - m_path.Set(pPath); - m_path.Normalize(); m_pathHash = m_path.HashCaseInsensitive(); m_bundleFileLocation = bundleFileLocation; SetModuleFileNameHintForDAC(); @@ -296,7 +294,7 @@ inline void PEImage::Init(LPCWSTR pPath, BundleFileLocation bundleFileLocation) /*static*/ -inline PTR_PEImage PEImage::FindByPath(LPCWSTR pPath, BOOL isInBundle /* = TRUE */) +inline PTR_PEImage PEImage::FindByPath(LPCWSTR pPath, BOOL isInBundle) { CONTRACTL { @@ -316,13 +314,13 @@ inline PTR_PEImage PEImage::FindByPath(LPCWSTR pPath, BOOL isInBundle /* = TRUE } /* static */ -inline PTR_PEImage PEImage::OpenImage(LPCWSTR pPath, MDInternalImportFlags flags /* = MDInternalImport_Default */, BundleFileLocation bundleFileLocation) +inline PTR_PEImage PEImage::OpenImage(LPCWSTR pPath, MDInternalImportFlags flags /* = MDInternalImport_Default */, BundleFileLocation bundleFileLocation /* = BundleFileLocation::Invalid() */) { BOOL forbidCache = (flags & MDInternalImport_NoCache); if (forbidCache) { - PEImageHolder pImage(new PEImage); - pImage->Init(pPath, bundleFileLocation); + PEImageHolder pImage(new PEImage{pPath}); + pImage->Init(bundleFileLocation); return dac_cast(pImage.Extract()); } @@ -337,8 +335,8 @@ inline PTR_PEImage PEImage::OpenImage(LPCWSTR pPath, MDInternalImportFlags flags return NULL; } - PEImageHolder pImage(new PEImage); - pImage->Init(pPath, bundleFileLocation); + PEImageHolder pImage(new PEImage{pPath}); + pImage->Init(bundleFileLocation); pImage->AddToHashMap(); return dac_cast(pImage.Extract());