Fix docstrings and implement dmpots suggestions

Jlalond · Jlalond · commit b2e941dc263f · 2025-07-17T16:23:52.000-07:00
diff --git a/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i b/lldb/bindings/interface/SBSaveCoreOptionsDocstrings.i
@@ -46,7 +46,7 @@ Note that currently ELF Core files are not supported."
 ) lldb::SBSaveCoreOptions::SetProcess;
 
 %feature("docstring", "
-    Get the process to save. If undefined, an invalid SBProcess will be returned."
+    Get the process to save. If a process is not defined, whether by calling clear or by not setting a process, an invalid process will be returned."
 ) lldb::SBSaveCoreOptions::GetProcess;
 
 %feature("docstring", "
@@ -69,7 +69,7 @@ Note that currently ELF Core files are not supported."
 
 %feature("docstring", "
     Get an SBMemoryRegionInfoList of all the Regions that LLDB will attempt to write into the Core. Note, reading from these
-    regions can fail, and it's guaraunteed every region will be present. If called without a valid process or style set an empty
+    regions can fail, and it's not guaraunteed every region will be present in the resulting core. If called without a valid process or style set an empty
     collection will be returned."
 ) lldb::SBSaveCoreOptions::GetMemoryRegionsToSave;
 
diff --git a/lldb/source/API/SBSaveCoreOptions.cpp b/lldb/source/API/SBSaveCoreOptions.cpp
@@ -142,15 +142,15 @@ lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() {
     return SBMemoryRegionInfoList();
   }
 
-  SBMemoryRegionInfoList m_memory_region_infos;
+  SBMemoryRegionInfoList memory_region_infos;
   for (const auto &range : *memory_ranges) {
     SBMemoryRegionInfo region_info(
         nullptr, range.GetRangeBase(), range.GetRangeEnd(),
         range.data.lldb_permissions, /*mapped=*/true);
-    m_memory_region_infos.Append(region_info);
+    memory_region_infos.Append(region_info);
   }
 
-  return m_memory_region_infos;
+  return memory_region_infos;
 }
 
 lldb_private::SaveCoreOptions &SBSaveCoreOptions::ref() const {
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
@@ -965,13 +965,11 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp,
     return error;
   }
 
-  // Set the process sp if not already set.
-  ProcessSP options_sp = options.GetProcess();
-  if (!options_sp)
+  if (!options.GetProcess())
     options.SetProcess(process_sp);
 
   // Make sure the process sp is the same as the one we are using.
-  if (options_sp && options_sp != process_sp) {
+  if (options.GetProcess() != process_sp) {
     error = Status::FromErrorString(
         "Save Core Options configured for a different process.");
     return error;
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -984,9 +984,17 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks(
 
     if (current_addr != addr + total_bytes_read) {
       LLDB_LOGF(log,
-                "Current addr is at expected address, 0x%" PRIx64
+                "Current addr is at unexpected address, 0x%" PRIx64
                 ", expected at 0x%" PRIx64,
                 current_addr, addr + total_bytes_read);
+
+      // Something went wrong and the address is not where it should be
+      // we'll error out of this Minidump generation.
+      addDataError = Status::FromErrorStringWithFormat(
+          "Unexpected address encounterd when reading memory in chunks "
+          "0x" PRIx64 " expected 0x" PRIx64,
+          current_addr, addr + total_bytes_read);
+      return lldb_private::IterationAction::Stop;
     }
 
     // Write to the minidump file with the chunk potentially flushing to
diff --git a/lldb/source/Symbol/SaveCoreOptions.cpp b/lldb/source/Symbol/SaveCoreOptions.cpp
@@ -194,7 +194,7 @@ llvm::Expected<uint64_t> SaveCoreOptions::GetCurrentSizeInBytes() {
   const lldb_private::CoreFileMemoryRanges &core_file_ranges =
       *core_file_ranges_maybe;
   uint64_t total_in_bytes = 0;
-  for (auto &core_range : core_file_ranges)
+  for (const auto &core_range : core_file_ranges)
     total_in_bytes += core_range.data.range.size();
 
   return total_in_bytes;
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump64b.py
@@ -13,118 +13,95 @@
 class ProcessSaveCoreMinidump64bTestCase(TestBase):
     def verify_minidump(
         self,
-        core_proc,
-        live_proc,
         options,
     ):
         """Verify that the minidump is the same byte for byte as the live process."""
-        # Get the memory regions we saved off in this core, we can't compare to the core
-        # because we pull from /proc/pid/maps, so even ranges that don't get mapped in will show up
-        # as ranges in the minidump.
-        #
-        # Instead, we have an API that returns to us the number of regions we planned to save from the live process
-        # and we compare those
-        memory_regions_to_compare = options.GetMemoryRegionsToSave()
-
-        for region in memory_regions_to_compare:
-            start_addr = region.GetRegionBase()
-            end_addr = region.GetRegionEnd()
-            actual_process_read_error = lldb.SBError()
-            actual = live_proc.ReadMemory(
-                start_addr, end_addr - start_addr, actual_process_read_error
-            )
-            expected_process_read_error = lldb.SBError()
-            expected = core_proc.ReadMemory(
-                start_addr, end_addr - start_addr, expected_process_read_error
-            )
-
-            # Both processes could fail to read a given memory region, so if they both pass
-            # compare, then we'll fail them if the core differs from the live process.
-            if (
-                actual_process_read_error.Success()
-                and expected_process_read_error.Success()
-            ):
-                self.assertEqual(
-                    actual, expected, "Bytes differ between live process and core"
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        target = self.dbg.CreateTarget(exe)
+        core_target = None
+        live_proc = target.LaunchSimple(
+            None, None, self.get_process_working_directory()
+        )
+        try:
+            self.assertState(live_proc.GetState(), lldb.eStateStopped)
+            error = live_proc.SaveCore(options)
+            self.assertTrue(error.Success(), error.GetCString())
+            core_target = self.dbg.CreateTarget(None)
+            core_proc = target.LoadCore(options.GetOutputFile().fullpath)
+            # Get the memory regions we saved off in this core, we can't compare to the core
+            # because we pull from /proc/pid/maps, so even ranges that don't get mapped in will show up
+            # as ranges in the minidump.
+            #
+            # Instead, we have an API that returns to us the number of regions we planned to save from the live process
+            # and we compare those
+            memory_regions_to_compare = options.GetMemoryRegionsToSave()
+
+            for region in memory_regions_to_compare:
+                start_addr = region.GetRegionBase()
+                end_addr = region.GetRegionEnd()
+                actual_process_read_error = lldb.SBError()
+                actual = live_proc.ReadMemory(
+                    start_addr, end_addr - start_addr, actual_process_read_error
+                )
+                expected_process_read_error = lldb.SBError()
+                expected = core_proc.ReadMemory(
+                    start_addr, end_addr - start_addr, expected_process_read_error
                 )
 
-            # Now we check if the error is the same, error isn't abnormal but they should fail for the same reason
-            self.assertTrue(
-                (
+                # Both processes could fail to read a given memory region, so if they both pass
+                # compare, then we'll fail them if the core differs from the live process.
+                if (
                     actual_process_read_error.Success()
                     and expected_process_read_error.Success()
+                ):
+                    self.assertEqual(
+                        actual, expected, "Bytes differ between live process and core"
+                    )
+
+                # Now we check if the error is the same, error isn't abnormal but they should fail for the same reason
+                self.assertTrue(
+                    (
+                        actual_process_read_error.Success()
+                        and expected_process_read_error.Success()
+                    )
+                    or (
+                        actual_process_read_error.Fail()
+                        and expected_process_read_error.Fail()
+                    ),
+                    f"Address range {hex(start_addr)} - {hex(end_addr)} failed to read from live process and core for different reasons",
                 )
-                or (
-                    actual_process_read_error.Fail()
-                    and expected_process_read_error.Fail()
-                ),
-                f"Address range {hex(start_addr)} - {hex(end_addr)} failed to read from live process and core for different reasons",
-            )
+        finally:
+            self.assertTrue(self.dbg.DeleteTarget(target))
+            if core_target is not None:
+                self.assertTrue(self.dbg.DeleteTarget(core_target))
 
     @skipUnlessArch("x86_64")
     @skipUnlessPlatform(["linux"])
     def test_minidump_save_style_full(self):
         """Test that a full minidump is the same byte for byte."""
-
-        self.build()
-        exe = self.getBuildArtifact("a.out")
         minidump_path = self.getBuildArtifact("minidump_full_force64b.dmp")
-
         try:
-            target = self.dbg.CreateTarget(exe)
-            live_process = target.LaunchSimple(
-                None, None, self.get_process_working_directory()
-            )
-            self.assertState(live_process.GetState(), lldb.eStateStopped)
             options = lldb.SBSaveCoreOptions()
-
             options.SetOutputFile(lldb.SBFileSpec(minidump_path))
             options.SetStyle(lldb.eSaveCoreFull)
             options.SetPluginName("minidump")
-            options.SetProcess(live_process)
-
-            error = live_process.SaveCore(options)
-            self.assertTrue(error.Success(), error.GetCString())
-
-            target = self.dbg.CreateTarget(None)
-            core_proc = target.LoadCore(minidump_path)
-
-            self.verify_minidump(core_proc, live_process, options)
+            self.verify_minidump(options)
         finally:
-            self.assertTrue(self.dbg.DeleteTarget(target))
             if os.path.isfile(minidump_path):
                 os.unlink(minidump_path)
 
     @skipUnlessArch("x86_64")
     @skipUnlessPlatform(["linux"])
     def test_minidump_save_style_mixed_memory(self):
         """Test that a mixed memory minidump is the same byte for byte."""
-
-        self.build()
-        exe = self.getBuildArtifact("a.out")
         minidump_path = self.getBuildArtifact("minidump_mixed_force64b.dmp")
-
         try:
-            target = self.dbg.CreateTarget(exe)
-            live_process = target.LaunchSimple(
-                None, None, self.get_process_working_directory()
-            )
-            self.assertState(live_process.GetState(), lldb.eStateStopped)
             options = lldb.SBSaveCoreOptions()
-
             options.SetOutputFile(lldb.SBFileSpec(minidump_path))
             options.SetStyle(lldb.eSaveCoreDirtyOnly)
             options.SetPluginName("minidump")
-            options.SetProcess(live_process)
-
-            error = live_process.SaveCore(options)
-            self.assertTrue(error.Success(), error.GetCString())
-
-            target = self.dbg.CreateTarget(None)
-            core_proc = target.LoadCore(minidump_path)
-
-            self.verify_minidump(core_proc, live_process, options)
+            self.verify_minidump(options)
         finally:
-            self.assertTrue(self.dbg.DeleteTarget(target))
             if os.path.isfile(minidump_path):
                 os.unlink(minidump_path)

Original file line number	Diff line number	Diff line change
`@@ -142,15 +142,15 @@ lldb::SBMemoryRegionInfoList SBSaveCoreOptions::GetMemoryRegionsToSave() {`
`142`	`142`	`return SBMemoryRegionInfoList();`
`143`	`143`	`}`
`144`	`144`
`145`		`- SBMemoryRegionInfoList m_memory_region_infos;`
	`145`	`+ SBMemoryRegionInfoList memory_region_infos;`
`146`	`146`	`for (const auto &range : *memory_ranges) {`
`147`	`147`	`SBMemoryRegionInfo region_info(`
`148`	`148`	`nullptr, range.GetRangeBase(), range.GetRangeEnd(),`
`149`	`149`	`range.data.lldb_permissions, /mapped=/true);`
`150`		`- m_memory_region_infos.Append(region_info);`
	`150`	`+ memory_region_infos.Append(region_info);`
`151`	`151`	`}`
`152`	`152`
`153`		`- return m_memory_region_infos;`
	`153`	`+ return memory_region_infos;`
`154`	`154`	`}`
`155`	`155`
`156`	`156`	`lldb_private::SaveCoreOptions &SBSaveCoreOptions::ref() const {`