From 3fb9d8d2fe094f09a914d31a551e14fe8a1f2215 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Wed, 3 May 2017 17:17:22 -0700 Subject: [PATCH] Createdump fixes and cleanup. (#11368) Fixed "with heap" option to actually include all the heaps. The way /proc/$pid/maps was parsed missed most of the RW data/heap regions. Added -u/--full full core dump options. Removed the useless m_memStatus variable in enummem.cpp. --- Documentation/botr/xplat-minidump-generation.md | 2 +- src/ToolBox/SOS/Strike/sosdocsunix.txt | 1 + src/ToolBox/SOS/Strike/strike.cpp | 8 +- src/debug/createdump/crashinfo.cpp | 226 +++++++++++++++--------- src/debug/createdump/crashinfo.h | 4 +- src/debug/createdump/createdump.cpp | 4 + src/debug/createdump/datatarget.cpp | 1 - src/debug/createdump/main.cpp | 5 + src/debug/createdump/memoryregion.h | 39 +--- src/debug/daccess/daccess.cpp | 2 - src/debug/daccess/dacimpl.h | 1 - src/debug/daccess/enummem.cpp | 8 +- 12 files changed, 175 insertions(+), 126 deletions(-) diff --git a/Documentation/botr/xplat-minidump-generation.md b/Documentation/botr/xplat-minidump-generation.md index 78660b6..03cd517 100644 --- a/Documentation/botr/xplat-minidump-generation.md +++ b/Documentation/botr/xplat-minidump-generation.md @@ -62,6 +62,7 @@ Environment variables supported: -n, --normal - create minidump (default). -h, --withheap - create minidump with heap. -t, --triage - create triage minidump. + -u, --full - create full core dump. -d, --diag - enable diagnostic messages. # Testing # @@ -73,7 +74,6 @@ The test plan is to modify the SOS tests in the (still) private debuggertests re - Do we need a full memory dump option? It would not use the _DAC_ to get the memory regions but all the readable memory from the shared module list. Do we include the shared modules' code? - May need more than just the pid for decorating dump names for docker containers because I think the _pid_ is always 1. - Do we need all the memory mappings from `/proc/$pid/maps` in the PT\_LOAD sections even though the memory is not actually in the dump? They have a file offset/size of 0. Full dumps generated by the system or _gdb_ do have these un-backed regions. -- Don't know how to get the proper size/range of the non-main thread stacks. Currently uses 4 pages around the stack pointer. The main thread has a memory region in `/proc/$pid/maps`. - There is no way to get the signal number, etc. that causes the abort from the _createdump_ utility using _ptrace_ or a /proc file. It would have to be passed from CoreCLR on the command line. - Do we need the "dynamic" sections of each shared module in the core dump? It is part of the "link_map" entry enumerated when gathering the _DSO_ information. - There may be more versioning and/or build id information needed to be added to the dump. diff --git a/src/ToolBox/SOS/Strike/sosdocsunix.txt b/src/ToolBox/SOS/Strike/sosdocsunix.txt index 517227c..e9fd59c 100644 --- a/src/ToolBox/SOS/Strike/sosdocsunix.txt +++ b/src/ToolBox/SOS/Strike/sosdocsunix.txt @@ -623,6 +623,7 @@ createdump [options] [dumpFileName] -n - create minidump. -h - create minidump with heap (default). -t - create triage minidump. +-f - create full core dump (everything). -d - enable diagnostic messages. Creates a platform (ELF core on Linux, etc.) minidump. The pid can be placed in the dump diff --git a/src/ToolBox/SOS/Strike/strike.cpp b/src/ToolBox/SOS/Strike/strike.cpp index 0d5d8c3..1fff17f 100644 --- a/src/ToolBox/SOS/Strike/strike.cpp +++ b/src/ToolBox/SOS/Strike/strike.cpp @@ -14385,6 +14385,7 @@ DECLARE_API(CreateDump) BOOL normal = FALSE; BOOL withHeap = FALSE; BOOL triage = FALSE; + BOOL full = FALSE; BOOL diag = FALSE; size_t nArg = 0; @@ -14393,6 +14394,7 @@ DECLARE_API(CreateDump) {"-n", &normal, COBOOL, FALSE}, {"-h", &withHeap, COBOOL, FALSE}, {"-t", &triage, COBOOL, FALSE}, + {"-f", &full, COBOOL, FALSE}, {"-d", &diag, COBOOL, FALSE}, }; CMDValue arg[] = @@ -14407,7 +14409,11 @@ DECLARE_API(CreateDump) ULONG pid = 0; g_ExtSystem->GetCurrentProcessId(&pid); - if (withHeap) + if (full) + { + minidumpType = MiniDumpWithFullMemory; + } + else if (withHeap) { minidumpType = MiniDumpWithPrivateReadWriteMemory; } diff --git a/src/debug/createdump/crashinfo.cpp b/src/debug/createdump/crashinfo.cpp index cae8857..7322fab 100644 --- a/src/debug/createdump/crashinfo.cpp +++ b/src/debug/createdump/crashinfo.cpp @@ -155,33 +155,68 @@ CrashInfo::GatherCrashInfo(const char* programPath, MINIDUMP_TYPE minidumpType) { return false; } - // Get shared module debug info - if (!GetDSOInfo()) - { - return false; - } // Gather all the module memory mappings (from /dev/$pid/maps) if (!EnumerateModuleMappings()) { return false; } - // Gather all the useful memory regions from the DAC - if (!EnumerateMemoryRegionsWithDAC(programPath, minidumpType)) + // Get shared module debug info + if (!GetDSOInfo()) { return false; } - // Add the thread's stack and some code memory to core - for (ThreadInfo* thread : m_threads) + // If full memory dump, include everything regardless of permissions + if (minidumpType & MiniDumpWithFullMemory) + { + for (const MemoryRegion& region : m_moduleMappings) + { + if (ValidRegion(region)) + { + InsertMemoryRegion(region); + } + } + for (const MemoryRegion& region : m_otherMappings) + { + if (ValidRegion(region)) + { + InsertMemoryRegion(region); + } + } + } + else { - uint64_t start; - size_t size; + // Add all the heap (read/write) memory regions but not the modules' r/w data segments + if (minidumpType & MiniDumpWithPrivateReadWriteMemory) + { + for (const MemoryRegion& region : m_otherMappings) + { + if (region.Permissions() == (PF_R | PF_W)) + { + if (ValidRegion(region)) + { + InsertMemoryRegion(region); + } + } + } + } + // Gather all the useful memory regions from the DAC + if (!EnumerateMemoryRegionsWithDAC(programPath, minidumpType)) + { + return false; + } + // Add the thread's stack and some code memory to core + for (ThreadInfo* thread : m_threads) + { + uint64_t start; + size_t size; - // Add the thread's stack and some of the code - thread->GetThreadStack(*this, &start, &size); - InsertMemoryRegion(start, size); + // Add the thread's stack and some of the code + thread->GetThreadStack(*this, &start, &size); + InsertMemoryRegion(start, size); - thread->GetThreadCode(&start, &size); - InsertMemoryRegion(start, size); + thread->GetThreadCode(&start, &size); + InsertMemoryRegion(start, size); + } } // Join all adjacent memory regions CombineMemoryRegions(); @@ -240,7 +275,7 @@ CrashInfo::EnumerateModuleMappings() // Here we read /proc//maps file in order to parse it and figure out what it says // about a library we are looking for. This file looks something like this: // - // [address] [perms] [offset] [dev] [inode] [pathname] - HEADER is not preset in an actual file + // [address] [perms] [offset] [dev] [inode] [pathname] - HEADER is not preset in an actual file // // 35b1800000-35b1820000 r-xp 00000000 08:02 135522 /usr/lib64/ld-2.15.so // 35b1a1f000-35b1a20000 r--p 0001f000 08:02 135522 /usr/lib64/ld-2.15.so @@ -282,8 +317,8 @@ CrashInfo::EnumerateModuleMappings() char* permissions = nullptr; char* moduleName = nullptr; - int c = 0; - if ((c = sscanf(line, "%lx-%lx %m[-rwxsp] %lx %*[:0-9a-f] %*d %ms\n", &start, &end, &permissions, &offset, &moduleName)) == 5) + int c = sscanf(line, "%lx-%lx %m[-rwxsp] %lx %*[:0-9a-f] %*d %ms\n", &start, &end, &permissions, &offset, &moduleName); + if (c == 4 || c == 5) { if (linuxGateAddress != nullptr && reinterpret_cast(start) == linuxGateAddress) { @@ -335,60 +370,6 @@ CrashInfo::EnumerateModuleMappings() } bool -CrashInfo::EnumerateMemoryRegionsWithDAC(const char* programPath, MINIDUMP_TYPE minidumpType) -{ - PFN_CLRDataCreateInstance pfnCLRDataCreateInstance = nullptr; - ICLRDataEnumMemoryRegions *clrDataEnumRegions = nullptr; - HMODULE hdac = nullptr; - HRESULT hr = S_OK; - bool result = false; - - // We assume that the DAC is in the same location as this createdump exe - std::string dacPath; - dacPath.append(programPath); - dacPath.append("/"); - dacPath.append(MAKEDLLNAME_A("mscordaccore")); - - // Load and initialize the DAC - hdac = LoadLibraryA(dacPath.c_str()); - if (hdac == nullptr) - { - fprintf(stderr, "LoadLibraryA(%s) FAILED %d\n", dacPath.c_str(), GetLastError()); - goto exit; - } - pfnCLRDataCreateInstance = (PFN_CLRDataCreateInstance)GetProcAddress(hdac, "CLRDataCreateInstance"); - if (pfnCLRDataCreateInstance == nullptr) - { - fprintf(stderr, "GetProcAddress(CLRDataCreateInstance) FAILED %d\n", GetLastError()); - goto exit; - } - hr = pfnCLRDataCreateInstance(__uuidof(ICLRDataEnumMemoryRegions), m_dataTarget, (void**)&clrDataEnumRegions); - if (FAILED(hr)) - { - fprintf(stderr, "CLRDataCreateInstance(ICLRDataEnumMemoryRegions) FAILED %08x\n", hr); - goto exit; - } - // Calls CrashInfo::EnumMemoryRegion for each memory region found by the DAC - hr = clrDataEnumRegions->EnumMemoryRegions(this, minidumpType, CLRDATA_ENUM_MEM_DEFAULT); - if (FAILED(hr)) - { - fprintf(stderr, "EnumMemoryRegions FAILED %08x\n", hr); - goto exit; - } - result = true; -exit: - if (clrDataEnumRegions != nullptr) - { - clrDataEnumRegions->Release(); - } - if (hdac != nullptr) - { - FreeLibrary(hdac); - } - return result; -} - -bool CrashInfo::GetDSOInfo() { Phdr* phdrAddr = reinterpret_cast(m_auxvValues[AT_PHDR]); @@ -471,6 +452,60 @@ CrashInfo::GetDSOInfo() return true; } +bool +CrashInfo::EnumerateMemoryRegionsWithDAC(const char* programPath, MINIDUMP_TYPE minidumpType) +{ + PFN_CLRDataCreateInstance pfnCLRDataCreateInstance = nullptr; + ICLRDataEnumMemoryRegions *clrDataEnumRegions = nullptr; + HMODULE hdac = nullptr; + HRESULT hr = S_OK; + bool result = false; + + // We assume that the DAC is in the same location as this createdump exe + std::string dacPath; + dacPath.append(programPath); + dacPath.append("/"); + dacPath.append(MAKEDLLNAME_A("mscordaccore")); + + // Load and initialize the DAC + hdac = LoadLibraryA(dacPath.c_str()); + if (hdac == nullptr) + { + fprintf(stderr, "LoadLibraryA(%s) FAILED %d\n", dacPath.c_str(), GetLastError()); + goto exit; + } + pfnCLRDataCreateInstance = (PFN_CLRDataCreateInstance)GetProcAddress(hdac, "CLRDataCreateInstance"); + if (pfnCLRDataCreateInstance == nullptr) + { + fprintf(stderr, "GetProcAddress(CLRDataCreateInstance) FAILED %d\n", GetLastError()); + goto exit; + } + hr = pfnCLRDataCreateInstance(__uuidof(ICLRDataEnumMemoryRegions), m_dataTarget, (void**)&clrDataEnumRegions); + if (FAILED(hr)) + { + fprintf(stderr, "CLRDataCreateInstance(ICLRDataEnumMemoryRegions) FAILED %08x\n", hr); + goto exit; + } + // Calls CrashInfo::EnumMemoryRegion for each memory region found by the DAC + hr = clrDataEnumRegions->EnumMemoryRegions(this, minidumpType, CLRDATA_ENUM_MEM_DEFAULT); + if (FAILED(hr)) + { + fprintf(stderr, "EnumMemoryRegions FAILED %08x\n", hr); + goto exit; + } + result = true; +exit: + if (clrDataEnumRegions != nullptr) + { + clrDataEnumRegions->Release(); + } + if (hdac != nullptr) + { + FreeLibrary(hdac); + } + return result; +} + // // ReadMemory from target and add to memory regions list // @@ -501,38 +536,67 @@ CrashInfo::InsertMemoryRegion(uint64_t address, size_t size) uint64_t end = ((address + size) + (PAGE_SIZE - 1)) & PAGE_MASK; assert(end > 0); - MemoryRegion memoryRegionFull(start, end); + MemoryRegion region(start, end); + InsertMemoryRegion(region); +} +// +// Add a memory region to the list +// +void +CrashInfo::InsertMemoryRegion(const MemoryRegion& region) +{ // First check if the full memory region can be added without conflicts - const auto& found = m_memoryRegions.find(memoryRegionFull); + const auto& found = m_memoryRegions.find(region); if (found == m_memoryRegions.end()) { // Add full memory region - m_memoryRegions.insert(memoryRegionFull); + m_memoryRegions.insert(region); } else { // The memory region is not wholely contained in region found - if (!found->Contains(memoryRegionFull)) + if (!found->Contains(region)) { + uint64_t start = region.StartAddress(); + // The region overlaps/conflicts with one already in the set so // add one page at a time to avoid the overlapping pages. - uint64_t numberPages = (end - start) >> PAGE_SHIFT; + uint64_t numberPages = region.Size() >> PAGE_SHIFT; for (int p = 0; p < numberPages; p++, start += PAGE_SIZE) { - MemoryRegion memoryRegion(start, start + PAGE_SIZE); + MemoryRegion memoryRegionPage(start, start + PAGE_SIZE); - const auto& found = m_memoryRegions.find(memoryRegion); + const auto& found = m_memoryRegions.find(memoryRegionPage); if (found == m_memoryRegions.end()) { - m_memoryRegions.insert(memoryRegion); + m_memoryRegions.insert(memoryRegionPage); } } } } } +bool +CrashInfo::ValidRegion(const MemoryRegion& region) +{ + uint64_t start = region.StartAddress(); + uint64_t numberPages = region.Size() >> PAGE_SHIFT; + + for (int p = 0; p < numberPages; p++, start += PAGE_SIZE) + { + BYTE buffer[1]; + uint32_t read; + + if (FAILED(m_dataTarget->ReadVirtual(start, buffer, 1, &read))) + { + return false; + } + } + return true; +} + // // Combine any adjacent memory regions into one // diff --git a/src/debug/createdump/crashinfo.h b/src/debug/createdump/crashinfo.h index 40d7f5d..914a88e 100644 --- a/src/debug/createdump/crashinfo.h +++ b/src/debug/createdump/crashinfo.h @@ -65,9 +65,11 @@ public: private: bool GetAuxvEntries(); bool EnumerateModuleMappings(); - bool EnumerateMemoryRegionsWithDAC(const char* programPath, MINIDUMP_TYPE minidumpType); bool GetDSOInfo(); + bool EnumerateMemoryRegionsWithDAC(const char* programPath, MINIDUMP_TYPE minidumpType); bool ReadMemory(void* address, void* buffer, size_t size); void InsertMemoryRegion(uint64_t address, size_t size); + void InsertMemoryRegion(const MemoryRegion& region); + bool ValidRegion(const MemoryRegion& region); void CombineMemoryRegions(); }; diff --git a/src/debug/createdump/createdump.cpp b/src/debug/createdump/createdump.cpp index 0a95e53..d121291 100644 --- a/src/debug/createdump/createdump.cpp +++ b/src/debug/createdump/createdump.cpp @@ -29,6 +29,10 @@ CreateDumpCommon(const char* programPath, const char* dumpPathTemplate, MINIDUMP dumpType = "triage minidump"; break; + case MiniDumpWithFullMemory: + dumpType = "full dump"; + break; + default: break; } diff --git a/src/debug/createdump/datatarget.cpp b/src/debug/createdump/datatarget.cpp index 38505e2..9609fa3 100644 --- a/src/debug/createdump/datatarget.cpp +++ b/src/debug/createdump/datatarget.cpp @@ -159,7 +159,6 @@ DumpDataTarget::ReadVirtual( size_t read = pread64(m_fd, buffer, size, (off64_t)address); if (read == -1) { - fprintf(stderr, "ReadVirtual FAILED %016lx %08x\n", address, size); *done = 0; return E_FAIL; } diff --git a/src/debug/createdump/main.cpp b/src/debug/createdump/main.cpp index 0338277..cb4d3c6 100644 --- a/src/debug/createdump/main.cpp +++ b/src/debug/createdump/main.cpp @@ -9,6 +9,7 @@ const char* g_help = "createdump [options] pid\n" "-n, --normal - create minidump.\n" "-h, --withheap - create minidump with heap (default).\n" "-t, --triage - create triage minidump.\n" +"-u, --full - create full core dump.\n" "-d, --diag - enable diagnostic messages.\n"; bool CreateDumpCommon(const char* programPath, const char* dumpPathTemplate, MINIDUMP_TYPE minidumpType, CrashInfo* crashInfo); @@ -60,6 +61,10 @@ int __cdecl main(const int argc, const char* argv[]) { minidumpType = MiniDumpFilterTriage; } + else if ((strcmp(*argv, "-u") == 0) || (strcmp(*argv, "--full") == 0)) + { + minidumpType = MiniDumpWithFullMemory; + } else if ((strcmp(*argv, "-d") == 0) || (strcmp(*argv, "--diag") == 0)) { g_diagnostics = true; diff --git a/src/debug/createdump/memoryregion.h b/src/debug/createdump/memoryregion.h index 16c4d1c..1332ab1 100644 --- a/src/debug/createdump/memoryregion.h +++ b/src/debug/createdump/memoryregion.h @@ -36,35 +36,12 @@ public: assert((end & ~PAGE_MASK) == 0); } - const uint32_t Permissions() const - { - return m_permissions; - } - - const uint64_t StartAddress() const - { - return m_startAddress; - } - - const uint64_t EndAddress() const - { - return m_endAddress; - } - - const uint64_t Size() const - { - return m_endAddress - m_startAddress; - } - - const uint64_t Offset() const - { - return m_offset; - } - - const char* FileName() const - { - return m_fileName; - } + const uint32_t Permissions() const { return m_permissions; } + const uint64_t StartAddress() const { return m_startAddress; } + const uint64_t EndAddress() const { return m_endAddress; } + const uint64_t Size() const { return m_endAddress - m_startAddress; } + const uint64_t Offset() const { return m_offset; } + const char* FileName() const { return m_fileName; } bool operator<(const MemoryRegion& rhs) const { @@ -88,10 +65,10 @@ public: void Print() const { if (m_fileName != nullptr) { - TRACE("%016lx - %016lx (%04ld) %016lx %x %s\n", m_startAddress, m_endAddress, (Size() >> PAGE_SHIFT), m_offset, m_permissions, m_fileName); + TRACE("%016lx - %016lx (%06ld) %016lx %x %s\n", m_startAddress, m_endAddress, (Size() >> PAGE_SHIFT), m_offset, m_permissions, m_fileName); } else { - TRACE("%016lx - %016lx (%04ld) %02x\n", m_startAddress, m_endAddress, (Size() >> PAGE_SHIFT), m_permissions); + TRACE("%016lx - %016lx (%06ld) %x\n", m_startAddress, m_endAddress, (Size() >> PAGE_SHIFT), m_permissions); } } }; diff --git a/src/debug/daccess/daccess.cpp b/src/debug/daccess/daccess.cpp index dcf6314..14ce251 100644 --- a/src/debug/daccess/daccess.cpp +++ b/src/debug/daccess/daccess.cpp @@ -6305,8 +6305,6 @@ bool ClrDataAccess::ReportMem(TADDR addr, TSIZE_T size, bool fExpectSuccess /*= status = m_enumMemCb->EnumMemoryRegion(TO_CDADDR(addr), enumSize); if (status != S_OK) { - m_memStatus = status; - // If dump generation was cancelled, allow us to throw upstack so we'll actually quit. if ((fExpectSuccess) && (status != COR_E_OPERATIONCANCELED)) return false; diff --git a/src/debug/daccess/dacimpl.h b/src/debug/daccess/dacimpl.h index 635be80..cd133eb 100644 --- a/src/debug/daccess/dacimpl.h +++ b/src/debug/daccess/dacimpl.h @@ -1432,7 +1432,6 @@ private: ICLRMetadataLocator * m_legacyMetaDataLocator; LONG m_refs; - HRESULT m_memStatus; MDImportsCache m_mdImports; ICLRDataEnumMemoryRegionsCallback* m_enumMemCb; ICLRDataEnumMemoryRegionsCallback2* m_updateMemCb; diff --git a/src/debug/daccess/enummem.cpp b/src/debug/daccess/enummem.cpp index 9305bba..ebe0fc8 100644 --- a/src/debug/daccess/enummem.cpp +++ b/src/debug/daccess/enummem.cpp @@ -374,7 +374,6 @@ HRESULT ClrDataAccess::EnumMemoryRegionsWorkerHeap(IN CLRDataEnumMemoryFlags fla // now dump the memory get dragged in by using DAC API implicitly. m_dumpStats.m_cbImplicity = m_instances.DumpAllInstances(m_enumMemCb); - status = m_memStatus; // Do not let any remaining implicitly enumerated memory leak out. Flush(); @@ -394,7 +393,7 @@ HRESULT ClrDataAccess::DumpManagedObject(CLRDataEnumMemoryFlags flags, OBJECTREF { SUPPORTS_DAC; - HRESULT status = S_OK; + HRESULT status = S_OK; if (objRef == NULL) { @@ -1616,7 +1615,6 @@ HRESULT ClrDataAccess::EnumMemoryRegionsWorkerSkinny(IN CLRDataEnumMemoryFlags f // now dump the memory get dragged in by using DAC API implicitly. m_dumpStats.m_cbImplicity = m_instances.DumpAllInstances(m_enumMemCb); - status = m_memStatus; // Do not let any remaining implicitly enumerated memory leak out. Flush(); @@ -1667,7 +1665,6 @@ HRESULT ClrDataAccess::EnumMemoryRegionsWorkerMicroTriage(IN CLRDataEnumMemoryFl // now dump the memory get dragged in by using DAC API implicitly. m_dumpStats.m_cbImplicity = m_instances.DumpAllInstances(m_enumMemCb); - status = m_memStatus; // Do not let any remaining implicitly enumerated memory leak out. Flush(); @@ -1811,8 +1808,6 @@ HRESULT ClrDataAccess::EnumMemoryRegionsWorkerCustom() status = E_INVALIDARG; } - status = m_memStatus; - return S_OK; } @@ -1935,7 +1930,6 @@ ClrDataAccess::EnumMemoryRegions(IN ICLRDataEnumMemoryRegionsCallback* callback, // We should not be trying to enumerate while we have an enumeration outstanding _ASSERTE(m_enumMemCb==NULL); - m_memStatus = S_OK; m_enumMemCb = callback; // QI for ICLRDataEnumMemoryRegionsCallback2 will succeed only for Win8+. -- 2.7.4