[lldb] Rework how we pass the execution context to the statusline

[JDevlieghere]

Currently, we always pass the "selected" execution context to the statusline. When handling a process or thread event, we can be more precise, and build an execution context from the event data. This PR also adopts the new StoppedExecutionContext that was recently introduced.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Currently, we always pass the "selected" execution context to the statusline. When handling a process or thread event, we can be more precise, and build an execution context from the event data. This PR also adopts the new StoppedExecutionContext that was recently introduced.

---

Full diff: https://github.com/llvm/llvm-project/pull/159887.diff

5 Files Affected:

• (modified) lldb/include/lldb/Core/Debugger.h (+10-3)
• (modified) lldb/include/lldb/Core/Statusline.h (+12-5)
• (modified) lldb/source/Core/Debugger.cpp (+37-19)
• (modified) lldb/source/Core/IOHandler.cpp (+1-1)
• (modified) lldb/source/Core/Statusline.cpp (+25-32)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index 250ad64b76d9a..5fe96f759607b 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -181,7 +181,14 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
     return m_target_list.GetSelectedTarget();
   }
 
+  /// Get the execution context for the selected target.
   ExecutionContext GetSelectedExecutionContext();
+
+  /// Similar to GetSelectedExecutionContext but returns a
+  /// ExecutionContextRef, and will hold the dummy target if no target is
+  /// currently selected.
+  ExecutionContextRef GetSelectedExecutionContextRef();
+
   /// Get accessor for the target list.
   ///
   /// The target list is part of the global debugger object. This the single
@@ -419,7 +426,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   void CancelInterruptRequest();
 
   /// Redraw the statusline if enabled.
-  void RedrawStatusline(bool update = true);
+  void RedrawStatusline(std::optional<ExecutionContextRef> exe_ctx_ref);
 
   /// This is the correct way to query the state of Interruption.
   /// If you are on the RunCommandInterpreter thread, it will check the
@@ -701,9 +708,9 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
 
   void HandleBreakpointEvent(const lldb::EventSP &event_sp);
 
-  void HandleProcessEvent(const lldb::EventSP &event_sp);
+  lldb::ProcessSP HandleProcessEvent(const lldb::EventSP &event_sp);
 
-  void HandleThreadEvent(const lldb::EventSP &event_sp);
+  lldb::ThreadSP HandleThreadEvent(const lldb::EventSP &event_sp);
 
   void HandleProgressEvent(const lldb::EventSP &event_sp);
 
diff --git a/lldb/include/lldb/Core/Statusline.h b/lldb/include/lldb/Core/Statusline.h
index 6bda153f822d2..a5ab1927b57f5 100644
--- a/lldb/include/lldb/Core/Statusline.h
+++ b/lldb/include/lldb/Core/Statusline.h
@@ -9,6 +9,8 @@
 #ifndef LLDB_CORE_STATUSLINE_H
 #define LLDB_CORE_STATUSLINE_H
 
+#include "lldb/Symbol/SymbolContext.h"
+#include "lldb/Target/ExecutionContext.h"
 #include "lldb/lldb-forward.h"
 #include <cstdint>
 #include <string>
@@ -19,15 +21,16 @@ class Statusline {
   Statusline(Debugger &debugger);
   ~Statusline();
 
+  using Context = std::pair<ExecutionContextRef, SymbolContext>;
+
   /// Reduce the scroll window and draw the statusline.
-  void Enable();
+  void Enable(std::optional<ExecutionContextRef> exe_ctx_ref);
 
   /// Hide the statusline and extend the scroll window.
   void Disable();
 
-  /// Redraw the statusline. If update is false, this will redraw the last
-  /// string.
-  void Redraw(bool update = true);
+  /// Redraw the statusline.
+  void Redraw(std::optional<ExecutionContextRef> exe_ctx_ref);
 
   /// Inform the statusline that the terminal dimensions have changed.
   void TerminalSizeChanged();
@@ -46,7 +49,11 @@ class Statusline {
   void UpdateScrollWindow(ScrollWindowMode mode);
 
   Debugger &m_debugger;
-  std::string m_last_str;
+
+  /// Cached copy of the execution context that allows us to redraw the
+  /// statusline.
+  ExecutionContextRef m_exe_ctx_ref;
+
   uint64_t m_terminal_width = 0;
   uint64_t m_terminal_height = 0;
 };
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ed674ee1275c7..c761d39d40030 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -253,16 +253,18 @@ Status Debugger::SetPropertyValue(const ExecutionContext *exe_ctx,
       // Statusline setting changed. If we have a statusline instance, update it
       // now. Otherwise it will get created in the default event handler.
       std::lock_guard<std::mutex> guard(m_statusline_mutex);
-      if (StatuslineSupported())
+      if (StatuslineSupported()) {
         m_statusline.emplace(*this);
-      else
+        m_statusline->Enable(GetSelectedExecutionContextRef());
+      } else {
         m_statusline.reset();
+      }
     } else if (property_path ==
                    g_debugger_properties[ePropertyStatuslineFormat].name ||
                property_path ==
                    g_debugger_properties[ePropertySeparator].name) {
       // Statusline format changed. Redraw the statusline.
-      RedrawStatusline();
+      RedrawStatusline(std::nullopt);
     } else if (property_path ==
                g_debugger_properties[ePropertyUseSourceCache].name) {
       // use-source-cache changed. Wipe out the cache contents if it was
@@ -501,7 +503,7 @@ FormatEntity::Entry Debugger::GetStatuslineFormat() const {
 bool Debugger::SetStatuslineFormat(const FormatEntity::Entry &format) {
   constexpr uint32_t idx = ePropertyStatuslineFormat;
   bool ret = SetPropertyAtIndex(idx, format);
-  RedrawStatusline();
+  RedrawStatusline(std::nullopt);
   return ret;
 }
 
@@ -526,7 +528,7 @@ llvm::StringRef Debugger::GetDisabledAnsiSuffix() const {
 bool Debugger::SetSeparator(llvm::StringRef s) {
   constexpr uint32_t idx = ePropertySeparator;
   bool ret = SetPropertyAtIndex(idx, s);
-  RedrawStatusline();
+  RedrawStatusline(std::nullopt);
   return ret;
 }
 
@@ -1210,14 +1212,18 @@ void Debugger::RestoreInputTerminalState() {
   {
     std::lock_guard<std::mutex> guard(m_statusline_mutex);
     if (m_statusline)
-      m_statusline->Enable();
+      m_statusline->Enable(GetSelectedExecutionContext());
   }
 }
 
-void Debugger::RedrawStatusline(bool update) {
+void Debugger::RedrawStatusline(
+    std::optional<ExecutionContextRef> exe_ctx_ref) {
   std::lock_guard<std::mutex> guard(m_statusline_mutex);
-  if (m_statusline)
-    m_statusline->Redraw(update);
+
+  if (!m_statusline)
+    return;
+
+  m_statusline->Redraw(GetSelectedExecutionContextRef());
 }
 
 ExecutionContext Debugger::GetSelectedExecutionContext() {
@@ -1226,6 +1232,13 @@ ExecutionContext Debugger::GetSelectedExecutionContext() {
   return ExecutionContext(exe_ctx_ref);
 }
 
+ExecutionContextRef Debugger::GetSelectedExecutionContextRef() {
+  if (TargetSP selected_target_sp = GetSelectedTarget())
+    return ExecutionContextRef(selected_target_sp.get(),
+                               /*adopt_selected=*/true);
+  return ExecutionContextRef(m_dummy_target_sp.get(), /*adopt_selected=*/false);
+}
+
 void Debugger::DispatchInputInterrupt() {
   std::lock_guard<std::recursive_mutex> guard(m_io_handler_stack.GetMutex());
   IOHandlerSP reader_sp(m_io_handler_stack.Top());
@@ -1941,8 +1954,7 @@ void Debugger::FlushProcessOutput(Process &process, bool flush_stdout,
 }
 
 // This function handles events that were broadcast by the process.
-void Debugger::HandleProcessEvent(const EventSP &event_sp) {
-  using namespace lldb;
+ProcessSP Debugger::HandleProcessEvent(const EventSP &event_sp) {
   const uint32_t event_type = event_sp->GetType();
   ProcessSP process_sp =
       (event_type == Process::eBroadcastBitStructuredData)
@@ -2024,23 +2036,24 @@ void Debugger::HandleProcessEvent(const EventSP &event_sp) {
     if (pop_process_io_handler)
       process_sp->PopProcessIOHandler();
   }
+  return process_sp;
 }
 
-void Debugger::HandleThreadEvent(const EventSP &event_sp) {
+ThreadSP Debugger::HandleThreadEvent(const EventSP &event_sp) {
   // At present the only thread event we handle is the Frame Changed event, and
   // all we do for that is just reprint the thread status for that thread.
-  using namespace lldb;
   const uint32_t event_type = event_sp->GetType();
   const bool stop_format = true;
+  ThreadSP thread_sp;
   if (event_type == Thread::eBroadcastBitStackChanged ||
       event_type == Thread::eBroadcastBitThreadSelected) {
-    ThreadSP thread_sp(
-        Thread::ThreadEventData::GetThreadFromEvent(event_sp.get()));
+    thread_sp = Thread::ThreadEventData::GetThreadFromEvent(event_sp.get());
     if (thread_sp) {
       thread_sp->GetStatus(*GetAsyncOutputStream(), 0, 1, 1, stop_format,
                            /*show_hidden*/ true);
     }
   }
+  return thread_sp;
 }
 
 bool Debugger::IsForwardingEvents() { return (bool)m_forward_listener_sp; }
@@ -2109,28 +2122,33 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
 
   if (StatuslineSupported()) {
     std::lock_guard<std::mutex> guard(m_statusline_mutex);
-    if (!m_statusline)
+    if (!m_statusline) {
       m_statusline.emplace(*this);
+      m_statusline->Enable(GetSelectedExecutionContextRef());
+    }
   }
 
   bool done = false;
   while (!done) {
     EventSP event_sp;
     if (listener_sp->GetEvent(event_sp, std::nullopt)) {
+      std::optional<ExecutionContextRef> exe_ctx_ref = std::nullopt;
       if (event_sp) {
         Broadcaster *broadcaster = event_sp->GetBroadcaster();
         if (broadcaster) {
           uint32_t event_type = event_sp->GetType();
           ConstString broadcaster_class(broadcaster->GetBroadcasterClass());
           if (broadcaster_class == broadcaster_class_process) {
-            HandleProcessEvent(event_sp);
+            if (ProcessSP process_sp = HandleProcessEvent(event_sp))
+              exe_ctx_ref = ExecutionContext(process_sp);
           } else if (broadcaster_class == broadcaster_class_target) {
             if (Breakpoint::BreakpointEventData::GetEventDataFromEvent(
                     event_sp.get())) {
               HandleBreakpointEvent(event_sp);
             }
           } else if (broadcaster_class == broadcaster_class_thread) {
-            HandleThreadEvent(event_sp);
+            if (ThreadSP thread_sp = HandleThreadEvent(event_sp))
+              exe_ctx_ref = ExecutionContext(thread_sp);
           } else if (broadcaster == m_command_interpreter_up.get()) {
             if (event_type &
                 CommandInterpreter::eBroadcastBitQuitCommandReceived) {
@@ -2168,7 +2186,7 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
         if (m_forward_listener_sp)
           m_forward_listener_sp->AddEvent(event_sp);
       }
-      RedrawStatusline();
+      RedrawStatusline(exe_ctx_ref);
     }
   }
 
diff --git a/lldb/source/Core/IOHandler.cpp b/lldb/source/Core/IOHandler.cpp
index f65a1113f3592..57819eeade6e8 100644
--- a/lldb/source/Core/IOHandler.cpp
+++ b/lldb/source/Core/IOHandler.cpp
@@ -442,7 +442,7 @@ void IOHandlerEditline::AutoCompleteCallback(CompletionRequest &request) {
 }
 
 void IOHandlerEditline::RedrawCallback() {
-  m_debugger.RedrawStatusline(/*update=*/false);
+  m_debugger.RedrawStatusline(std::nullopt);
 }
 
 #endif
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index 393d427241021..d765bf852922c 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -35,9 +35,7 @@ using namespace lldb_private;
 
 Statusline::Statusline(Debugger &debugger)
     : m_debugger(debugger), m_terminal_width(m_debugger.GetTerminalWidth()),
-      m_terminal_height(m_debugger.GetTerminalHeight()) {
-  Enable();
-}
+      m_terminal_height(m_debugger.GetTerminalHeight()) {}
 
 Statusline::~Statusline() { Disable(); }
 
@@ -47,16 +45,16 @@ void Statusline::TerminalSizeChanged() {
 
   UpdateScrollWindow(ResizeStatusline);
 
-  // Draw the old statusline.
-  Redraw(/*update=*/false);
+  // Redraw the old statusline.
+  Redraw(std::nullopt);
 }
 
-void Statusline::Enable() {
+void Statusline::Enable(std::optional<ExecutionContextRef> exe_ctx_ref) {
   // Reduce the scroll window to make space for the status bar below.
   UpdateScrollWindow(EnableStatusline);
 
   // Draw the statusline.
-  Redraw(/*update=*/true);
+  Redraw(exe_ctx_ref);
 }
 
 void Statusline::Disable() {
@@ -69,8 +67,6 @@ void Statusline::Draw(std::string str) {
   if (!stream_sp)
     return;
 
-  m_last_str = str;
-
   str = ansi::TrimAndPad(str, m_terminal_width);
 
   LockedStreamFile locked_stream = stream_sp->Lock();
@@ -127,33 +123,30 @@ void Statusline::UpdateScrollWindow(ScrollWindowMode mode) {
   m_debugger.RefreshIOHandler();
 }
 
-void Statusline::Redraw(bool update) {
-  if (!update) {
-    Draw(m_last_str);
-    return;
-  }
-
-  ExecutionContext exe_ctx = m_debugger.GetSelectedExecutionContext();
-
-  // For colors and progress events, the format entity needs access to the
-  // debugger, which requires a target in the execution context.
-  if (!exe_ctx.HasTargetScope())
-    exe_ctx.SetTargetPtr(&m_debugger.GetSelectedOrDummyTarget());
-
-  SymbolContext symbol_ctx;
-  if (ProcessSP process_sp = exe_ctx.GetProcessSP()) {
-    // Check if the process is stopped, and if it is, make sure it remains
-    // stopped until we've computed the symbol context.
-    Process::StopLocker stop_locker;
-    if (stop_locker.TryLock(&process_sp->GetRunLock())) {
-      if (auto frame_sp = exe_ctx.GetFrameSP())
-        symbol_ctx = frame_sp->GetSymbolContext(eSymbolContextEverything);
-    }
+void Statusline::Redraw(std::optional<ExecutionContextRef> exe_ctx_ref) {
+  // Update the cached execution context.
+  if (exe_ctx_ref)
+    m_exe_ctx_ref = *exe_ctx_ref;
+
+  // Lock the execution context.
+  ExecutionContext exe_ctx =
+      m_exe_ctx_ref.Lock(/*thread_and_frame_only_if_stopped=*/true);
+
+  // Compute the symbol context if we're stopped.
+  SymbolContext sym_ctx;
+  llvm::Expected<StoppedExecutionContext> stopped_exe_ctx =
+      GetStoppedExecutionContext(&m_exe_ctx_ref);
+  if (stopped_exe_ctx) {
+    if (auto frame_sp = stopped_exe_ctx->GetFrameSP())
+      sym_ctx = frame_sp->GetSymbolContext(eSymbolContextEverything);
+  } else {
+    // We can draw the statusline without being stopped.
+    llvm::consumeError(stopped_exe_ctx.takeError());
   }
 
   StreamString stream;
   FormatEntity::Entry format = m_debugger.GetStatuslineFormat();
-  FormatEntity::Format(format, stream, &symbol_ctx, &exe_ctx, nullptr, nullptr,
+  FormatEntity::Format(format, stream, &sym_ctx, &exe_ctx, nullptr, nullptr,
                        false, false);
 
   Draw(stream.GetString().str());

[jimingham]

You're being a little inconsistent here.

ExecutionContext(lldb::ProcessSP &) returns an ExecutionContext with only the target & process shared pointers set. In other places where you use GetSelectedExecutionContextRef, you're passing in all the selected entities. That makes status line code that might want to show thread info have to guess whether this means it shouldn't show anything about threads because the caller on purpose didn't send in a particular thread, or if it should get that process' selected thread.

I think it would be easier on the status line formatting code if we always put as much detail as we know. Since this is a process event, then we should provide the selected Thread & Frame. For Thread events below, we should fill in the selected frame for the thread. And if there were to be frame events (we don't have those yet) that would determine everything above it.

That way when you're in the status line code you always know what the caller means.

[JDevlieghere]

It's not only inconsistent, it's actually incorrect. I didn't notice because of the bug you spotted earlier where we weren't actually passing the computed execution context to the statusline. I originally misunderstood the meaning of ExecutionContext::Lokc(thread_and_frame_only_if_stopped). I thought the argument decided whether we computed the selected thread & frame, but it only decides whether to set it in the ExecutionContext, if they are present in the ExecutionContextRef.

Changing the Lock method could be another way to solve this. I fixed it the way you suggested, by adding overloads to the constructor for the Process & Thread that take an adopt_selected argument.

[jimingham]

I kinda want an assert here if this isn't true. How could a StoppedExecutionContext not have a frame?

[JDevlieghere]

A little counter-intuitively, the StoppedExecutionContext only ensures that we hold the run lock, so the process could be in an exited or unloaded state.

[jimingham]

Ah, makes sense. Might want to add that as a comment, since that's not 100% obvious.

[jimingham]

LGTM

If we can't resolve the follow-fork crash right away, you might could just not calculate the execution context for those process events with a big FIXME. This is doing something a little underhanded - usually we don't change the process main binary on a stop event and it looks like something is failing to get cleared.