[LLDB/Coredump] Only take the Pthread from stack start to the stackpointer + red_zone #92002
Conversation
…d zone) Add python tests to verify we can read up to sp + redzone - 1.
…nters instead of trying to take the entire range
…the process to ensure the addresses are saved correctly.
|
@clayborg Could you take a look at this? |
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesCurrently 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. Full diff: https://github.com/llvm/llvm-project/pull/92002.diff 2 Files Affected:
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..56f75ec7e9708 100644
--- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
+++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
@@ -9,10 +9,9 @@
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
+ 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 +35,37 @@ 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()
+ # 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)
@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,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
@@ -100,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
@@ -116,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())
|
lldb/source/Target/Process.cpp
Outdated
| @@ -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(); | |||
There was a problem hiding this comment.
Asking for my understanding:
This will only really do anything if the size of the red zone is greater than 0 right? Otherwise, this change is a no-op. Is that right?
There was a problem hiding this comment.
Correct, but on systems where it would be greater than 0, we ensure we capture it for the memory dump as well.
Jlalond
changed the title
| // cases where relying on that side-effect causes the shutdown to be | ||
| // flakey, so we should send a positive signal to interrupt the wait. |
There was a problem hiding this comment.
This got pulled back in by running clang-formatter on this file. Several different places were reformatted, do you want me to manually revert this still?
lldb/source/Target/Process.cpp
Outdated
| 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); |
There was a problem hiding this comment.
Let's do some validation that stack_head is greater than range start in case GetRedZoneSize returns some bogus value.
There was a problem hiding this comment.
What do you have in mind? Can we not trust ABI to return us a valid value?
There was a problem hiding this comment.
It depends on where does the API get the value from. I haven't looked but it is likely GetRedZoneSize() value is fetched from dwarf, which can be bogus value generated from compiler/linker/BOLT etc...
Simply sanity check that if (stack_head > sp_region.GetRange().GetRangeBase()) will ensure we are not reading reading beyond valid memory region.
There was a problem hiding this comment.
It depends on where does the API get the value from. I haven't looked but it is likely
GetRedZoneSize()value is fetched from dwarf, which can be bogus value generated from compiler/linker/BOLT etc...Simply sanity check that
if (stack_head > sp_region.GetRange().GetRangeBase())will ensure we are not reading reading beyond valid memory region.
Red zone is gotten from the ABI plug-ins. This is correct behavior, but we should make sure that (sp - red_zone) is actually not before the start of the memory region.
const addr_t stack_head = (sp - red_zone) > sp_region.GetRange.GetRangeBase() ? (sp - red_zone) : sp_region.GetRange.GetRangeBase();
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
…r | Fix the saivng stack logic in MinidumpFileBuilder | Save off stack's before all other styles and prevent double saving of that region
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
Jlalond
force-pushed
the
memory-regions-stackpointer-limiting
branch
from
99c0a9f to
25ff389
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
✅ With the latest revision this PR passed the Python code formatter. |
| // 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(); |
There was a problem hiding this comment.
In general we don't want to use size_t, it is 32 bits on 32 bit system and 64 on 64 bit systems. Change to addr_t
lldb/source/Target/Process.cpp
Outdated
| const size_t stack_head = (sp - red_zone); | ||
| const size_t stack_size = sp_region.GetRange().GetRangeEnd() - stack_head; |
There was a problem hiding this comment.
Use addr_t as this will cause 32 bit compiled LLDB's to not be able to correctly save 64 bit core files because size_t is 32 bits on 32 bit systems
lldb/source/Target/Process.cpp
Outdated
| 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); | ||
| } |
There was a problem hiding this comment.
We should check if the stack head makes sense before adusting it:
We should make sure stack_head is >= sp_region.GetRange().GetRangeBase():
// Adjust start of head of stack if possible
const size_t stack_head = (sp - red_zone);
if (stack_head > sp_region.GetRange().GetRangeBase()) {
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);
There was a problem hiding this comment.
Done, also made sure we do this in MinidumpFileBuilder
…ckhead is above the bottom of the memory range, and finally run the formatter
|
@Jlalond Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
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.