Move "SelectMostRelevantFrame" from Thread::WillStop

[jingham]

Summary

SelectMostRelevantFrame triggers the StackFrameRecognizer construction, which can run arbitrary Python code, call expressions etc. WillStop gets called on every private stop while the recognizers are a user-facing feature, so first off doing this work on every stop is inefficient. But more importantly, you can get in to situations where the recognizer causes an expression to get run, then when we fetch the stop event at the end of the expression evaluation, we call WillStop again on the expression handling thread, which will do the same StackFrameRecognizer work again. If anyone is locking along that path, you will end up with a deadlock between the two threads.

The example that brought this to my attention was the objc_exception_throw recognizer which can cause the objc runtime introspection functions to get run, and those take a lock in lldb_private::AppleObjCRuntimeV2::DynamicClassInfoExtractor::UpdateISAToDescriptorMap along this path, so the second thread servicing the expression deadlocks against the first thread waiting for the expression to complete.

It makes more sense to have the frame recognizers run on demand, either when someone asks for the variables for the frame, or when someone does GetSelectedFrame. The former already worked that way, the only reason this was being done in WillStop was because the StackFrameRecognizers can change the SelectedFrame, so you needed to run them before the anyone requested the SelectedFrame.

This patch moves SelectMostRelevantFrame to StackFrameList, and runs it when GetSelectedFrame is called for the first time on a given stop. If you call SetSelectedFrame before GetSelectedFrame, then you should NOT run the recognizer & change the frame out from under you. This patch also makes that work. There were already tests for this behavior, and for the feature that caused the hang, but the hang is racy, and it doesn't trigger all the time, so I don't have a way to test that explicitly.

One more detail: it's actually pretty easy to end up calling GetSelectedFrame, for instance if you ask for the best ExecutionContext from an ExecutionContextRef it will fill the StackFrame with the result of GetSelectedFrame and that would still have the same problems if this happens on the Private State Thread. So this patch also short-circuits SelectMostRelevantFrame if run on the that thread. I can't think of any reason the computations that go on on the Private State Thread would actually want the SelectedFrame - that's a user-facing concept, so avoiding that complication is the best way to go.

[JDevlieghere]

lldb/include/lldb/Target/StackFrameList.h

138–143

The purpose of the boolean is to differentiate between m_selected_frame_idx == 0 meaning the zeroth frame is selected versus we haven't yet selected the current frame, right? If so I think we should make this an std::optional<bool>.

lldb/source/Target/StackFrameList.cpp

822

IIUC SetSelectedFrame always sets m_selected_frame_set to true, so is this redundant?

882

Should m_selected_frame_idx also be set to 0 here?

[jingham]

lldb/include/lldb/Target/StackFrameList.h

138–143

I don't follow. Did you mean make m_selected_frame_idx a std::optional<uint32_t>?

You could do it that way for sure. I didn't because I'm not as fond of gadgets. Most clients should be calling GetSelectedFrameIndex which will always set it to something (0 if it can't figure anything else out). So the optionality shouldn't be much visible.

lldb/source/Target/StackFrameList.cpp

822

Sure, but if you do that you have to change SelectMostRelevantFrame to select the 0th frame if there's no relevant frame. At present it leaves it unselected. Doing the selection always in SMRF is clearer, though, so it would be a good change.

882

Yes, though this function doesn't currently matter.

[JDevlieghere]

lldb/source/Target/StackFrameList.cpp

822

Nvm, I confused SelectMostRelevantFrame with SetSelectedFrame

[mib]

This makes sense to me. LGTM!

[JDevlieghere]

Jim asked me to land this on his behalf as he's OOO. I've addressed my own comments :-)

lldb/include/lldb/Target/StackFrameList.h

public:
 
   /// Retrieve the stack frame with the given ID \p stack_id.
   lldb::StackFrameSP GetFrameWithStackID(const StackID &stack_id);
 
   /// Mark a stack frame as the currently selected frame and return its index.
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
 
   /// Get the currently selected frame index.
-  uint32_t GetSelectedFrameIndex() const;
+  uint32_t GetSelectedFrameIndex();
 
   /// Mark a stack frame as the currently selected frame using the frame index
   /// \p idx. Like \ref GetFrameAtIndex, invisible frames cannot be selected.
   bool SetSelectedFrameByIndex(uint32_t idx);
 
   /// If the current inline depth (i.e the number of invisible frames) is valid,
   /// subtract it from \p idx. Otherwise simply return \p idx.
   uint32_t GetVisibleStackFrameIndex(uint32_t idx) {
protected:
   bool DecrementCurrentInlinedDepth();
 
   void ResetCurrentInlinedDepth();
 
   uint32_t GetCurrentInlinedDepth();
 
   void SetCurrentInlinedDepth(uint32_t new_depth);
 
+  void SelectMostRelevantFrame();
+
   typedef std::vector<lldb::StackFrameSP> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
   /// The thread this frame list describes.
   Thread &m_thread;
 
   /// The old stack frame list.
   // TODO: The old stack frame list is used to fill in missing frame info
   // heuristically when it's otherwise unavailable (say, because the unwinder
   // fails). We should have stronger checks to make sure that this is a valid
   // source of information.
   lldb::StackFrameListSP m_prev_frames_sp;
 
   /// A mutex for this frame list.
   // TODO: This mutex may not always be held when required. In particular, uses
   // of the StackFrameList APIs in lldb_private::Thread look suspect. Consider
   // passing around a lock_guard reference to enforce proper locking.
   mutable std::recursive_mutex m_mutex;
 
   /// A cache of frames. This may need to be updated when the program counter
   /// changes.
   collection m_frames;
 
-  /// The currently selected frame.
-  uint32_t m_selected_frame_idx;
+  /// The currently selected frame. An optional is used to record whether anyone
+  /// has set the selected frame on this stack yet. We only let recognizers
+  /// change the frame if this is the first time GetSelectedFrame is called.
+  std::optional<uint32_t> m_selected_frame_idx;
 
   /// The number of concrete frames fetched while filling the frame list. This
   /// is only used when synthetic frames are enabled.
   uint32_t m_concrete_frames_fetched;
 
   /// The number of synthetic function activations (invisible frames) expanded
   /// from the concrete frame #0 activation.
   // TODO: Use an optional instead of UINT32_MAX to denote invalid values.
   uint32_t m_current_inlined_depth;

lldb/include/lldb/Target/Thread.h

public:
   // sure to call it.
   virtual void DidResume();
 
   // This notifies the thread when a private stop occurs.
   virtual void DidStop();
 
   virtual void RefreshStateAfterStop() = 0;
 
-  void SelectMostRelevantFrame();
-
   std::string GetStopDescription();
 
   std::string GetStopDescriptionRaw();
 
   void WillStop();
 
   bool ShouldStop(Event *event_ptr);
 

lldb/source/Target/StackFrameList.cpp

 #include "lldb/Core/SourceManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/StackFrame.h"
+#include "lldb/Target/StackFrameRecognizer.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/Unwind.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/ADT/SmallPtrSet.h"
 
 #include <memory>
 
 //#define DEBUG_STACK_FRAMES 1
 
 using namespace lldb;
 using namespace lldb_private;
 
 // StackFrameList constructor
 StackFrameList::StackFrameList(Thread &thread,
                                const lldb::StackFrameListSP &prev_frames_sp,
                                bool show_inline_frames)
     : m_thread(thread), m_prev_frames_sp(prev_frames_sp), m_mutex(), m_frames(),
-      m_selected_frame_idx(0), m_concrete_frames_fetched(0),
+      m_selected_frame_idx(), m_concrete_frames_fetched(0),
       m_current_inlined_depth(UINT32_MAX),
       m_current_inlined_pc(LLDB_INVALID_ADDRESS),
       m_show_inlined_frames(show_inline_frames) {
   if (prev_frames_sp) {
     m_current_inlined_depth = prev_frames_sp->m_current_inlined_depth;
     m_current_inlined_pc = prev_frames_sp->m_current_inlined_pc;
   }
 }

 struct CallDescriptor {
   Function *func;
   CallEdge::AddrType address_type = CallEdge::AddrType::Call;
   addr_t address = LLDB_INVALID_ADDRESS;
 };
 using CallSequence = std::vector<CallDescriptor>;
 
 /// Find the unique path through the call graph from \p begin (with return PC
-/// \p return_pc) to \p end. On success this path is stored into \p path, and 
+/// \p return_pc) to \p end. On success this path is stored into \p path, and
 /// on failure \p path is unchanged.
 static void FindInterveningFrames(Function &begin, Function &end,
                                   ExecutionContext &exe_ctx, Target &target,
                                   addr_t return_pc, CallSequence &path,
                                   ModuleList &images, Log *log) {
   LLDB_LOG(log, "Finding frames between {0} and {1}, retn-pc={2:x}",
            begin.GetDisplayName(), end.GetDisplayName(), return_pc);
 
bool StackFrameList::SetFrameAtIndex(uint32_t idx, StackFrameSP &frame_sp) {
   // Make sure allocation succeeded by checking bounds again
   if (idx < m_frames.size()) {
     m_frames[idx] = frame_sp;
     return true;
   }
   return false; // resize failed, out of memory?
 }
 
-uint32_t StackFrameList::GetSelectedFrameIndex() const {
+void StackFrameList::SelectMostRelevantFrame() {
+  // Don't call into the frame recognizers on the private state thread as
+  // they can cause code to run in the target, and that can cause deadlocks
+  // when fetching stop events for the expression.
+  if (m_thread.GetProcess()->CurrentThreadIsPrivateStateThread())
+    return;
+
+  Log *log = GetLog(LLDBLog::Thread);
+
+  // Only the top frame should be recognized.
+  StackFrameSP frame_sp = GetFrameAtIndex(0);
+  if (!frame_sp) {
+    LLDB_LOG(log, "Failed to construct Frame #0");
+    return;
+  }
+
+  RecognizedStackFrameSP recognized_frame_sp = frame_sp->GetRecognizedFrame();
+
+  if (!recognized_frame_sp) {
+    LLDB_LOG(log, "Frame #0 not recognized");
+    return;
+  }
+
+  if (StackFrameSP most_relevant_frame_sp =
+          recognized_frame_sp->GetMostRelevantFrame()) {
+    LLDB_LOG(log, "Found most relevant frame at index {0}",
+             most_relevant_frame_sp->GetFrameIndex());
+    SetSelectedFrame(most_relevant_frame_sp.get());
+  } else {
+    LLDB_LOG(log, "No relevant frame!");
+  }
+}
+
+uint32_t StackFrameList::GetSelectedFrameIndex() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  return m_selected_frame_idx;
+  if (!m_selected_frame_idx)
+    SelectMostRelevantFrame();
+  if (!m_selected_frame_idx)
+    m_selected_frame_idx = 0;
+  return *m_selected_frame_idx;
 }
 
 uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   const_iterator pos;
   const_iterator begin = m_frames.begin();
   const_iterator end = m_frames.end();
   m_selected_frame_idx = 0;
+
   for (pos = begin; pos != end; ++pos) {
     if (pos->get() == frame) {
       m_selected_frame_idx = std::distance(begin, pos);
       uint32_t inlined_depth = GetCurrentInlinedDepth();
       if (inlined_depth != UINT32_MAX)
-        m_selected_frame_idx -= inlined_depth;
+        m_selected_frame_idx = *m_selected_frame_idx - inlined_depth;
       break;
     }
   }
+
   SetDefaultFileAndLineToSelectedFrame();
-  return m_selected_frame_idx;
+  return *m_selected_frame_idx;
 }
 
 bool StackFrameList::SetSelectedFrameByIndex(uint32_t idx) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   StackFrameSP frame_sp(GetFrameAtIndex(idx));
   if (frame_sp) {
     SetSelectedFrame(frame_sp.get());
     return true;
if (frame_sp) {
         m_thread.CalculateTarget()->GetSourceManager().SetDefaultFileAndLine(
             sc.line_entry.file, sc.line_entry.line);
     }
   }
 }
 
 // The thread has been run, reset the number stack frames to zero so we can
 // determine how many frames we have lazily.
+// Note, we don't actually re-use StackFrameLists, we always make a new
+// StackFrameList every time we stop, and then copy frame information frame
+// by frame from the old to the new StackFrameList.  So the comment above,
+// does not describe how StackFrameLists are currently used.
+// Clear is currently only used to clear the list in the destructor.
 void StackFrameList::Clear() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   m_frames.clear();
   m_concrete_frames_fetched = 0;
+  m_selected_frame_idx.reset();
 }
 
 lldb::StackFrameSP
 StackFrameList::GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr) {
   const_iterator pos;
   const_iterator begin = m_frames.begin();
   const_iterator end = m_frames.end();
   lldb::StackFrameSP ret_sp;

lldb/source/Target/Thread.cpp

if (stop_info_sp && stop_info_sp->IsValid()) {
     raw_stop_description = stop_info_sp->GetDescription();
     assert((!raw_stop_description.empty() ||
             stop_info_sp->GetStopReason() == eStopReasonNone) &&
            "StopInfo returned an empty description.");
   }
   return raw_stop_description;
 }
 
-void Thread::SelectMostRelevantFrame() {
-  Log *log = GetLog(LLDBLog::Thread);
-
-  auto frames_list_sp = GetStackFrameList();
-
-  // Only the top frame should be recognized.
-  auto frame_sp = frames_list_sp->GetFrameAtIndex(0);
-
-  auto recognized_frame_sp = frame_sp->GetRecognizedFrame();
-
-  if (!recognized_frame_sp) {
-    LLDB_LOG(log, "Frame #0 not recognized");
-    return;
-  }
-
-  if (StackFrameSP most_relevant_frame_sp =
-          recognized_frame_sp->GetMostRelevantFrame()) {
-    LLDB_LOG(log, "Found most relevant frame at index {0}",
-             most_relevant_frame_sp->GetFrameIndex());
-    SetSelectedFrame(most_relevant_frame_sp.get());
-  } else {
-    LLDB_LOG(log, "No relevant frame!");
-  }
-}
-
 void Thread::WillStop() {
   ThreadPlan *current_plan = GetCurrentPlan();
 
-  SelectMostRelevantFrame();
-
   // FIXME: I may decide to disallow threads with no plans.  In which
   // case this should go to an assert.
 
   if (!current_plan)
     return;
 
   current_plan->WillStop();
 }