More code review feedback chantges

dotnet · May 9, 2023 · 4a89e23 · 4a89e23
1 parent b1f2d13
commit 4a89e23
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 28 deletions.
diff --git a/src/coreclr/debug/createdump/crashinfo.cpp b/src/coreclr/debug/createdump/crashinfo.cpp
@@ -149,7 +149,7 @@ CrashInfo::LogMessage(
 // Gather all the necessary crash dump info.
 //
 bool
-CrashInfo::GatherCrashInfo(DumpType* dumpType)
+CrashInfo::GatherCrashInfo(DumpType dumpType)
 {
     // Get the info about the threads (registers, etc.)
     for (ThreadInfo* thread : m_threads)
@@ -182,7 +182,7 @@ CrashInfo::GatherCrashInfo(DumpType* dumpType)
     }
 #endif
     // Load and initialize DAC interfaces
-    if (!InitializeDAC())
+    if (!InitializeDAC(dumpType))
     {
         return false;
     }
@@ -208,13 +208,8 @@ CrashInfo::GatherCrashInfo(DumpType* dumpType)
             region.Trace();
         }
     }
-    // If the DAC module present side-by-side (the default for single-file and native AOT apps), fallback to full dump.
-    if (m_pClrDataProcess == nullptr)
-    {
-        *dumpType = DumpType::Full;
-    }
     // If full memory dump, include everything regardless of permissions
-    if (*dumpType == DumpType::Full)
+    if (dumpType == DumpType::Full)
     {
         for (const MemoryRegion& region : m_moduleMappings)
         {
@@ -233,7 +228,7 @@ CrashInfo::GatherCrashInfo(DumpType* dumpType)
     {
         // Add all the heap read/write memory regions (m_otherMappings contains the heaps). On Alpine
         // the heap regions are marked RWX instead of just RW.
-        if (*dumpType == DumpType::Heap)
+        if (dumpType == DumpType::Heap)
         {
             for (const MemoryRegion& region : m_otherMappings)
             {
@@ -283,9 +278,9 @@ GetHResultString(HRESULT hr)
 // Enumerate all the memory regions using the DAC memory region support given a minidump type
 //
 bool
-CrashInfo::InitializeDAC()
+CrashInfo::InitializeDAC(DumpType dumpType)
 {
-    // Don't attempt to load the DAC if createdump is statically linked into the runtime
+    // Don't attempt to load the DAC if native AOT app (there is no DAC)
     if (m_appModel == AppModelType::NativeAOT)
     {
         return true;
@@ -312,15 +307,7 @@ CrashInfo::InitializeDAC()
     m_dacModule = dlopen(dacPath.c_str(), RTLD_LAZY);
     if (m_dacModule == nullptr)
     {
-        // Don't fail for single-file apps when the DAC can't be found. Will fall back to full dump.
-        if (m_appModel == AppModelType::SingleFile)
-        {
-            result = true;
-        }
-        else
-        {
-            printf_error("InitializeDAC: dlopen(%s) FAILED %s\n", dacPath.c_str(), dlerror());
-        }
+        printf_error("InitializeDAC: dlopen(%s) FAILED %s\n", dacPath.c_str(), dlerror());
         goto exit;
     }
     pfnDllMain = (PFN_DLLMAIN)dlsym(m_dacModule, "DllMain");

diff --git a/src/coreclr/debug/createdump/crashinfo.h b/src/coreclr/debug/createdump/crashinfo.h
@@ -93,7 +93,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi
     bool Initialize();
     void CleanupAndResumeProcess();
     bool EnumerateAndSuspendThreads();
-    bool GatherCrashInfo(DumpType* dumpType);
+    bool GatherCrashInfo(DumpType dumpType);
     void CombineMemoryRegions();
     bool EnumerateMemoryRegionsWithDAC(DumpType dumpType);
     bool ReadMemory(void* address, void* buffer, size_t size);                          // read memory and add to dump
@@ -154,7 +154,7 @@ class CrashInfo : public ICLRDataEnumMemoryRegionsCallback, public ICLRDataLoggi
     void VisitProgramHeader(uint64_t loadbias, uint64_t baseAddress, ElfW(Phdr)* phdr);
     bool EnumerateMemoryRegions();
 #endif
-    bool InitializeDAC();
+    bool InitializeDAC(DumpType dumpType);
     bool EnumerateManagedModules();
     bool UnwindAllThreads();
     void AddOrReplaceModuleMapping(CLRDATA_ADDRESS baseAddress, ULONG64 size, const std::string& pszName);

diff --git a/src/coreclr/debug/createdump/createdump.h b/src/coreclr/debug/createdump/createdump.h
@@ -90,15 +90,15 @@ typedef int T_CONTEXT;
 #include <array>
 #include <string>
 
-enum DumpType
+enum class DumpType
 {
     Mini,
     Heap,
     Triage,
     Full
 };
 
-enum AppModelType
+enum class AppModelType
 {
     Normal,
     SingleFile,

diff --git a/src/coreclr/debug/createdump/createdumpunix.cpp b/src/coreclr/debug/createdump/createdumpunix.cpp
@@ -14,7 +14,6 @@ bool
 CreateDump(const CreateDumpOptions& options)
 {
     ReleaseHolder<CrashInfo> crashInfo = new CrashInfo(options);
-    DumpType dumpType = options.DumpType;
     DumpWriter dumpWriter(*crashInfo);
     std::string dumpPath;
     bool result = false;
@@ -43,7 +42,7 @@ CreateDump(const CreateDumpOptions& options)
         goto exit;
     }
     // Gather all the info about the process, threads (registers, etc.) and memory regions
-    if (!crashInfo->GatherCrashInfo(&dumpType))
+    if (!crashInfo->GatherCrashInfo(options.DumpType))
     {
         goto exit;
     }
@@ -61,14 +60,14 @@ CreateDump(const CreateDumpOptions& options)
     if (options.CreateDump)
     {
         // Gather all the useful memory regions from the DAC
-        if (!crashInfo->EnumerateMemoryRegionsWithDAC(dumpType))
+        if (!crashInfo->EnumerateMemoryRegionsWithDAC(options.DumpType))
         {
             goto exit;
         }
         // Join all adjacent memory regions
         crashInfo->CombineMemoryRegions();
 
-        printf_status("Writing %s to file %s\n", GetDumpTypeString(dumpType), dumpPath.c_str());
+        printf_status("Writing %s to file %s\n", GetDumpTypeString(options.DumpType), dumpPath.c_str());
 
         // Write the actual dump file
         if (!dumpWriter.OpenDump(dumpPath.c_str()))