[lldb] Move "SelectMostRelevantFrame" from Thread::WillStop

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
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.

rdar://107643231

Differential revision: https://reviews.llvm.org/D147753

diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index e05a398..ae99853 100644 (file)
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -46,7 +46,7 @@ public:
   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.
@@ -110,6 +110,8 @@ protected:
 
   void SetCurrentInlinedDepth(uint32_t new_depth);
 
+  void SelectMostRelevantFrame();
+
   typedef std::vector<lldb::StackFrameSP> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
@@ -134,8 +136,10 @@ protected:
   /// 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.

diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index efe8272..882f001 100644 (file)
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -217,8 +217,6 @@ public:
 
   virtual void RefreshStateAfterStop() = 0;
 
-  void SelectMostRelevantFrame();
-
   std::string GetStopDescription();
 
   std::string GetStopDescriptionRaw();

diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index 2610b1c..3b0c66a 100644 (file)
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -18,6 +18,7 @@
 #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"
@@ -38,7 +39,7 @@ 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) {
@@ -252,7 +253,7 @@ struct CallDescriptor {
 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,
@@ -781,9 +782,46 @@ bool StackFrameList::SetFrameAtIndex(uint32_t idx, StackFrameSP &frame_sp) {
   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) {
@@ -792,17 +830,19 @@ uint32_t StackFrameList::SetSelectedFrame(lldb_private::StackFrame *frame) {
   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) {
@@ -830,10 +870,16 @@ void StackFrameList::SetDefaultFileAndLineToSelectedFrame() {
 
 // 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

diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index d620f74..ab8c115 100644 (file)
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -588,36 +588,9 @@ std::string Thread::GetStopDescriptionRaw() {
   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.