[LLDB/Coredump] Only take the Pthread from stack start to the stackpointer + red_zone (llvm#92002)

Jlalond · web-flow · commit 47d80ec1802d · 2024-05-16T14:17:19.000-07:00
Currently in Core dumps, the entire pthread is copied, including the
unused space beyond the stack pointer. This causes large amounts of core
dump inflation when the number of threads is high, but the stack usage
is low. Such as when an application is using a thread pool.

This change will optimize for these situations in addition to generally
improving the core dump performance for all of lldb.
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/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();
 
diff --git a/lldb/source/Target/Process.cpp b/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;
   }
 
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/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())
