[LLDB/Coredump] Only take the Pthread from stack start to the stackpointer + red_zone

lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp

@@ -14,6 +14,7 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/Section.h"
+#include "lldb/Target/ABI.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
@@ -491,6 +492,17 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) {
         std::errc::not_supported,
         "unable to load stack segment of the process");
 
+  // This is a duplicate of the logic in
+  // Process::SaveOffRegionsWithStackPointers but ultimately, we need to only
+  // save up from the start of the stack down to the stack pointer
+  const addr_t range_end = range_info.GetRange().GetRangeEnd();
+  const addr_t red_zone = process_sp->GetABI()->GetRedZoneSize();
+  const addr_t stack_head = rsp - red_zone;
+  if (stack_head > range_info.GetRange().GetRangeEnd()) {
+    range_info.GetRange().SetRangeBase(stack_head);
+    range_info.GetRange().SetByteSize(range_end - stack_head);
+  }
+
   const addr_t addr = range_info.GetRange().GetRangeBase();
   const addr_t size = range_info.GetRange().GetByteSize();
 

lldb/source/Target/Process.cpp

@@ -3857,8 +3857,8 @@ thread_result_t Process::RunPrivateStateThread(bool is_secondary_thread) {
         // case we should tell it to stop doing that.  Normally, we don't NEED
         // to do that because we will next close the communication to the stub
         // and that will get it to shut down.  But there are remote debugging
-        // cases where relying on that side-effect causes the shutdown to be 
-        // flakey, so we should send a positive signal to interrupt the wait. 
+        // cases where relying on that side-effect causes the shutdown to be
+        // flakey, so we should send a positive signal to interrupt the wait.
         Status error = HaltPrivate();
         BroadcastEvent(eBroadcastBitInterrupt, nullptr);
       } else if (StateIsRunningState(m_last_broadcast_state)) {
@@ -6335,30 +6335,65 @@ static void AddRegion(const MemoryRegionInfo &region, bool try_dirty_pages,
   ranges.push_back(CreateCoreFileMemoryRange(region));
 }
 
+static void SaveOffRegionsWithStackPointers(
+    Process &process, const MemoryRegionInfos &regions,
+    Process::CoreFileMemoryRanges &ranges, std::set<addr_t> &stack_ends) {
+  const bool try_dirty_pages = true;
+
+  // Before we take any dump, we want to save off the used portions of the
+  // stacks and mark those memory regions as saved. This prevents us from saving
+  // the unused portion of the stack below the stack pointer. Saving space on
+  // the dump.
+  for (lldb::ThreadSP thread_sp : process.GetThreadList().Threads()) {
+    if (!thread_sp)
+      continue;
+    StackFrameSP frame_sp = thread_sp->GetStackFrameAtIndex(0);
+    if (!frame_sp)
+      continue;
+    RegisterContextSP reg_ctx_sp = frame_sp->GetRegisterContext();
+    if (!reg_ctx_sp)
+      continue;
+    const addr_t sp = reg_ctx_sp->GetSP();
+    const size_t red_zone = process.GetABI()->GetRedZoneSize();
+    lldb_private::MemoryRegionInfo sp_region;
+    if (process.GetMemoryRegionInfo(sp, sp_region).Success()) {
+      const size_t stack_head = (sp - red_zone);
+      const size_t stack_size = sp_region.GetRange().GetRangeEnd() - stack_head;
+      sp_region.GetRange().SetRangeBase(stack_head);
+      sp_region.GetRange().SetByteSize(stack_size);
+      stack_ends.insert(sp_region.GetRange().GetRangeEnd());
+      AddRegion(sp_region, try_dirty_pages, ranges);
+    }
+  }
+}
+
 // Save all memory regions that are not empty or have at least some permissions
 // for a full core file style.
 static void GetCoreFileSaveRangesFull(Process &process,
                                       const MemoryRegionInfos &regions,
-                                      Process::CoreFileMemoryRanges &ranges) {
+                                      Process::CoreFileMemoryRanges &ranges,
+                                      std::set<addr_t> &stack_ends) {
 
   // Don't add only dirty pages, add full regions.
 const bool try_dirty_pages = false;
   for (const auto &region : regions)
-    AddRegion(region, try_dirty_pages, ranges);
+    if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0)
+      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 fall back to saving out all ranges with write permissions.
-static void
-GetCoreFileSaveRangesDirtyOnly(Process &process,
-                               const MemoryRegionInfos &regions,
-                               Process::CoreFileMemoryRanges &ranges) {
+static void GetCoreFileSaveRangesDirtyOnly(
+    Process &process, const MemoryRegionInfos &regions,
+    Process::CoreFileMemoryRanges &ranges, std::set<addr_t> &stack_ends) {
+
   // Iterate over the regions and find all dirty pages.
   bool have_dirty_page_info = false;
   for (const auto &region : regions) {
-    if (AddDirtyPages(region, ranges))
+    if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0 &&
+        AddDirtyPages(region, ranges))
       have_dirty_page_info = true;
   }
 
@@ -6367,7 +6402,8 @@ GetCoreFileSaveRangesDirtyOnly(Process &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)
+      if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0 &&
+          region.GetWritable() == MemoryRegionInfo::eYes)
         AddRegion(region, try_dirty_pages, ranges);
   }
 }
@@ -6380,43 +6416,18 @@ GetCoreFileSaveRangesDirtyOnly(Process &process,
 // dirty regions as this will make the core file smaller. If the process
 // doesn't support dirty regions, then it will fall back to adding the full
 // stack region.
-static void
-GetCoreFileSaveRangesStackOnly(Process &process,
-                               const MemoryRegionInfos &regions,
-                               Process::CoreFileMemoryRanges &ranges) {
+static void GetCoreFileSaveRangesStackOnly(
+    Process &process, const MemoryRegionInfos &regions,
+    Process::CoreFileMemoryRanges &ranges, std::set<addr_t> &stack_ends) {
+  const bool try_dirty_pages = true;
   // Some platforms support annotating the region information that tell us that
   // it comes from a thread stack. So look for those regions first.
 
-  // Keep track of which stack regions we have added
-  std::set<addr_t> stack_bases;
-
-  const bool try_dirty_pages = true;
   for (const auto &region : regions) {
-    if (region.IsStackMemory() == MemoryRegionInfo::eYes) {
-      stack_bases.insert(region.GetRange().GetRangeBase());
+    // Save all the stack memory ranges not associated with a stack pointer.
+    if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0 &&
+        region.IsStackMemory() == MemoryRegionInfo::eYes)
       AddRegion(region, try_dirty_pages, ranges);
-    }
-  }
-
-  // Also check with our threads and get the regions for their stack pointers
-  // and add those regions if not already added above.
-  for (lldb::ThreadSP thread_sp : process.GetThreadList().Threads()) {
-    if (!thread_sp)
-      continue;
-    StackFrameSP frame_sp = thread_sp->GetStackFrameAtIndex(0);
-    if (!frame_sp)
-      continue;
-    RegisterContextSP reg_ctx_sp = frame_sp->GetRegisterContext();
-    if (!reg_ctx_sp)
-      continue;
-    const addr_t sp = reg_ctx_sp->GetSP();
-    lldb_private::MemoryRegionInfo sp_region;
-    if (process.GetMemoryRegionInfo(sp, sp_region).Success()) {
-      // Only add this region if not already added above. If our stack pointer
-      // is pointing off in the weeds, we will want this range.
-      if (stack_bases.count(sp_region.GetRange().GetRangeBase()) == 0)
-        AddRegion(sp_region, try_dirty_pages, ranges);
-    }
   }
 }
 
@@ -6428,23 +6439,27 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
     return err;
   if (regions.empty())
     return Status("failed to get any valid memory regions from the process");
+  if (core_style == eSaveCoreUnspecified)
+    return Status("callers must set the core_style to something other than "
+                  "eSaveCoreUnspecified");
+
+  std::set<addr_t> stack_ends;
+  SaveOffRegionsWithStackPointers(*this, regions, ranges, stack_ends);
 
   switch (core_style) {
   case eSaveCoreUnspecified:
-    err = Status("callers must set the core_style to something other than "
-                 "eSaveCoreUnspecified");
     break;
 
   case eSaveCoreFull:
-    GetCoreFileSaveRangesFull(*this, regions, ranges);
+    GetCoreFileSaveRangesFull(*this, regions, ranges, stack_ends);
     break;
 
   case eSaveCoreDirtyOnly:
-    GetCoreFileSaveRangesDirtyOnly(*this, regions, ranges);
+    GetCoreFileSaveRangesDirtyOnly(*this, regions, ranges, stack_ends);
     break;
 
   case eSaveCoreStackOnly:
-    GetCoreFileSaveRangesStackOnly(*this, regions, ranges);
+    GetCoreFileSaveRangesStackOnly(*this, regions, ranges, stack_ends);
     break;
   }
 

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

@@ -2,7 +2,6 @@
 Test saving a mini dump.
 """
 
-
 import os
 import lldb
 from lldbsuite.test.decorators import *
@@ -12,7 +11,12 @@
 
 class ProcessSaveCoreMinidumpTestCase(TestBase):
     def verify_core_file(
-        self, core_path, expected_pid, expected_modules, expected_threads
+        self,
+        core_path,
+        expected_pid,
+        expected_modules,
+        expected_threads,
+        stacks_to_sps_map,
     ):
         # To verify, we'll launch with the mini dump
         target = self.dbg.CreateTarget(None)
@@ -36,17 +40,36 @@ def verify_core_file(
             self.assertEqual(module_file_name, expected_file_name)
             self.assertEqual(module.GetUUIDString(), expected.GetUUIDString())
 
+        red_zone = process.GetTarget().GetStackRedZoneSize()
         for thread_idx in range(process.GetNumThreads()):
             thread = process.GetThreadAtIndex(thread_idx)
             self.assertTrue(thread.IsValid())
             thread_id = thread.GetThreadID()
             self.assertIn(thread_id, expected_threads)
+            frame = thread.GetFrameAtIndex(0)
+            sp_region = lldb.SBMemoryRegionInfo()
+            sp = frame.GetSP()
+            err = process.GetMemoryRegionInfo(sp, sp_region)
+            self.assertTrue(err.Success(), err.GetCString())
+            error = lldb.SBError()
+            # Ensure thread_id is in the saved map
+            self.assertIn(thread_id, stacks_to_sps_map)
+            # Ensure the SP is correct
+            self.assertEqual(stacks_to_sps_map[thread_id], sp)
+            # Try to read at the end of the stack red zone and succeed
+            process.ReadMemory(sp - red_zone, 1, error)
+            self.assertTrue(error.Success(), error.GetCString())
+            # Try to read just past the red zone and fail
+            process.ReadMemory(sp - red_zone - 1, 1, error)
+            self.assertTrue(error.Fail(), "No failure when reading past the red zone")
+
         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_stack = self.getBuildArtifact("core.stack.dmp")
@@ -69,45 +92,67 @@ def test_save_linux_mini_dump(self):
             expected_modules = target.modules
             expected_number_of_threads = process.GetNumThreads()
             expected_threads = []
+            stacks_to_sp_map = {}
 
             for thread_idx in range(process.GetNumThreads()):
                 thread = process.GetThreadAtIndex(thread_idx)
                 thread_id = thread.GetThreadID()
                 expected_threads.append(thread_id)
+                stacks_to_sp_map[thread_id] = thread.GetFrameAtIndex(0).GetSP()
 
             # save core and, kill process and verify corefile existence
             base_command = "process save-core --plugin-name=minidump "
             self.runCmd(base_command + " --style=stack '%s'" % (core_stack))
             self.assertTrue(os.path.isfile(core_stack))
             self.verify_core_file(
-                core_stack, expected_pid, expected_modules, expected_threads
+                core_stack,
+                expected_pid,
+                expected_modules,
+                expected_threads,
+                stacks_to_sp_map,
             )
 
             self.runCmd(base_command + " --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
+                core_dirty,
+                expected_pid,
+                expected_modules,
+                expected_threads,
+                stacks_to_sp_map,
             )
 
             self.runCmd(base_command + " --style=full '%s'" % (core_full))
             self.assertTrue(os.path.isfile(core_full))
             self.verify_core_file(
-                core_full, expected_pid, expected_modules, expected_threads
+                core_full,
+                expected_pid,
+                expected_modules,
+                expected_threads,
+                stacks_to_sp_map,
             )
 
             # validate saving via SBProcess
             error = process.SaveCore(core_sb_stack, "minidump", lldb.eSaveCoreStackOnly)
             self.assertTrue(error.Success())
             self.assertTrue(os.path.isfile(core_sb_stack))
             self.verify_core_file(
-                core_sb_stack, expected_pid, expected_modules, expected_threads
+                core_sb_stack,
+                expected_pid,
+                expected_modules,
+                expected_threads,
+                stacks_to_sp_map,
             )
 
             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
+                core_sb_dirty,
+                expected_pid,
+                expected_modules,
+                expected_threads,
+                stacks_to_sp_map,
             )
 
             # Minidump can now save full core files, but they will be huge and
@@ -116,7 +161,11 @@ def test_save_linux_mini_dump(self):
             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
+                core_sb_full,
+                expected_pid,
+                expected_modules,
+                expected_threads,
+                stacks_to_sp_map,
             )
 
             self.assertSuccess(process.Kill())