Make sure SelectMostRelevantFrame happens only when returning to the user.

This is a user facing action, it is meant to focus the user's attention on
something other than the 0th frame when you stop somewhere where that's
helpful. For instance, stopping in pthread_kill after an assert will select
the assert frame.

This is not something you want to have happen internally in lldb, both
because internally you really don't want the selected frame changing out
from under you, and because the recognizers can do arbitrary work, and that
can cause deadlocks or other unexpected behavior.

However, it's not something that the current code does
explicitly after a stop has been delivered, it's expected to happen implicitly
as part of stopping. I changing this to call SMRF explicitly after a user
stop, but that got pretty ugly quickly.

So I added a bool to control whether to run this and audited all the current
uses to determine whether we're returning to the user or not.

Differential Revision: https://reviews.llvm.org/D148863

(cherry picked from commit 076341d)

Author: jimingham

lldb/include/lldb/Target/Process.h

@@ -835,6 +835,7 @@ class Process : public std::enable_shared_from_this<Process>,
   /// \see Thread:Suspend()
   Status Resume();
 
+  /// Resume a process, and wait for it to stop.
   Status ResumeSynchronous(Stream *stream);
 
   /// Halts a running process.
@@ -2184,12 +2185,17 @@ class Process : public std::enable_shared_from_this<Process>,
   // process is hijacked and use_run_lock is true (the default), then this
   // function releases the run lock after the stop. Setting use_run_lock to
   // false will avoid this behavior.
+  // If we are waiting to stop that will return control to the user,
+  // then we also want to run SelectMostRelevantFrame, which is controlled
+  // by "select_most_relevant".
   lldb::StateType
   WaitForProcessToStop(const Timeout<std::micro> &timeout,
                        lldb::EventSP *event_sp_ptr = nullptr,
                        bool wait_always = true,
                        lldb::ListenerSP hijack_listener = lldb::ListenerSP(),
-                       Stream *stream = nullptr, bool use_run_lock = true);
+                       Stream *stream = nullptr, bool use_run_lock = true,
+                       SelectMostRelevant select_most_relevant =
+                           DoNoSelectMostRelevantFrame);
 
   uint32_t GetIOHandlerID() const { return m_iohandler_sync.GetValue(); }
 
@@ -2232,10 +2238,11 @@ class Process : public std::enable_shared_from_this<Process>,
   /// \return
   ///     \b true if the event describes a process state changed event, \b false
   ///     otherwise.
-  static bool HandleProcessStateChangedEvent(const lldb::EventSP &event_sp,
-                                             Stream *stream,
-                                             bool &pop_process_io_handler,
-                                             bool &pop_command_interpreter);
+  static bool
+  HandleProcessStateChangedEvent(const lldb::EventSP &event_sp, Stream *stream,
+                                 SelectMostRelevant select_most_relevant,
+                                 bool &pop_process_io_handler,
+                                 bool &pop_command_interpreter);
 
   Event *PeekAtStateChangedEvents();
 

lldb/include/lldb/Target/StackFrameList.h

@@ -46,7 +46,15 @@ class StackFrameList {
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
 
   /// Get the currently selected frame index.
-  uint32_t GetSelectedFrameIndex();
+  /// We should only call SelectMostRelevantFrame if (a) the user hasn't already
+  /// selected a frame, and (b) if this really is a user facing
+  /// "GetSelectedFrame".  SMRF runs the frame recognizers which can do
+  /// arbitrary work that ends up being dangerous to do internally.  Also,
+  /// for most internal uses we don't actually want the frame changed by the
+  /// SMRF logic.  So unless this is in a command or SB API, you should
+  /// pass false here.
+  uint32_t
+  GetSelectedFrameIndex(SelectMostRelevant select_most_relevant_frame);
 
   /// Mark a stack frame as the currently selected frame using the frame index
   /// \p idx. Like \ref GetFrameAtIndex, invisible frames cannot be selected.

lldb/include/lldb/Target/Thread.h

@@ -426,11 +426,16 @@ class Thread : public std::enable_shared_from_this<Thread>,
     return lldb::StackFrameSP();
   }
 
-  uint32_t GetSelectedFrameIndex() {
-    return GetStackFrameList()->GetSelectedFrameIndex();
+  // Only pass true to select_most_relevant if you are fulfilling an explicit
+  // user request for GetSelectedFrameIndex.  The most relevant frame is only
+  // for showing to the user, and can do arbitrary work, so we don't want to
+  // call it internally.
+  uint32_t GetSelectedFrameIndex(SelectMostRelevant select_most_relevant) {
+    return GetStackFrameList()->GetSelectedFrameIndex(select_most_relevant);
   }
 
-  lldb::StackFrameSP GetSelectedFrame();
+  lldb::StackFrameSP
+  GetSelectedFrame(SelectMostRelevant select_most_relevant);
 
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame,
                             bool broadcast = false);

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

@@ -288,4 +288,9 @@ template <> struct format_provider<lldb_private::Vote> {
 };
 }
 
+enum SelectMostRelevant : bool {
+  SelectMostRelevantFrame = true,
+  DoNoSelectMostRelevantFrame = false,
+};
+
 #endif // LLDB_LLDB_PRIVATE_ENUMERATIONS_H

lldb/source/API/SBThread.cpp

@@ -816,7 +816,11 @@ SBError SBThread::StepOverUntil(lldb::SBFrame &sb_frame,
     }
 
     if (!frame_sp) {
-      frame_sp = thread->GetSelectedFrame();
+      // We don't want to run SelectMostRelevantFrame here, for instance if
+      // you called a sequence of StepOverUntil's you wouldn't want the
+      // frame changed out from under you because you stepped into a
+      // recognized frame.
+      frame_sp = thread->GetSelectedFrame(DoNoSelectMostRelevantFrame);
       if (!frame_sp)
         frame_sp = thread->GetStackFrameAtIndex(0);
     }
@@ -1160,7 +1164,8 @@ lldb::SBFrame SBThread::GetSelectedFrame() {
   if (exe_ctx.HasThreadScope()) {
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&exe_ctx.GetProcessPtr()->GetRunLock())) {
-      frame_sp = exe_ctx.GetThreadPtr()->GetSelectedFrame();
+      frame_sp =
+          exe_ctx.GetThreadPtr()->GetSelectedFrame(SelectMostRelevantFrame);
       sb_frame.SetFrameSP(frame_sp);
     }
   }

lldb/source/Commands/CommandObjectFrame.cpp

@@ -134,7 +134,7 @@ class CommandObjectFrameDiagnose : public CommandObjectParsed {
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Thread *thread = m_exe_ctx.GetThreadPtr();
-    StackFrameSP frame_sp = thread->GetSelectedFrame();
+    StackFrameSP frame_sp = thread->GetSelectedFrame(SelectMostRelevantFrame);
 
     ValueObjectSP valobj_sp;
 
@@ -307,7 +307,7 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
     uint32_t frame_idx = UINT32_MAX;
     if (m_options.relative_frame_offset) {
       // The one and only argument is a signed relative frame index
-      frame_idx = thread->GetSelectedFrameIndex();
+      frame_idx = thread->GetSelectedFrameIndex(SelectMostRelevantFrame);
       if (frame_idx == UINT32_MAX)
         frame_idx = 0;
 
@@ -361,7 +361,7 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
           return false;
         }
       } else if (command.GetArgumentCount() == 0) {
-        frame_idx = thread->GetSelectedFrameIndex();
+        frame_idx = thread->GetSelectedFrameIndex(SelectMostRelevantFrame);
         if (frame_idx == UINT32_MAX) {
           frame_idx = 0;
         }
@@ -371,7 +371,7 @@ class CommandObjectFrameSelect : public CommandObjectParsed {
     bool success = thread->SetSelectedFrameByIndexNoisily(
         frame_idx, result.GetOutputStream());
     if (success) {
-      m_exe_ctx.SetFrameSP(thread->GetSelectedFrame());
+      m_exe_ctx.SetFrameSP(thread->GetSelectedFrame(SelectMostRelevantFrame));
       result.SetStatus(eReturnStatusSuccessFinishResult);
     } else {
       result.AppendErrorWithFormat("Frame index (%u) out of range.\n",

lldb/source/Commands/CommandObjectThread.cpp

@@ -555,8 +555,9 @@ class CommandObjectThreadStepWithTypeAndScope : public CommandObjectParsed {
     } else if (m_step_type == eStepTypeOut) {
       new_plan_sp = thread->QueueThreadPlanForStepOut(
           abort_other_plans, nullptr, false, bool_stop_other_threads, eVoteYes,
-          eVoteNoOpinion, thread->GetSelectedFrameIndex(), new_plan_status,
-          m_options.m_step_out_avoid_no_debug);
+          eVoteNoOpinion,
+          thread->GetSelectedFrameIndex(DoNoSelectMostRelevantFrame),
+          new_plan_status, m_options.m_step_out_avoid_no_debug);
     } else if (m_step_type == eStepTypeScripted) {
       new_plan_sp = thread->QueueThreadPlanForStepScripted(
           abort_other_plans, m_class_options.GetName().c_str(),
@@ -1526,7 +1527,8 @@ class CommandObjectThreadReturn : public CommandObjectRaw {
         bool success =
             thread->SetSelectedFrameByIndexNoisily(0, result.GetOutputStream());
         if (success) {
-          m_exe_ctx.SetFrameSP(thread->GetSelectedFrame());
+          m_exe_ctx.SetFrameSP(
+              thread->GetSelectedFrame(DoNoSelectMostRelevantFrame));
           result.SetStatus(eReturnStatusSuccessFinishResult);
         } else {
           result.AppendErrorWithFormat(

lldb/source/Commands/CommandObjectType.cpp

@@ -2804,7 +2804,8 @@ class CommandObjectFormatterInfo : public CommandObjectRaw {
       return false;
     }
 
-    StackFrameSP frame_sp = thread->GetSelectedFrame();
+    StackFrameSP frame_sp =
+        thread->GetSelectedFrame(DoNoSelectMostRelevantFrame);
     ValueObjectSP result_valobj_sp;
     EvaluateExpressionOptions options;
     lldb::ExpressionResults expr_result = target_sp->EvaluateExpression(

lldb/source/Core/Debugger.cpp

@@ -1665,6 +1665,7 @@ void Debugger::HandleProcessEvent(const EventSP &event_sp) {
     // Display running state changes first before any STDIO
     if (got_state_changed && !state_is_stopped) {
       Process::HandleProcessStateChangedEvent(event_sp, output_stream_sp.get(),
+                                              DoNoSelectMostRelevantFrame,
                                               pop_process_io_handler,
                                               // BEGIN SWIFT
                                               pop_command_interpreter
@@ -1708,6 +1709,7 @@ void Debugger::HandleProcessEvent(const EventSP &event_sp) {
     // Now display any stopped state changes after any STDIO
     if (got_state_changed && state_is_stopped) {
       Process::HandleProcessStateChangedEvent(event_sp, output_stream_sp.get(),
+                                              DoNoSelectMostRelevantFrame,
                                               pop_process_io_handler,
                                               // BEGIN SWIFT
                                               pop_command_interpreter

lldb/source/Core/IOHandlerCursesGUI.cpp

@@ -5258,7 +5258,8 @@ class ThreadsTreeDelegate : public TreeDelegate {
     for (size_t i = 0; i < num_threads; ++i) {
       ThreadSP thread = threads.GetThreadAtIndex(i);
       if (selected_thread->GetID() == thread->GetID()) {
-        selected_item = &root[i][thread->GetSelectedFrameIndex()];
+        selected_item =
+            &root[i][thread->GetSelectedFrameIndex(SelectMostRelevantFrame)];
         selection_index = selected_item->GetRowIndex();
         return;
       }
@@ -6410,7 +6411,8 @@ class ApplicationDelegate : public WindowDelegate, public MenuDelegate {
         if (process && process->IsAlive() &&
             StateIsStoppedState(process->GetState(), true)) {
           Thread *thread = exe_ctx.GetThreadPtr();
-          uint32_t frame_idx = thread->GetSelectedFrameIndex();
+          uint32_t frame_idx =
+              thread->GetSelectedFrameIndex(SelectMostRelevantFrame);
           exe_ctx.GetThreadRef().StepOut(frame_idx);
         }
       }
@@ -6826,7 +6828,7 @@ class SourceFileWindowDelegate : public WindowDelegate {
       if (process_alive) {
         thread = exe_ctx.GetThreadPtr();
         if (thread) {
-          frame_sp = thread->GetSelectedFrame();
+          frame_sp = thread->GetSelectedFrame(SelectMostRelevantFrame);
           auto tid = thread->GetID();
           thread_changed = tid != m_tid;
           m_tid = tid;
@@ -7373,7 +7375,8 @@ class SourceFileWindowDelegate : public WindowDelegate {
         if (exe_ctx.HasThreadScope() &&
             StateIsStoppedState(exe_ctx.GetProcessRef().GetState(), true)) {
           Thread *thread = exe_ctx.GetThreadPtr();
-          uint32_t frame_idx = thread->GetSelectedFrameIndex();
+          uint32_t frame_idx =
+              thread->GetSelectedFrameIndex(SelectMostRelevantFrame);
           exe_ctx.GetThreadRef().StepOut(frame_idx);
         }
       }
@@ -7412,15 +7415,16 @@ class SourceFileWindowDelegate : public WindowDelegate {
           m_debugger.GetCommandInterpreter().GetExecutionContext();
       if (exe_ctx.HasThreadScope()) {
         Thread *thread = exe_ctx.GetThreadPtr();
-        uint32_t frame_idx = thread->GetSelectedFrameIndex();
+        uint32_t frame_idx =
+            thread->GetSelectedFrameIndex(SelectMostRelevantFrame);
         if (frame_idx == UINT32_MAX)
           frame_idx = 0;
         if (c == 'u' && frame_idx + 1 < thread->GetStackFrameCount())
           ++frame_idx;
         else if (c == 'd' && frame_idx > 0)
           --frame_idx;
         if (thread->SetSelectedFrameByIndex(frame_idx, true))
-          exe_ctx.SetFrameSP(thread->GetSelectedFrame());
+          exe_ctx.SetFrameSP(thread->GetSelectedFrame(SelectMostRelevantFrame));
       }
     }
       return eKeyHandled;