Fix test to validate we're reading beyond the end of the stack pointer | Fix the saivng stack logic in MinidumpFileBuilder | Save off stack's before all other styles and prevent double saving of that region

Author: Jlalond

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"
@@ -490,9 +491,12 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) {
     return llvm::createStringError(
         std::errc::not_supported,
         "unable to load stack segment of the process");
-
-  const addr_t addr = range_info.GetRange().GetRangeBase();
-  const addr_t size = range_info.GetRange().GetByteSize();
+  // 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 size_t red_zone = process_sp->GetABI()->GetRedZoneSize();
+  const addr_t addr = rsp - red_zone;
+  const addr_t size = range_end - addr;
 
   if (size == 0)
     return llvm::createStringError(std::errc::not_supported,

lldb/source/Target/Process.cpp

@@ -6335,16 +6335,51 @@ 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
@@ -6354,11 +6389,14 @@ const bool try_dirty_pages = false;
 static void
 GetCoreFileSaveRangesDirtyOnly(Process &process,
                                const MemoryRegionInfos &regions,
-                               Process::CoreFileMemoryRanges &ranges) {
+                               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 +6405,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);
   }
 }
@@ -6383,48 +6422,17 @@ GetCoreFileSaveRangesDirtyOnly(Process &process,
 static void
 GetCoreFileSaveRangesStackOnly(Process &process,
                                const MemoryRegionInfos &regions,
-                               Process::CoreFileMemoryRanges &ranges) {
+                               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());
-      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();
-    const size_t red_zone = process.GetABI()->GetRedZoneSize();
-    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) {
-        // Take only the start of the stack to the stack pointer and include the redzone.
-        // Because stacks grow 'down' to include the red_zone we have to subtract it from the sp.
-        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);
-        AddRegion(sp_region, try_dirty_pages, ranges);
-      }
-    }
+    // 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);
   }
 }
 
@@ -6436,23 +6444,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

@@ -47,16 +47,17 @@ def verify_core_file(
             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_region.GetRegionEnd() - 1, 1, error)
+            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_region.GetRegionEnd() + 1, 1, error)
-            # Try to read from the base of the stack
-            self.assertTrue(error.Fail(), error.GetCString())
-            self.assertTrue(stacks_to_sps_map.__contains__(thread_id), "stacks_to_sps_map does not contain thread_idx: %d" % thread_idx)
-            # Ensure the SP is correct
-            self.assertEqual(stacks_to_sps_map[thread_id], sp)
+            process.ReadMemory(sp - red_zone - 1, 1, error)
+            self.assertTrue(error.Fail(), "No failure when reading past the red zone")
+
 
 
         self.dbg.DeleteTarget(target)