jingham created this revision.
jingham added reviewers: JDevlieghere, mib, bulbazord.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D150236
Files:
lldb/include/lldb/Target/StackFrameList.h
lldb/source/Commands/CommandCompletions.cpp
lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
lldb/source/Target/StackFrameList.cpp
lldb/test/API/functionalities/bt-interrupt/Makefile
lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
lldb/test/API/functionalities/bt-interrupt/main.c
Index: lldb/test/API/functionalities/bt-interrupt/main.c
===================================================================
--- /dev/null
+++ 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);
+}
Index: lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
@@ -0,0 +1,48 @@
+"""
+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):
+ """There can be many tests in a test case - describe this test here."""
+ 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()
+
+
Index: lldb/test/API/functionalities/bt-interrupt/Makefile
===================================================================
--- /dev/null
+++ lldb/test/API/functionalities/bt-interrupt/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
Index: lldb/source/Target/StackFrameList.cpp
===================================================================
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -86,7 +86,7 @@
std::lock_guard<std::recursive_mutex> guard(m_mutex);
- GetFramesUpTo(0);
+ GetFramesUpTo(0, false);
if (m_frames.empty())
return;
if (!m_frames[0]->IsInlined()) {
@@ -436,21 +436,22 @@
next_frame.SetFrameIndex(m_frames.size());
}
-void StackFrameList::GetFramesUpTo(uint32_t end_idx) {
+bool StackFrameList::GetFramesUpTo(uint32_t end_idx, bool 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 +475,6 @@
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 +506,15 @@
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,13 +627,18 @@
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);
+ GetFramesUpTo(UINT32_MAX, false); // Don't allow interrupt or we might not
+ // return the correct count
return GetVisibleStackFrameIndex(m_frames.size());
}
@@ -672,7 +680,13 @@
// 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);
+ bool interrupted = GetFramesUpTo(idx);
+ if (interrupted) {
+ 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 +961,14 @@
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),
Index: lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
===================================================================
--- lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
+++ lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
@@ -220,8 +220,7 @@
}
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));
Index: lldb/source/Commands/CommandCompletions.cpp
===================================================================
--- lldb/source/Commands/CommandCompletions.cpp
+++ lldb/source/Commands/CommandCompletions.cpp
@@ -721,10 +721,14 @@
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());
}
Index: lldb/include/lldb/Target/StackFrameList.h
===================================================================
--- lldb/include/lldb/Target/StackFrameList.h
+++ lldb/include/lldb/Target/StackFrameList.h
@@ -100,7 +100,9 @@
bool SetFrameAtIndex(uint32_t idx, lldb::StackFrameSP &frame_sp);
- void GetFramesUpTo(uint32_t end_idx);
+ /// Gets frames up to 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, bool allow_interrupt = true);
void GetOnlyConcreteFramesUpTo(uint32_t end_idx, Unwind &unwinder);
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
