Merge pull request #6623 from apple/🍒/5.9/730c8e160c9d

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

Author: JDevlieghere

lldb/include/lldb/Target/StackFrameList.h

@@ -46,7 +46,7 @@ class StackFrameList {
   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 @@ class StackFrameList {
 
   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 @@ class StackFrameList {
   /// 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.

lldb/include/lldb/Target/Thread.h

@@ -217,8 +217,6 @@ class Thread : public std::enable_shared_from_this<Thread>,
 
   virtual void RefreshStateAfterStop() = 0;
 
-  void SelectMostRelevantFrame();
-
   std::string GetStopDescription();
 
   std::string GetStopDescriptionRaw();

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,
@@ -776,9 +777,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) {
@@ -787,17 +825,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) {
@@ -825,10 +865,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

lldb/source/Target/Thread.cpp

@@ -605,36 +605,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.