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.

9 files changed, 166 insertions, 124 deletions

diff --git a/src/debug/createdump/crashinfo.cpp b/src/debug/createdump/crashinfo.cpp
index cae8857d65..7322fabd19 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/<pid>/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<void*>(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<Phdr*>(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 40d7f5da45..914a88e8dd 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 0a95e535aa..d121291e38 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 38505e2d45..9609fa30a7 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 03382779a5..cb4d3c6fce 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 16c4d1c693..1332ab1416 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 dcf6314549..14ce251bfc 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 635be80232..cd133ebf1e 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 9305bba488..ebe0fc87bb 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+.