From: Mike McLaughlin Date: Tue, 2 Jul 2019 22:59:05 +0000 (-0700) Subject: Fix missing metadata problems (#370) X-Git-Tag: submit/tizen/20190813.035844~4^2^2~11 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=3d5dd561ee58258eda05a82a75704c72e20a08bf;p=platform%2Fcore%2Fdotnet%2Fdiagnostics.git Fix missing metadata problems (#370) Fix missing metadata problems The previous fix forced the metadata to always go through the metadata locator callback interface. This fix the problem when the metadata in the core dump isn't complete, but broke getting metadata for user/app assemblies that only exist in the core dump. Needed to "fallback" to reading the core dump memory for the metadata when the assembly can't be found or downloaded. Cleaned up building the module list. Add PublishPackages variable --- diff --git a/.vsts-dotnet.yml b/.vsts-dotnet.yml index d8b4a9736..39589a28e 100644 --- a/.vsts-dotnet.yml +++ b/.vsts-dotnet.yml @@ -460,7 +460,7 @@ phases: /p:DotNetSignType=$(SignType) /p:DotNetPublishBlobFeedKey=$(dotnetfeed-storage-access-key-1) /p:DotNetPublishBlobFeedUrl=$(_PublishBlobFeedUrl) - /p:DotNetPublishToBlobFeed=true + /p:DotNetPublishToBlobFeed=$(PublishPackages) /p:DotNetSymbolServerTokenMsdl=$(microsoft-symbol-server-pat) /p:DotNetSymbolServerTokenSymWeb=$(symweb-symbol-server-pat) /p:OfficialBuildId=$(BUILD.BUILDNUMBER) diff --git a/src/SOS/SOS.UnitTests/Scripts/StackAndOtherTests.script b/src/SOS/SOS.UnitTests/Scripts/StackAndOtherTests.script index c3559bbaf..ac2ec47fe 100644 --- a/src/SOS/SOS.UnitTests/Scripts/StackAndOtherTests.script +++ b/src/SOS/SOS.UnitTests/Scripts/StackAndOtherTests.script @@ -64,9 +64,6 @@ VERIFY:.*\s+\s+\s+SymbolTestApp\.Program\.Foo1\(.*\)\s+\[(?i:.*[ VERIFY:.*\s+\s+\s+SymbolTestApp\.Program\.Main\(.*\)\s+\[(?i:.*[\\|/]SymbolTestApp\.cs) @ 19\]\s* ENDIF:PROJECTK -# Issue #https://github.com/dotnet/diagnostics/issues/266 -IFDEF:NEVER - # Verify that ClrStack with the ICorDebug options works IFDEF:PROJECTK SOSCOMMAND:ClrStack -i @@ -80,6 +77,8 @@ VERIFY:.*\s+\s+\s+\[DEFAULT\] Void SymbolTestApp\.Program\.Main\ VERIFY:.*\s+Stack walk complete.\s+ ENDIF:PROJECTK +!IFDEF:ALPINE + # Verify that ClrStack with the ICorDebug options and all option (locals/params) works IFDEF:PROJECTK SOSCOMMAND:ClrStack -i -a @@ -97,10 +96,6 @@ VERIFY:.*\s+\s+\s+\[DEFAULT\] Void SymbolTestApp\.Program\.Main\ VERIFY:.*\s+Stack walk complete.\s+ ENDIF:PROJECTK -ENDIF:NEVER - -!IFDEF:ALPINE - # Verify DumpStackObjects works IFDEF:PROJECTK SOSCOMMAND:DumpStackObjects diff --git a/src/SOS/SOS.UnitTests/Scripts/StackTests.script b/src/SOS/SOS.UnitTests/Scripts/StackTests.script index 05ad94536..115509c26 100644 --- a/src/SOS/SOS.UnitTests/Scripts/StackTests.script +++ b/src/SOS/SOS.UnitTests/Scripts/StackTests.script @@ -66,9 +66,6 @@ VERIFY:\s+[r|e]ax=\s+[r|e]bx=\s+[r|e]cx=\s+ ENDIF:64BIT ENDIF:PROJECTK -# Issue #https://github.com/dotnet/diagnostics/issues/266 -IFDEF:NEVER - # 5) Verifying that ClrStack with the ICorDebug options works SOSCOMMAND:ClrStack -i VERIFY:.*\s+Dumping managed stack and managed variables using ICorDebug.\s+ @@ -79,6 +76,7 @@ VERIFY:.*\s+\s+\s+\[DEFAULT\] Void NestedExceptionTest\.Program\ VERIFY:.*\s+Stack walk complete.\s+ # 6) Verifying that ClrStack with the ICorDebug options and all option (locals/params) works +!IFDEF:ALPINE IFDEF:PROJECTK SOSCOMMAND:ClrStack -i -a VERIFY:.*\s+Dumping managed stack and managed variables using ICorDebug.\s+ @@ -92,8 +90,7 @@ VERIFY:\s+\+ System.FormatException ex @ 0x\s+ VERIFY:.*\s+\s+\s+\[DEFAULT\] Void NestedExceptionTest\.Program\.Main\(.*\)\s+\(.*\)\s+ VERIFY:.*\s+Stack walk complete.\s+ ENDIF:PROJECTK - -ENDIF:NEVER +ENDIF:ALPINE # 7) Verify DumpStackObjects works IFDEF:PROJECTK diff --git a/src/SOS/Strike/datatarget.cpp b/src/SOS/Strike/datatarget.cpp index 71970e6f3..e9bcf395d 100644 --- a/src/SOS/Strike/datatarget.cpp +++ b/src/SOS/Strike/datatarget.cpp @@ -138,13 +138,16 @@ DataTarget::ReadVirtual( #ifdef FEATURE_PAL if (g_sos != nullptr) { - // LLDB synthesizes memory (returns 0's) for missing pages (in this case the missing metadata - // pages) in core dumps. This functions creates a list of the metadata regions and returns true - // if the read would be in the metadata of a loaded assembly. This allows an error to be returned - // instead of 0's so the DAC will call the GetMetadataLocator datatarget callback. - if (IsMetadataMemory(address, request)) - { - return E_ACCESSDENIED; + // LLDB synthesizes memory (returns 0's) for missing pages (in this case the missing metadata pages) + // in core dumps. This functions creates a list of the metadata regions and caches the metadata if + // available from the local or downloaded assembly. If the read would be in the metadata of a loaded + // assembly, the metadata from the this cache will be returned. + HRESULT hr = GetMetadataMemory(address, request, buffer); + if (SUCCEEDED(hr)) { + if (done != nullptr) { + *done = request; + } + return hr; } } #endif @@ -303,14 +306,7 @@ DataTarget::GetMetadata( BYTE* buffer, /* [out] */ ULONG32* dataSize) { - HRESULT hr = InitializeHosting(); - if (FAILED(hr)) - { - return hr; - } - InitializeSymbolStore(); - _ASSERTE(g_SOSNetCoreCallbacks.GetMetadataLocatorDelegate != nullptr); - return g_SOSNetCoreCallbacks.GetMetadataLocatorDelegate(imagePath, imageTimestamp, imageSize, mvid, mdRva, flags, bufferSize, buffer, dataSize); + return ::GetMetadataLocator(imagePath, imageTimestamp, imageSize, mvid, mdRva, flags, bufferSize, buffer, dataSize); } diff --git a/src/SOS/Strike/disasm.cpp b/src/SOS/Strike/disasm.cpp index ceb0fed30..0d8356ea6 100644 --- a/src/SOS/Strike/disasm.cpp +++ b/src/SOS/Strike/disasm.cpp @@ -26,6 +26,7 @@ namespace X86GCDump #define DAC_ARG(x) #define CONTRACTL_END #define LIMITED_METHOD_CONTRACT +#undef NOTHROW #define NOTHROW #define GC_NOTRIGGER #define SUPPORTS_DAC diff --git a/src/SOS/Strike/hostcoreclr.cpp b/src/SOS/Strike/hostcoreclr.cpp index f0e23b2bf..1447718a5 100644 --- a/src/SOS/Strike/hostcoreclr.cpp +++ b/src/SOS/Strike/hostcoreclr.cpp @@ -924,6 +924,29 @@ void DisableSymbolStore() } } +/**********************************************************************\ + * Returns the metadata from a local or downloaded assembly +\**********************************************************************/ +HRESULT GetMetadataLocator( + LPCWSTR imagePath, + ULONG32 imageTimestamp, + ULONG32 imageSize, + GUID* mvid, + ULONG32 mdRva, + ULONG32 flags, + ULONG32 bufferSize, + BYTE* buffer, + ULONG32* dataSize) +{ + HRESULT hr = InitializeHosting(); + if (FAILED(hr)) { + return hr; + } + InitializeSymbolStore(); + _ASSERTE(g_SOSNetCoreCallbacks.GetMetadataLocatorDelegate != nullptr); + return g_SOSNetCoreCallbacks.GetMetadataLocatorDelegate(imagePath, imageTimestamp, imageSize, mvid, mdRva, flags, bufferSize, buffer, dataSize); +} + /**********************************************************************\ * Load symbols for an ICorDebugModule. Used by "clrstack -i". \**********************************************************************/ @@ -1193,6 +1216,9 @@ HRESULT SymbolReader::GetLineByILOffset(___in mdMethodDef methodToken, ___in ULO return E_FAIL; } +/**********************************************************************\ + * Returns the name of the local variable from a PDB. +\**********************************************************************/ HRESULT SymbolReader::GetNamedLocalVariable(___in ISymUnmanagedScope * pScope, ___in ICorDebugILFrame * pILFrame, ___in mdMethodDef methodToken, ___in ULONG localIndex, __out_ecount(paramNameLen) WCHAR* paramName, ___in ULONG paramNameLen, ICorDebugValue** ppValue) { @@ -1410,4 +1436,4 @@ HRESULT SymbolReader::ResolveSequencePoint(__in_z WCHAR* pFilename, ___in ULONG3 #endif // FEATURE_PAL return E_FAIL; -} +} \ No newline at end of file diff --git a/src/SOS/Strike/hostcoreclr.h b/src/SOS/Strike/hostcoreclr.h index f6c17d9c9..3e92486b4 100644 --- a/src/SOS/Strike/hostcoreclr.h +++ b/src/SOS/Strike/hostcoreclr.h @@ -73,6 +73,17 @@ extern HRESULT LoadNativeSymbols(bool runtimeOnly = false); extern void DisplaySymbolStore(); extern void DisableSymbolStore(); +extern HRESULT GetMetadataLocator( + LPCWSTR imagePath, + ULONG32 imageTimestamp, + ULONG32 imageSize, + GUID* mvid, + ULONG32 mdRva, + ULONG32 flags, + ULONG32 bufferSize, + BYTE* buffer, + ULONG32* dataSize); + class SymbolReader { private: diff --git a/src/SOS/Strike/strike.cpp b/src/SOS/Strike/strike.cpp index 4461be4ea..a9e31bb5f 100644 --- a/src/SOS/Strike/strike.cpp +++ b/src/SOS/Strike/strike.cpp @@ -197,7 +197,6 @@ HMODULE g_hInstance = NULL; #ifdef FEATURE_PAL -#define NOTHROW #define MINIDUMP_NOT_SUPPORTED() #else // !FEATURE_PAL @@ -210,8 +209,6 @@ HMODULE g_hInstance = NULL; return Status; \ } -#define NOTHROW (std::nothrow) - #include "safemath.h" DECLARE_API (MinidumpMode) diff --git a/src/SOS/Strike/util.cpp b/src/SOS/Strike/util.cpp index 6bc5f2325..8bc368dea 100644 --- a/src/SOS/Strike/util.cpp +++ b/src/SOS/Strike/util.cpp @@ -2469,8 +2469,7 @@ DWORD_PTR *ModuleFromName(__in_opt LPSTR mName, int *numModule) return NULL; } - WCHAR StringData[MAX_LONGPATH]; - char fileName[sizeof(StringData)/2]; + ArrayHolder fileName = new char[MAX_LONGPATH]; // Search all domains to find a module for (int n = 0; n < adsData.DomainCount+numSpecialDomains; n++) @@ -2550,13 +2549,14 @@ DWORD_PTR *ModuleFromName(__in_opt LPSTR mName, int *numModule) goto Failure; } - FileNameForModule ((DWORD_PTR)ModuleAddr, StringData); - int m; - for (m = 0; StringData[m] != L'\0'; m++) + if (mName != NULL) { - fileName[m] = (char)StringData[m]; + ArrayHolder moduleName = new WCHAR[MAX_LONGPATH]; + FileNameForModule((DWORD_PTR)ModuleAddr, moduleName); + + int bytesWritten = WideCharToMultiByte(CP_ACP, 0, moduleName, -1, fileName, MAX_LONGPATH, NULL, NULL); + _ASSERTE(bytesWritten > 0); } - fileName[m] = '\0'; if ((mName == NULL) || IsSameModuleName(fileName, mName) || @@ -4427,6 +4427,22 @@ public: { return E_UNEXPECTED; } +#ifdef FEATURE_PAL + if (g_sos != nullptr) + { + // LLDB synthesizes memory (returns 0's) for missing pages (in this case the missing metadata pages) + // in core dumps. This functions creates a list of the metadata regions and caches the metadata if + // available from the local or downloaded assembly. If the read would be in the metadata of a loaded + // assembly, the metadata from the this cache will be returned. + HRESULT hr = GetMetadataMemory(address, request, pBuffer); + if (SUCCEEDED(hr)) { + if (pcbRead != nullptr) { + *pcbRead = request; + } + return hr; + } + } +#endif return g_ExtData->ReadVirtual(address, pBuffer, request, (PULONG) pcbRead); } @@ -6203,18 +6219,59 @@ struct MemoryRegion private: uint64_t m_startAddress; uint64_t m_endAddress; + CLRDATA_ADDRESS m_peFile; + BYTE* m_metadataMemory; + volatile LONG m_busy; -public: - MemoryRegion(uint64_t start, uint64_t end) : - m_startAddress(start), - m_endAddress(end) + HRESULT CacheMetadata() { + if (m_metadataMemory == nullptr) + { + HRESULT hr; + CLRDATA_ADDRESS baseAddress; + if (FAILED(hr = g_sos->GetPEFileBase(m_peFile, &baseAddress))) { + return hr; + } + ArrayHolder imagePath = new WCHAR[MAX_LONGPATH]; + if (FAILED(hr = g_sos->GetPEFileName(m_peFile, MAX_LONGPATH, imagePath.GetPtr(), NULL))) { + return hr; + } + IMAGE_DOS_HEADER DosHeader; + if (FAILED(hr = g_ExtData->ReadVirtual(baseAddress, &DosHeader, sizeof(DosHeader), NULL))) { + return hr; + } + IMAGE_NT_HEADERS Header; + if (FAILED(hr = g_ExtData->ReadVirtual(baseAddress + DosHeader.e_lfanew, &Header, sizeof(Header), NULL))) { + return hr; + } + // If there is no COMHeader, this can not be managed code. + if (Header.OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_COMHEADER].VirtualAddress == 0) { + return E_ACCESSDENIED; + } + ULONG32 imageSize = Header.OptionalHeader.SizeOfImage; + ULONG32 timeStamp = Header.FileHeader.TimeDateStamp; + ULONG32 bufferSize = (ULONG32)Size(); + + ArrayHolder buffer = new NOTHROW BYTE[bufferSize]; + if (buffer == nullptr) { + return E_OUTOFMEMORY; + } + ULONG32 actualSize = 0; + if (FAILED(hr = GetMetadataLocator(imagePath, timeStamp, imageSize, nullptr, 0, 0, bufferSize, buffer, &actualSize))) { + return hr; + } + m_metadataMemory = buffer.Detach(); + } + return S_OK; } - // copy constructor - MemoryRegion(const MemoryRegion& region) : - m_startAddress(region.m_startAddress), - m_endAddress(region.m_endAddress) +public: + MemoryRegion(uint64_t start, uint64_t end, CLRDATA_ADDRESS peFile) : + m_startAddress(start), + m_endAddress(end), + m_peFile(peFile), + m_metadataMemory(nullptr), + m_busy(0) { } @@ -6222,6 +6279,8 @@ public: uint64_t EndAddress() const { return m_endAddress; } uint64_t Size() const { return m_endAddress - m_startAddress; } + CLRDATA_ADDRESS const PEFile() { return m_peFile; } + bool operator<(const MemoryRegion& rhs) const { return (m_startAddress < rhs.m_startAddress) && (m_endAddress <= rhs.m_startAddress); @@ -6232,6 +6291,47 @@ public: { return (m_startAddress <= rhs.m_startAddress) && (m_endAddress >= rhs.m_endAddress); } + + HRESULT ReadMetadata(CLRDATA_ADDRESS address, ULONG32 bufferSize, BYTE* buffer) + { + _ASSERTE((m_startAddress <= address) && (m_endAddress >= (address + bufferSize))); + + HRESULT hr = E_ACCESSDENIED; + + // Skip in-memory and dynamic modules or if CacheMetadata failed + if (m_peFile != 0) + { + if (InterlockedIncrement(&m_busy) == 1) + { + // Attempt to get the assembly metadata from local file or by downloading from a symbol server + hr = CacheMetadata(); + if (FAILED(hr)) { + // If we can get the metadata from the assembly, mark this region to always fail. + m_peFile = 0; + } + } + InterlockedDecrement(&m_busy); + } + + if (FAILED(hr)) { + return hr; + } + + // Read the memory from the cached metadata blob + _ASSERTE(m_metadataMemory != nullptr); + uint64_t offset = address - m_startAddress; + memcpy(buffer, m_metadataMemory + offset, bufferSize); + return S_OK; + } + + void Dispose() + { + if (m_metadataMemory != nullptr) + { + delete[] m_metadataMemory; + m_metadataMemory = nullptr; + } + } }; std::set g_metadataRegions; @@ -6239,21 +6339,13 @@ bool g_metadataRegionsPopulated = false; void FlushMetadataRegions() { + for (const MemoryRegion& region : g_metadataRegions) + { + const_cast(region).Dispose(); + } g_metadataRegionsPopulated = false; } -//------------------------------------------------------------------------------- -// Lifted from "..\md\inc\mdfileformat.h" -#define STORAGE_MAGIC_SIG 0x424A5342 // BSJB -struct STORAGESIGNATURE -{ - ULONG lSignature; // "Magic" signature. - USHORT iMajorVer; // Major file version. - USHORT iMinorVer; // Minor file version. - ULONG iExtraData; // Offset to next structure of information - ULONG iVersionString; // Length of version string -}; - void PopulateMetadataRegions() { g_metadataRegions.clear(); @@ -6272,17 +6364,8 @@ void PopulateMetadataRegions() { if (moduleData.metadataStart != 0) { - MemoryRegion region(moduleData.metadataStart, moduleData.metadataStart + moduleData.metadataSize); + MemoryRegion region(moduleData.metadataStart, moduleData.metadataStart + moduleData.metadataSize, moduleData.File); g_metadataRegions.insert(region); -#ifdef METADATA_REGION_LOGGING - ArrayHolder name = new WCHAR[MAX_LONGPATH]; - name[0] = '\0'; - if (moduleData.File != 0) - { - g_sos->GetPEFileName(moduleData.File, MAX_LONGPATH, name.GetPtr(), NULL); - } - ExtOut("%c%016x %016x %016x %S\n", add ? '*' : ' ', moduleData.metadataStart, moduleData.metadataStart + moduleData.metadataSize, moduleData.metadataSize, name.GetPtr()); -#endif // METADATA_REGION_LOGGING } } } @@ -6290,20 +6373,21 @@ void PopulateMetadataRegions() } } -bool IsMetadataMemory(CLRDATA_ADDRESS address, ULONG32 size) +HRESULT GetMetadataMemory(CLRDATA_ADDRESS address, ULONG32 bufferSize, BYTE* buffer) { + // Populate the metadata memory region map if (!g_metadataRegionsPopulated) { g_metadataRegionsPopulated = true; PopulateMetadataRegions(); } - MemoryRegion region(address, address + size); + // Check if the memory address is in a metadata memory region + MemoryRegion region(address, address + bufferSize, 0); const auto& found = g_metadataRegions.find(region); - if (found != g_metadataRegions.end()) - { - return found->Contains(region); + if (found != g_metadataRegions.end() && found->Contains(region)) { + return const_cast(*found).ReadMetadata(address, bufferSize, buffer); } - return false; + return E_ACCESSDENIED; } #endif // FEATURE_PAL \ No newline at end of file diff --git a/src/SOS/Strike/util.h b/src/SOS/Strike/util.h index 15f7dd939..14a205de7 100644 --- a/src/SOS/Strike/util.h +++ b/src/SOS/Strike/util.h @@ -41,6 +41,12 @@ inline void RestoreSOToleranceState() {} typedef LPCSTR LPCUTF8; typedef LPSTR LPUTF8; +#ifdef FEATURE_PAL +#define NOTHROW +#else +#define NOTHROW (std::nothrow) +#endif + DECLARE_HANDLE(OBJECTHANDLE); #if defined(_TARGET_WIN64_) @@ -1853,7 +1859,7 @@ BOOL TryGetMethodDescriptorForDelegate(CLRDATA_ADDRESS delegateAddr, CLRDATA_ADD #ifdef FEATURE_PAL void FlushMetadataRegions(); -bool IsMetadataMemory(CLRDATA_ADDRESS address, ULONG32 size); +HRESULT GetMetadataMemory(CLRDATA_ADDRESS address, ULONG32 bufferSize, BYTE* buffer); #endif /* Returns a list of all modules in the process.