[Lldb-commits] [lldb] [LLDB/Coredump] Only take the Pthread from start start to the stackpointer + red_zone (PR #92002)
Jacob Lalonde via lldb-commits
lldb-commits at lists.llvm.org
Mon May 13 10:20:10 PDT 2024
https://github.com/Jlalond created https://github.com/llvm/llvm-project/pull/92002
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.
>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/3] 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/3] 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/3] 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())
More information about the lldb-commits
mailing list