Make changes according to feedback

- If we are saving core style with eSaveCoreDirtyOnly and process doesn't support dirty ranges, save all regions with write permssions as a backup.
- Modify the TestProcessSaveCoreMinidump.py test case to handle minidump now able to handle eSaveCoreDirtyOnly and eSaveCoreFull

Author: clayborg

lldb/source/Target/Process.cpp

@@ -6311,16 +6311,16 @@ static void GetCoreFileSaveRangesFull(Process &process,
                                       Process::CoreFileMemoryRanges &ranges) {
 
   // Don't add only dirty pages, add full regions.
-  const bool try_dirty_pages = false;
+const bool try_dirty_pages = false;
   for (const auto &region : regions)
     AddRegion(region, try_dirty_pages, ranges);
 }
 
 // Save only the dirty pages to the core file. Make sure the process has at
 // least some dirty pages, as some OS versions don't support reporting what
 // pages are dirty within an memory region. If no memory regions have dirty
-// page information, return an error.
-static Status
+// page information fall back to saving out all ranges with write permissions.
+static void
 GetCoreFileSaveRangesDirtyOnly(Process &process,
                                const MemoryRegionInfos &regions,
                                Process::CoreFileMemoryRanges &ranges) {
@@ -6331,10 +6331,14 @@ GetCoreFileSaveRangesDirtyOnly(Process &process,
       have_dirty_page_info = true;
   }
 
-  if (!have_dirty_page_info)
-    return Status("no process memory regions have dirty page information");
-
-  return Status();
+  if (!have_dirty_page_info) {
+    // We didn't find support for reporting dirty pages from the process
+    // plug-in so fall back to any region with write access permissions.
+    const bool try_dirty_pages = false;
+    for (const auto &region : regions)
+      if (region.GetWritable() == MemoryRegionInfo::eYes)
+        AddRegion(region, try_dirty_pages, ranges);
+  }
 }
 
 // Save all thread stacks to the core file. Some OS versions support reporting
@@ -6405,7 +6409,7 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
     break;
 
   case eSaveCoreDirtyOnly:
-    err = GetCoreFileSaveRangesDirtyOnly(*this, regions, ranges);
+    GetCoreFileSaveRangesDirtyOnly(*this, regions, ranges);
     break;
 
   case eSaveCoreStackOnly:

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

@@ -11,14 +11,50 @@
 
 
 class ProcessSaveCoreMinidumpTestCase(TestBase):
+
+    def verify_core_file(self, core_path, expected_pid, expected_modules,
+                         expected_threads):
+        # To verify, we'll launch with the mini dump
+        target = self.dbg.CreateTarget(None)
+        process = target.LoadCore(core_path)
+
+        # check if the core is in desired state
+        self.assertTrue(process, PROCESS_IS_VALID)
+        self.assertTrue(process.GetProcessInfo().IsValid())
+        self.assertEqual(process.GetProcessInfo().GetProcessID(), expected_pid)
+        self.assertTrue(target.GetTriple().find("linux") != -1)
+        self.assertTrue(target.GetNumModules(), len(expected_modules))
+        self.assertEqual(process.GetNumThreads(), len(expected_threads))
+
+        for module, expected in zip(target.modules, expected_modules):
+            self.assertTrue(module.IsValid())
+            module_file_name = module.GetFileSpec().GetFilename()
+            expected_file_name = expected.GetFileSpec().GetFilename()
+            # skip kernel virtual dynamic shared objects
+            if "vdso" in expected_file_name:
+                continue
+            self.assertEqual(module_file_name, expected_file_name)
+            self.assertEqual(module.GetUUIDString(), expected.GetUUIDString())
+
+        for thread_idx in range(process.GetNumThreads()):
+            thread = process.GetThreadAtIndex(thread_idx)
+            self.assertTrue(thread.IsValid())
+            thread_id = thread.GetThreadID()
+            self.assertTrue(thread_id in expected_threads)
+        self.dbg.DeleteTarget(target)
+
     @skipUnlessArch("x86_64")
     @skipUnlessPlatform(["linux"])
     def test_save_linux_mini_dump(self):
         """Test that we can save a Linux mini dump."""
         self.build()
         exe = self.getBuildArtifact("a.out")
-        core = self.getBuildArtifact("core.dmp")
-        core_sb = self.getBuildArtifact("core_sb.dmp")
+        core_stack = self.getBuildArtifact("core.stack.dmp")
+        core_dirty = self.getBuildArtifact("core.dirty.dmp")
+        core_full = self.getBuildArtifact("core.full.dmp")
+        core_sb_stack = self.getBuildArtifact("core_sb.stack.dmp")
+        core_sb_dirty = self.getBuildArtifact("core_sb.dirty.dmp")
+        core_sb_full = self.getBuildArtifact("core_sb.full.dmp")
         try:
             target = self.dbg.CreateTarget(exe)
             process = target.LaunchSimple(
@@ -41,53 +77,68 @@ def test_save_linux_mini_dump(self):
 
             # save core and, kill process and verify corefile existence
             self.runCmd(
-                "process save-core --plugin-name=minidump --style=stack " + core
+                "process save-core --plugin-name=minidump --style=stack '%s'" % (core_stack)
+            )
+            self.assertTrue(os.path.isfile(core_stack))
+            self.verify_core_file(core_stack, expected_pid, expected_modules,
+                                  expected_threads)
+
+            self.runCmd(
+                "process save-core --plugin-name=minidump --style=modified-memory '%s'" (core_dirty)
+            )
+            self.assertTrue(os.path.isfile(core_dirty))
+            self.verify_core_file(core_dirty, expected_pid, expected_modules,
+                                  expected_threads)
+
+
+            self.runCmd(
+                "process save-core --plugin-name=minidump --style=full '%s'" (core_full)
             )
-            self.assertTrue(os.path.isfile(core))
+            self.assertTrue(os.path.isfile(core_full))
+            self.verify_core_file(core_full, expected_pid, expected_modules,
+                                  expected_threads)
+
 
-            # validate savinig via SBProcess
-            error = process.SaveCore(core_sb, "minidump", lldb.eSaveCoreStackOnly)
+            # validate saving via SBProcess
+            error = process.SaveCore(core_sb_stack, "minidump",
+                                     lldb.eSaveCoreStackOnly)
             self.assertTrue(error.Success())
-            self.assertTrue(os.path.isfile(core_sb))
+            self.assertTrue(os.path.isfile(core_sb_stack))
+            self.verify_core_file(core_sb_stack, expected_pid, expected_modules,
+                                  expected_threads)
 
-            error = process.SaveCore(core_sb, "minidump", lldb.eSaveCoreFull)
-            self.assertTrue(error.Fail())
-            error = process.SaveCore(core_sb, "minidump", lldb.eSaveCoreDirtyOnly)
-            self.assertTrue(error.Fail())
 
-            self.assertSuccess(process.Kill())
+            error = process.SaveCore(core_sb_dirty, "minidump",
+                                     lldb.eSaveCoreDirtyOnly)
+            self.assertTrue(error.Success())
+            self.assertTrue(os.path.isfile(core_sb_dirty))
+            self.verify_core_file(core_sb_dirty, expected_pid, expected_modules,
+                                  expected_threads)
 
-            # To verify, we'll launch with the mini dump
-            target = self.dbg.CreateTarget(None)
-            process = target.LoadCore(core)
-
-            # check if the core is in desired state
-            self.assertTrue(process, PROCESS_IS_VALID)
-            self.assertTrue(process.GetProcessInfo().IsValid())
-            self.assertEqual(process.GetProcessInfo().GetProcessID(), expected_pid)
-            self.assertTrue(target.GetTriple().find("linux") != -1)
-            self.assertTrue(target.GetNumModules(), expected_number_of_modules)
-            self.assertEqual(process.GetNumThreads(), expected_number_of_threads)
-
-            for module, expected in zip(target.modules, expected_modules):
-                self.assertTrue(module.IsValid())
-                module_file_name = module.GetFileSpec().GetFilename()
-                expected_file_name = expected.GetFileSpec().GetFilename()
-                # skip kernel virtual dynamic shared objects
-                if "vdso" in expected_file_name:
-                    continue
-                self.assertEqual(module_file_name, expected_file_name)
-                self.assertEqual(module.GetUUIDString(), expected.GetUUIDString())
+            # Minidump can now save full core files, but they will be huge and
+            # they might cause this test to timeout.
+            error = process.SaveCore(core_sb_full, "minidump",
+                                     lldb.eSaveCoreFull)
+            self.assertTrue(error.Success())
+            self.assertTrue(os.path.isfile(core_sb_full))
+            self.verify_core_file(core_sb_full, expected_pid, expected_modules,
+                                  expected_threads)
 
-            for thread_idx in range(process.GetNumThreads()):
-                thread = process.GetThreadAtIndex(thread_idx)
-                self.assertTrue(thread.IsValid())
-                thread_id = thread.GetThreadID()
-                self.assertTrue(thread_id in expected_threads)
+            self.assertSuccess(process.Kill())
         finally:
             # Clean up the mini dump file.
             self.assertTrue(self.dbg.DeleteTarget(target))
             if os.path.isfile(core):
                 os.unlink(core)
-            if os.path.isfile(core_sb):
-                os.unlink(core_sb)
+            if os.path.isfile(core_stack):
+                os.unlink(core_stack)
+            if os.path.isfile(core_dirty):
+                os.unlink(core_dirty)
+            if os.path.isfile(core_full):
+                os.unlink(core_full)
+            if os.path.isfile(core_sb_stack):
+                os.unlink(core_sb_stack)
+            if os.path.isfile(core_sb_dirty):
+                os.unlink(core_sb_dirty)
+            if os.path.isfile(core_sb_full):
+                os.unlink(core_sb_full)