[Lldb-commits] [lldb] [LLDB/Coredump] Only take the Pthread from stack start to the stackpointer + red_zone (PR #92002)
Jacob Lalonde via lldb-commits
lldb-commits at lists.llvm.org
Thu May 16 11:28:03 PDT 2024
https://github.com/Jlalond updated https://github.com/llvm/llvm-project/pull/92002
>From 2d192f640b332c2f1381cf96b75be60ad18de3ac Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Fri, 10 May 2024 09:35:11 -0700
Subject: [PATCH 1/5] change core dump stacks to only include up to the stack
pointer (+ red zone) Add python tests to verify we can read up to sp +
redzone - 1.
---
lldb/source/Target/Process.cpp | 14 +++++++++++---
.../TestProcessSaveCoreMinidump.py | 12 ++++++++++++
2 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 25afade9a8275..a11e45909202f 100644
--- 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)) {
@@ -6410,12 +6410,20 @@ GetCoreFileSaveRangesStackOnly(Process &process,
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)
+ 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);
+ }
}
}
}
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 9fe5e89142987..123bbd472be05 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -14,6 +14,7 @@ class ProcessSaveCoreMinidumpTestCase(TestBase):
def verify_core_file(
self, core_path, expected_pid, expected_modules, expected_threads
):
+ breakpoint()
# To verify, we'll launch with the mini dump
target = self.dbg.CreateTarget(None)
process = target.LoadCore(core_path)
@@ -36,11 +37,22 @@ 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)
+ oldest_frame = thread.GetFrameAtIndex(thread.GetNumFrames() - 1)
+ stack_start = oldest_frame.GetSymbol().GetStartAddress().GetFileAddress()
+ frame = thread.GetFrameAtIndex(0)
+ sp = frame.GetSP()
+ stack_size = stack_start - (sp - red_zone)
+ byte_array = process.ReadMemory(sp - red_zone + 1, stack_size, error)
+ self.assertTrue(error.Success(), "Failed to read stack")
+ self.assertEqual(stack_size - 1, len(byte_array), "Incorrect stack size read"),
+
+
self.dbg.DeleteTarget(target)
@skipUnlessArch("x86_64")
>From aa6d0a24b64816c328c7c4c3d00c7563407a3deb Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Fri, 10 May 2024 14:51:12 -0700
Subject: [PATCH 2/5] Refactor test to read the top and bottom stack frame's
respective pointers instead of trying to take the entire range
---
.../TestProcessSaveCoreMinidump.py | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 123bbd472be05..163bccbe08164 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -9,12 +9,10 @@
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
-
class ProcessSaveCoreMinidumpTestCase(TestBase):
def verify_core_file(
self, core_path, expected_pid, expected_modules, expected_threads
):
- breakpoint()
# To verify, we'll launch with the mini dump
target = self.dbg.CreateTarget(None)
process = target.LoadCore(core_path)
@@ -44,13 +42,14 @@ def verify_core_file(
thread_id = thread.GetThreadID()
self.assertIn(thread_id, expected_threads)
oldest_frame = thread.GetFrameAtIndex(thread.GetNumFrames() - 1)
- stack_start = oldest_frame.GetSymbol().GetStartAddress().GetFileAddress()
+ stack_start = oldest_frame.GetSP()
frame = thread.GetFrameAtIndex(0)
sp = frame.GetSP()
- stack_size = stack_start - (sp - red_zone)
- byte_array = process.ReadMemory(sp - red_zone + 1, stack_size, error)
- self.assertTrue(error.Success(), "Failed to read stack")
- self.assertEqual(stack_size - 1, len(byte_array), "Incorrect stack size read"),
+ error = lldb.SBError()
+ process.ReadMemory(sp - red_zone + 1, 1, error)
+ self.assertTrue(error.Success(), error.GetCString())
+ process.ReadMemory(stack_start - 1, 1, error)
+ self.assertTrue(error.Success(), error.GetCString())
self.dbg.DeleteTarget(target)
@@ -59,6 +58,7 @@ def verify_core_file(
@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")
>From 5953f54e14ec6242e2bc8d6ed6de593815854771 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Mon, 13 May 2024 09:53:04 -0700
Subject: [PATCH 3/5] Compare saved stack pointer and the cached thread stack
pointer from the process to ensure the addresses are saved correctly.
---
.../TestProcessSaveCoreMinidump.py | 33 ++++++++++++-------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 163bccbe08164..56f75ec7e9708 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -11,7 +11,7 @@
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)
@@ -41,15 +41,22 @@ def verify_core_file(
self.assertTrue(thread.IsValid())
thread_id = thread.GetThreadID()
self.assertIn(thread_id, expected_threads)
- oldest_frame = thread.GetFrameAtIndex(thread.GetNumFrames() - 1)
- stack_start = oldest_frame.GetSP()
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()
- process.ReadMemory(sp - red_zone + 1, 1, error)
- self.assertTrue(error.Success(), error.GetCString())
- process.ReadMemory(stack_start - 1, 1, error)
+ # Try to read at the end of the stack red zone and succeed
+ process.ReadMemory(sp_region.GetRegionEnd() - 1, 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)
self.dbg.DeleteTarget(target)
@@ -81,30 +88,32 @@ 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
@@ -112,14 +121,14 @@ def test_save_linux_mini_dump(self):
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
@@ -128,7 +137,7 @@ 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())
>From 25ff389fd2b1ca5caccfe239d8ab1bd166ee75cb Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Wed, 15 May 2024 14:53:48 -0700
Subject: [PATCH 4/5] 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
---
.../Minidump/MinidumpFileBuilder.cpp | 10 +-
lldb/source/Target/Process.cpp | 108 ++++++++++--------
.../TestProcessSaveCoreMinidump.py | 15 +--
3 files changed, 75 insertions(+), 58 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 601f11d51d428..190c7670d1f8d 100644
--- 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"
@@ -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,
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index a11e45909202f..6a3bfe0a6a188 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6335,16 +6335,51 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
ranges.push_back(CreateCoreFileMemoryRange(region));
}
+static void
+SaveOffRegionsWithStackPointers(Process &process,
+ const MemoryRegionInfos ®ions,
+ 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 ®ions,
- 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 ®ion : 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 ®ions,
- 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 ®ion : 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 ®ion : 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 ®ions,
- 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 ®ion : 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;
}
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 56f75ec7e9708..0718ae7d1c74a 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/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)
>From 879f2b639c755a0465207b347a4f77562648cc8d Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Thu, 16 May 2024 11:27:23 -0700
Subject: [PATCH 5/5] Code review feedback on addr_t, only update the stacks if
the new stackhead is above the bottom of the memory range, and finally run
the formatter
---
.../Minidump/MinidumpFileBuilder.cpp | 18 ++++--
lldb/source/Target/Process.cpp | 59 +++++++++----------
.../TestProcessSaveCoreMinidump.py | 47 +++++++++++----
3 files changed, 77 insertions(+), 47 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 190c7670d1f8d..7231433619ffb 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -491,12 +491,20 @@ findStackHelper(const lldb::ProcessSP &process_sp, uint64_t rsp) {
return llvm::createStringError(
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.
+
+ // 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;
+ 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();
if (size == 0)
return llvm::createStringError(std::errc::not_supported,
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 6a3bfe0a6a188..216d2f21abfef 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -6335,16 +6335,15 @@ static void AddRegion(const MemoryRegionInfo ®ion, bool try_dirty_pages,
ranges.push_back(CreateCoreFileMemoryRange(region));
}
-static void
-SaveOffRegionsWithStackPointers(Process &process,
- const MemoryRegionInfos ®ions,
- Process::CoreFileMemoryRanges &ranges,
- std::set<addr_t> &stack_ends) {
+static void SaveOffRegionsWithStackPointers(
+ Process &process, const MemoryRegionInfos ®ions,
+ 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.
+ // 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;
@@ -6358,12 +6357,12 @@ SaveOffRegionsWithStackPointers(Process &process,
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);
+ 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);
}
}
}
@@ -6386,17 +6385,15 @@ const bool try_dirty_pages = false;
// 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 ®ions,
- Process::CoreFileMemoryRanges &ranges,
- std::set<addr_t> &stack_ends) {
+static void GetCoreFileSaveRangesDirtyOnly(
+ Process &process, const MemoryRegionInfos ®ions,
+ 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 ®ion : regions) {
- if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0
- && AddDirtyPages(region, ranges))
+ if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0 &&
+ AddDirtyPages(region, ranges))
have_dirty_page_info = true;
}
@@ -6405,8 +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 ®ion : regions)
- if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0
- && region.GetWritable() == MemoryRegionInfo::eYes)
+ if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0 &&
+ region.GetWritable() == MemoryRegionInfo::eYes)
AddRegion(region, try_dirty_pages, ranges);
}
}
@@ -6419,20 +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 ®ions,
- Process::CoreFileMemoryRanges &ranges,
- std::set<addr_t> &stack_ends) {
+static void GetCoreFileSaveRangesStackOnly(
+ Process &process, const MemoryRegionInfos ®ions,
+ 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.
for (const auto ®ion : regions) {
// 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);
+ if (stack_ends.count(region.GetRange().GetRangeEnd()) == 0 &&
+ region.IsStackMemory() == MemoryRegionInfo::eYes)
+ AddRegion(region, try_dirty_pages, ranges);
}
}
@@ -6446,7 +6441,7 @@ Status Process::CalculateCoreFileSaveRanges(lldb::SaveCoreStyle core_style,
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");
+ "eSaveCoreUnspecified");
std::set<addr_t> stack_ends;
SaveOffRegionsWithStackPointers(*this, regions, ranges, stack_ends);
diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
index 0718ae7d1c74a..1e171e726fb6b 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -2,16 +2,21 @@
Test saving a mini dump.
"""
-
import os
import lldb
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
+
class ProcessSaveCoreMinidumpTestCase(TestBase):
def verify_core_file(
- self, core_path, expected_pid, expected_modules, expected_threads, stacks_to_sps_map
+ 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)
@@ -58,8 +63,6 @@ def verify_core_file(
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")
@@ -102,19 +105,31 @@ def test_save_linux_mini_dump(self):
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, stacks_to_sp_map
+ 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, stacks_to_sp_map
+ 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, stacks_to_sp_map
+ core_full,
+ expected_pid,
+ expected_modules,
+ expected_threads,
+ stacks_to_sp_map,
)
# validate saving via SBProcess
@@ -122,14 +137,22 @@ def test_save_linux_mini_dump(self):
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, stacks_to_sp_map
+ 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, stacks_to_sp_map
+ 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
@@ -138,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, stacks_to_sp_map
+ core_sb_full,
+ expected_pid,
+ expected_modules,
+ expected_threads,
+ stacks_to_sp_map,
)
self.assertSuccess(process.Kill())
More information about the lldb-commits
mailing list