Thread::GetStackFrameCount should forestall interruption

[jingham]

Summary

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.

[bulbazord]

lldb/include/lldb/Target/StackFrameList.h

103–104

Is the range inclusive of end_idx? as in, is it [0, end_idx) or [0, end_idx]?

lldb/source/Target/StackFrameList.cpp

89

nit:

GetFramesUpTo(/*end_idx = */0, /*allow_interrupt = */false);

Or something like this... A little easier to understand IMO.

640–641

Same here I think.

683–684

nit: Since interrupted isn't used anywhere else, you could do something like:

if (bool interrupted = GetFramesUpTo(idx)) {
 // Code here
}

You could also just do if (GetFramesUpTo(idx)) but I think the name of the function isn't descriptive enough to do that and stay readable.

lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py

17

I have a feeling this docstring was supposed to be different?

[mib]

I don't understand why the succeeding return value for GetFramesUpTo is false. It looks counter-intuitive to me. What's the motivation behind that ?

lldb/include/lldb/Target/StackFrameList.h

103–104

@bulbazord I believe this should be inclusive.

@jingham This comment sounds like we only return true if the function was interrupted, which is not the expected behavior, right ?

lldb/source/Target/StackFrameList.cpp

448

I find it confusing the fail here given that we've succeeded at fetching the frames

633

Again, I believe this should return true since we succeeded at retrieving all the requested frames.

683–684

+1: reading if (GetFramesUpTo(idx)), I'd never think that the succeeding result would be false.

lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py

47

Is this necessary if you already have it in the cleanup method ?

[jingham]

lldb/include/lldb/Target/StackFrameList.h

103–104

The function used to return void, so it had no expected behavior before. I am defining the "bool" return here, and I define it to be "was_interrupted" as I say in the doc string. That's also why the return values you are questioning below are actually correct. This is NOT returning "success" it is returning "interrupted".

lldb/source/Target/StackFrameList.cpp

448

GetFramesUpTo returns true if interrupted, false if not. This was not interrupted, so it should return false. I could switch this to "true if not interrupted" so that I could return true here, but that would be weird. This isn't a "success or fail" result, it's an "interrupted or not interrupted" result.

[jingham]

Address review comments

[JDevlieghere]

lldb/include/lldb/Target/StackFrameList.h

106

I personally would prefer to have an InterruptPolicy (e.g. AllowInterrupt, DenyInterrupt) to limit the proliferation of boolean flags and improve readability.

lldb/source/Target/StackFrameList.cpp

513–514

Nit: you could inline his and still fit in a single line (probably).

Long term I think we should have an API in Debugger that allows you to report who was interrupted and then we can centralize this logging + have a way for the API tests to request that info (rather than having to parse the logs).

[mib]

lldb/include/lldb/Target/StackFrameList.h

103–104

Given that this changes the behavior of GetFramesUpTo, I'd rather pass a bool reference that says is_interrupted and keep void as the return type, because if (GetFramesUpTo(idx, true) == false) really doesn't tell me much. Looking at this code, I'd think why did GetFramesUpTo failed.

106

+1, this would greatly improve readability

[jingham]

Address Review Comments

[jingham]

I added the enum, that's a good idea.

I agree that we should centralize internal reporting of Interrupt events, but as your "longer term" suggests, that's a design task and orthogonal to this patch. I'll do that in a subsequent patch.

lldb/include/lldb/Target/StackFrameList.h

103–104

I still don't see this. GetFramesUpTo says clearly what the return value is, so I don't see why this is confusing. You would only thing "why did GetFramesUpTo fail" if you didn't read the function definition.

Moreover, the code you are looking at will always do:

if (GetFramesUpTo(idx, AllowInterruption)) {

// Do something that's explicitly handling interruption

}

so it should be pretty clear what is going on.

I don't think adding another parameter is warranted. You would have to pass it even it you passed DoNotAllowInterruption, which is weird. Or we could take a bool * but that's just more code to no very good purpose, IMO.

[mib]

I guess the InterruptionControl argument makes it clearer. LGTM

lldb/include/lldb/Target/StackFrameList.h

size_t GetStatus(Stream &strm, uint32_t first_frame, uint32_t num_frames,
                    const char *frame_marker = nullptr);
 
 protected:
   friend class Thread;
   friend class ScriptedThread;
 
   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);
 
   void SynthesizeTailCallFrames(StackFrame &next_frame);
 
   bool GetAllFramesFetched() { return m_concrete_frames_fetched == UINT32_MAX; }
 
   void SetAllFramesFetched() { m_concrete_frames_fetched = UINT32_MAX; }

lldb/include/lldb/lldb-private-enumerations.h

 };
 }
 
 enum SelectMostRelevant : bool {
   SelectMostRelevantFrame = true,
   DoNoSelectMostRelevantFrame = false,
 };
 
+enum InterruptionControl : bool {
+  AllowInterruption = true,
+  DoNotAllowInterruption = false,
+};
+
 #endif // LLDB_LLDB_PRIVATE_ENUMERATIONS_H

lldb/source/Commands/CommandCompletions.cpp

 void CommandCompletions::FrameIndexes(CommandInterpreter &interpreter,
                                       CompletionRequest &request,
                                       SearchFilter *searcher) {
   const ExecutionContext &exe_ctx = interpreter.GetExecutionContext();
   if (!exe_ctx.HasProcessScope())
     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());
   }
 }
 
 void CommandCompletions::StopHookIDs(CommandInterpreter &interpreter,
                                      CompletionRequest &request,
                                      SearchFilter *searcher) {

lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp

if (m_libdispatch_tsd_indexes.IsValid()) {
                            m_libdispatch_tsd_indexes.dti_voucher_index);
       dict->AddIntegerItem("dti_qos_class_index",
                            m_libdispatch_tsd_indexes.dti_qos_class_index);
     }
   }
 }
 
 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));
     static ConstString g_select_symbol("__select");
     if (sym_ctx.GetFunctionName() == g_select_symbol) {
       return false;
     }
   }

lldb/source/Target/StackFrameList.cpp

uint32_t StackFrameList::GetCurrentInlinedDepth() {
   }
 }
 
 void StackFrameList::ResetCurrentInlinedDepth() {
   if (!m_show_inlined_frames)
     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()) {
     m_current_inlined_depth = UINT32_MAX;
     m_current_inlined_pc = LLDB_INVALID_ADDRESS;
     Log *log = GetLog(LLDBLog::Step);
     if (log && log->GetVerbose())
       LLDB_LOGF(
for (auto calleeInfo : llvm::reverse(path)) {
     LLDB_LOG(log, "Pushed frame {0} at {1:x}", callee->GetDisplayName(), pc);
   }
 
   // If any frames were created, adjust next_frame's index.
   if (!path.empty())
     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)
   StreamFile s(stdout, false);
 #endif
   // If we are hiding some frames from the outside world, we need to add
   // those onto the total count of frames to fetch.  However, we don't need
   // to do that if end_idx is 0 since in that case we always get the first
   // concrete frame and all the inlined frames below it...  And of course, if
   // end_idx is UINT32_MAX that means get all, so just do that...
 
   uint32_t inlined_depth = 0;
   if (end_idx > 0 && end_idx != UINT32_MAX) {
     inlined_depth = GetCurrentInlinedDepth();
     if (inlined_depth != UINT32_MAX) {
       if (end_idx > 0)
         end_idx += inlined_depth;
     }
   }
 
   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;
     bool behaves_like_zeroth_frame = (idx == 0);
     if (idx == 0) {
       // We might have already created frame zero, only create it if we need
       // to.
       if (m_frames.empty()) {
if (idx == 0) {
               cfa, pc, behaves_like_zeroth_frame, nullptr);
           m_frames.push_back(unwind_frame_sp);
         }
       } else {
         unwind_frame_sp = m_frames.front();
         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) {
         // We've gotten to the end of the stack.
         SetAllFramesFetched();
         break;
       }
       const bool cfa_is_valid = true;
#endif
     m_prev_frames_sp.reset();
   }
 
 #if defined(DEBUG_STACK_FRAMES)
   s.PutCString("\n\nNew frames:\n");
   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());
 }
 
 void StackFrameList::Dump(Stream *s) {
   if (s == nullptr)
     return;
 
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
StackFrameSP StackFrameList::GetFrameAtIndex(uint32_t idx) {
 
   if (idx < m_frames.size())
     frame_sp = m_frames[idx];
 
   if (frame_sp)
     return frame_sp;
 
   // 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
       // GetFramesUpTo.
       frame_sp = m_frames[idx];
     } else {
       addr_t pc, cfa;
       bool behaves_like_zeroth_frame = (idx == 0);
if (!frame_sp)
       break;
 
     if (selected_frame_marker != nullptr) {
       if (frame_sp == selected_frame_sp)
         marker = selected_frame_marker;
       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),
                              show_unique, marker))
       break;
     ++num_frames_displayed;
   }
 
   strm.IndentLess();
   return num_frames_displayed;
 }

lldb/test/API/functionalities/bt-interrupt/Makefile

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules

lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py

+"""
+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()
+
+        

lldb/test/API/functionalities/bt-interrupt/main.c

+#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);
+}