[Lldb-commits] [lldb] e19387e - We can't let GetStackFrameCount get interrupted or it will give the
Jim Ingham via lldb-commits
lldb-commits at lists.llvm.org
Thu May 11 14:49:16 PDT 2023
Author: Jim Ingham
Date: 2023-05-11T14:48:54-07:00
New Revision: e19387e6936c9ccc6200b32f3affea7b1020664c
URL: https://github.com/llvm/llvm-project/commit/e19387e6936c9ccc6200b32f3affea7b1020664c
DIFF: https://github.com/llvm/llvm-project/commit/e19387e6936c9ccc6200b32f3affea7b1020664c.diff
LOG: We can't let GetStackFrameCount get interrupted or it will give the
wrong answer. Plus, it's useful in some places to have a way to force
the full stack to be created even in the face of
interruption. Moreover, most of the time when you're just getting
frames, you don't need to know the number of frames in the stack to
start with. You just keep calling
Thread::GetStackFrameAtIndex(index++) and when you get a null
StackFrameSP back, you're done. That's also more amenable to
interruption if you are doing some work frame by frame.
So this patch makes GetStackFrameCount always return the full count,
suspending interruption. I also went through all the places that use
GetStackFrameCount to make sure that they really needed the full stack
walk. In many cases, they did not. For instance frame select -r 10 was
getting the number of frames just to check whether cur_frame_idx + 10
was within the stack. It's better in that case to see if that frame
exists first, since that doesn't force a full stack walk, and only
deal with walking off the end of the stack if it doesn't...
I also added a test for some of these behaviors.
Differential Revision: https://reviews.llvm.org/D150236
Added:
lldb/test/API/functionalities/bt-interrupt/Makefile
lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
lldb/test/API/functionalities/bt-interrupt/main.c
Modified:
lldb/include/lldb/Target/StackFrameList.h
lldb/include/lldb/lldb-private-enumerations.h
lldb/source/Commands/CommandCompletions.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
lldb/source/Target/StackFrameList.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index e0bc210298fbe..88e211ff692bd 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -100,7 +100,11 @@ class StackFrameList {
bool SetFrameAtIndex(uint32_t idx, lldb::StackFrameSP &frame_sp);
- void GetFramesUpTo(uint32_t end_idx);
+ /// Realizes frames up to (and including) end_idx (which can be greater than
+ /// the actual number of frames.)
+ /// Returns true if the function was interrupted, false otherwise.
+ bool GetFramesUpTo(uint32_t end_idx,
+ InterruptionControl allow_interrupt = AllowInterruption);
void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder);
diff --git a/lldb/include/lldb/lldb-private-enumerations.h b/lldb/include/lldb/lldb-private-enumerations.h
index 4fae9e6e38ff6..7f98220f9f164 100644
--- a/lldb/include/lldb/lldb-private-enumerations.h
+++ b/lldb/include/lldb/lldb-private-enumerations.h
@@ -274,4 +274,9 @@ enum SelectMostRelevant : bool {
DoNoSelectMostRelevantFrame = false,
};
+enum InterruptionControl : bool {
+ AllowInterruption = true,
+ DoNotAllowInterruption = false,
+};
+
#endif // LLDB_LLDB_PRIVATE_ENUMERATIONS_H
diff --git a/lldb/source/Commands/CommandCompletions.cpp b/lldb/source/Commands/CommandCompletions.cpp
index a3c9cf78dfe1a..bacb00cfe35ff 100644
--- a/lldb/source/Commands/CommandCompletions.cpp
+++ b/lldb/source/Commands/CommandCompletions.cpp
@@ -718,10 +718,14 @@ void CommandCompletions::FrameIndexes(CommandInterpreter &interpreter,
return;
lldb::ThreadSP thread_sp = exe_ctx.GetThreadSP();
+ Debugger &dbg = interpreter.GetDebugger();
const uint32_t frame_num = thread_sp->GetStackFrameCount();
for (uint32_t i = 0; i < frame_num; ++i) {
lldb::StackFrameSP frame_sp = thread_sp->GetStackFrameAtIndex(i);
StreamString strm;
+ // Dumping frames can be slow, allow interruption.
+ if (dbg.InterruptRequested())
+ break;
frame_sp->Dump(&strm, false, true);
request.TryCompleteCurrentArg(std::to_string(i), strm.GetString());
}
diff --git a/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp b/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
index 5ce85f47072e9..ae5e1b7baed57 100644
--- a/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
+++ b/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
@@ -220,8 +220,7 @@ void SystemRuntimeMacOSX::AddThreadExtendedInfoPacketHints(
}
bool SystemRuntimeMacOSX::SafeToCallFunctionsOnThisThread(ThreadSP thread_sp) {
- if (thread_sp && thread_sp->GetStackFrameCount() > 0 &&
- thread_sp->GetFrameWithConcreteFrameIndex(0)) {
+ if (thread_sp && thread_sp->GetFrameWithConcreteFrameIndex(0)) {
const SymbolContext sym_ctx(
thread_sp->GetFrameWithConcreteFrameIndex(0)->GetSymbolContext(
eSymbolContextSymbol));
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index 86939a14a288d..81e7b00ee995d 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -85,8 +85,8 @@ void StackFrameList::ResetCurrentInlinedDepth() {
return;
std::lock_guard<std::recursive_mutex> guard(m_mutex);
-
- GetFramesUpTo(0);
+
+ GetFramesUpTo(0, DoNotAllowInterruption);
if (m_frames.empty())
return;
if (!m_frames[0]->IsInlined()) {
@@ -436,21 +436,23 @@ void StackFrameList::SynthesizeTailCallFrames(StackFrame &next_frame) {
next_frame.SetFrameIndex(m_frames.size());
}
-void StackFrameList::GetFramesUpTo(uint32_t end_idx) {
+bool StackFrameList::GetFramesUpTo(uint32_t end_idx,
+ InterruptionControl allow_interrupt) {
// Do not fetch frames for an invalid thread.
+ bool was_interrupted = false;
if (!m_thread.IsValid())
- return;
+ return false;
// We've already gotten more frames than asked for, or we've already finished
// unwinding, return.
if (m_frames.size() > end_idx || GetAllFramesFetched())
- return;
+ return false;
Unwind &unwinder = m_thread.GetUnwinder();
if (!m_show_inlined_frames) {
GetOnlyConcreteFramesUpTo(end_idx, unwinder);
- return;
+ return false;
}
#if defined(DEBUG_STACK_FRAMES)
@@ -474,13 +476,6 @@ void StackFrameList::GetFramesUpTo(uint32_t end_idx) {
StackFrameSP unwind_frame_sp;
Debugger &dbg = m_thread.GetProcess()->GetTarget().GetDebugger();
do {
- // Check for interruption here when building the frames - this is the
- // expensive part, Dump later on is cheap.
- if (dbg.InterruptRequested()) {
- Log *log = GetLog(LLDBLog::Host);
- LLDB_LOG(log, "Interrupted %s", __FUNCTION__);
- break;
- }
uint32_t idx = m_concrete_frames_fetched++;
lldb::addr_t pc = LLDB_INVALID_ADDRESS;
lldb::addr_t cfa = LLDB_INVALID_ADDRESS;
@@ -512,6 +507,15 @@ void StackFrameList::GetFramesUpTo(uint32_t end_idx) {
cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
}
} else {
+ // Check for interruption when building the frames.
+ // Do the check in idx > 0 so that we'll always create a 0th frame.
+ if (allow_interrupt && dbg.InterruptRequested()) {
+ Log *log = GetLog(LLDBLog::Host);
+ LLDB_LOG(log, "Interrupted %s", __FUNCTION__);
+ was_interrupted = true;
+ break;
+ }
+
const bool success =
unwinder.GetFrameInfoAtIndex(idx, cfa, pc, behaves_like_zeroth_frame);
if (!success) {
@@ -624,14 +628,19 @@ void StackFrameList::GetFramesUpTo(uint32_t end_idx) {
Dump(&s);
s.EOL();
#endif
+ // Don't report interrupted if we happen to have gotten all the frames:
+ if (!GetAllFramesFetched())
+ return was_interrupted;
+ return false;
}
uint32_t StackFrameList::GetNumFrames(bool can_create) {
std::lock_guard<std::recursive_mutex> guard(m_mutex);
- if (can_create)
- GetFramesUpTo(UINT32_MAX);
-
+ if (can_create) {
+ // Don't allow interrupt or we might not return the correct count
+ GetFramesUpTo(UINT32_MAX, DoNotAllowInterruption);
+ }
return GetVisibleStackFrameIndex(m_frames.size());
}
@@ -672,7 +681,13 @@ StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
// GetFramesUpTo will fill m_frames with as many frames as you asked for, if
// there are that many. If there weren't then you asked for too many frames.
- GetFramesUpTo(idx);
+ // GetFramesUpTo returns true if interrupted:
+ if (GetFramesUpTo(idx)) {
+ Log *log = GetLog(LLDBLog::Thread);
+ LLDB_LOG(log, "GetFrameAtIndex was interrupted");
+ return {};
+ }
+
if (idx < m_frames.size()) {
if (m_show_inlined_frames) {
// When inline frames are enabled we actually create all the frames in
@@ -947,6 +962,14 @@ size_t StackFrameList::GetStatus(Stream &strm, uint32_t first_frame,
else
marker = unselected_marker;
}
+ // Check for interruption here. If we're fetching arguments, this loop
+ // can go slowly:
+ Debugger &dbg = m_thread.GetProcess()->GetTarget().GetDebugger();
+ if (dbg.InterruptRequested()) {
+ Log *log = GetLog(LLDBLog::Host);
+ LLDB_LOG(log, "Interrupted %s", __FUNCTION__);
+ break;
+ }
if (!frame_sp->GetStatus(strm, show_frame_info,
num_frames_with_source > (first_frame - frame_idx),
diff --git a/lldb/test/API/functionalities/bt-interrupt/Makefile b/lldb/test/API/functionalities/bt-interrupt/Makefile
new file mode 100644
index 0000000000000..695335e068c0c
--- /dev/null
+++ b/lldb/test/API/functionalities/bt-interrupt/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py b/lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
new file mode 100644
index 0000000000000..57a994d11035d
--- /dev/null
+++ b/lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
@@ -0,0 +1,49 @@
+"""
+Ensure that when the interrupt is raised we still make frame 0.
+and make sure "GetNumFrames" isn't interrupted.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestInterruptingBacktrace(TestBase):
+
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_backtrace_interrupt(self):
+ """Use RequestInterrupt followed by stack operations
+ to ensure correct interrupt behavior for stacks."""
+ self.build()
+ self.main_source_file = lldb.SBFileSpec("main.c")
+ self.bt_interrupt_test()
+
+ def bt_interrupt_test(self):
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+ "Set a breakpoint here", self.main_source_file)
+
+ # Now continue, and when we stop we will have crashed.
+ process.Continue()
+ self.dbg.RequestInterrupt()
+
+ # Be sure to turn this off again:
+ def cleanup ():
+ if self.dbg.InterruptRequested():
+ self.dbg.CancelInterruptRequest()
+ self.addTearDownHook(cleanup)
+
+ frame_0 = thread.GetFrameAtIndex(0)
+ self.assertTrue(frame_0.IsValid(), "Got a good 0th frame")
+ # The interrupt flag is up already, so any attempt to backtrace
+ # should be cut short:
+ frame_1 = thread.GetFrameAtIndex(1)
+ self.assertFalse(frame_1.IsValid(), "Prevented from getting more frames")
+ # Since GetNumFrames is a contract, we don't interrupt it:
+ num_frames = thread.GetNumFrames()
+ print(f"Number of frames: {num_frames}")
+ self.assertGreater(num_frames, 1, "Got many frames")
+
+ self.dbg.CancelInterruptRequest()
+
+
diff --git a/lldb/test/API/functionalities/bt-interrupt/main.c b/lldb/test/API/functionalities/bt-interrupt/main.c
new file mode 100644
index 0000000000000..bdaf423d334ef
--- /dev/null
+++ b/lldb/test/API/functionalities/bt-interrupt/main.c
@@ -0,0 +1,23 @@
+#include <stdio.h>
+
+// This example is meant to recurse infinitely.
+// The extra struct is just to make the frame dump
+// more complicated.
+
+struct Foo {
+ int a;
+ int b;
+ char *c;
+};
+
+int
+forgot_termination(int input, struct Foo my_foo) {
+ return forgot_termination(++input, my_foo);
+}
+
+int
+main()
+{
+ struct Foo myFoo = {100, 300, "A string you will print a lot"}; // Set a breakpoint here
+ return forgot_termination(1, myFoo);
+}
More information about the lldb-commits
mailing list