[lldb] Always compute the execution & symbol context

[JDevlieghere]

Always compute the execution and symbol context, regardless of whether the statusline is enabled. This code gets called from the default event handler thread and has uncovered threading issues that otherwise only reproduce when the statusline is enabled.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Always compute the execution and symbol context, regardless of whether the statusline is enabled. This code gets called from the default event handler thread and has uncovered threading issues that otherwise only reproduce when the statusline is enabled.

---

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

3 Files Affected:

• (modified) lldb/include/lldb/Core/Statusline.h (+3-3)
• (modified) lldb/source/Core/Debugger.cpp (+29-1)
• (modified) lldb/source/Core/Statusline.cpp (+7-24)

diff --git a/lldb/include/lldb/Core/Statusline.h b/lldb/include/lldb/Core/Statusline.h
index 6bda153f822d2..13866a9083c63 100644
--- a/lldb/include/lldb/Core/Statusline.h
+++ b/lldb/include/lldb/Core/Statusline.h
@@ -25,9 +25,9 @@ class Statusline {
   /// 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. If both exe_ctx and sym_ctx are NULL, this redraws
+  /// the last string.
+  void Redraw(const ExecutionContext *exe_ctx, const SymbolContext *sym_ctx);
 
   /// Inform the statusline that the terminal dimensions have changed.
   void TerminalSizeChanged();
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index ed674ee1275c7..1bf4dbd0e9691 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1216,8 +1216,36 @@ void Debugger::RestoreInputTerminalState() {
 
 void Debugger::RedrawStatusline(bool update) {
   std::lock_guard<std::mutex> guard(m_statusline_mutex);
+
+  if (!update && m_statusline) {
+    m_statusline->Redraw(nullptr, nullptr);
+    return;
+  }
+
+  // Always compute the execution and symbol context, regardless of whether the
+  // statusline is enabled. This code gets called from the default event handler
+  // thread and has uncovered threading issues that otherwise only reproduce
+  // when the statusline is enabled.
+  ExecutionContext exe_ctx = 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(&GetSelectedOrDummyTarget());
+
+  SymbolContext sym_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())
+        sym_ctx = frame_sp->GetSymbolContext(eSymbolContextEverything);
+    }
+  }
+
   if (m_statusline)
-    m_statusline->Redraw(update);
+    m_statusline->Redraw(&exe_ctx, &sym_ctx);
 }
 
 ExecutionContext Debugger::GetSelectedExecutionContext() {
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index 393d427241021..08374abd94aa8 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -48,15 +48,15 @@ void Statusline::TerminalSizeChanged() {
   UpdateScrollWindow(ResizeStatusline);
 
   // Draw the old statusline.
-  Redraw(/*update=*/false);
+  Redraw(nullptr, nullptr);
 }
 
 void Statusline::Enable() {
   // Reduce the scroll window to make space for the status bar below.
   UpdateScrollWindow(EnableStatusline);
 
-  // Draw the statusline.
-  Redraw(/*update=*/true);
+  // Draw the old statusline.
+  Redraw(nullptr, nullptr);
 }
 
 void Statusline::Disable() {
@@ -127,33 +127,16 @@ void Statusline::UpdateScrollWindow(ScrollWindowMode mode) {
   m_debugger.RefreshIOHandler();
 }
 
-void Statusline::Redraw(bool update) {
-  if (!update) {
+void Statusline::Redraw(const ExecutionContext *exe_ctx,
+                        const SymbolContext *sym_ctx) {
+  if (!exe_ctx && !sym_ctx) {
     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);
-    }
-  }
-
   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]

This code seems fine to me. I was imagining that we would only always do this work when running in the TestSuite, so maybe controlled by a setting? OTOH, this is only on public stops and someone is bound to have computed the stop Symbol & Execution contexts so we aren't adding that much work, just doing it at the proper slightly unpredictable times. So this simpler version is fine.

[JDevlieghere]

This code seems fine to me. I was imagining that we would only always do this work when running in the TestSuite, so maybe controlled by a setting? OTOH, this is only on public stops and someone is bound to have computed the stop Symbol & Execution contexts so we aren't adding that much work, just doing it at the proper slightly unpredictable times. So this simpler version is fine.

Another potential alternative is to only do this unconditionally when asserts are enabled, but I came to the same conclusion that it's likely simple enough to do it everywhere, as well as eliminate the same variance in release builds.

[jimingham]

Except for the quibble that "current" is ambiguous, and it's better to use the explicit "currently selected" this looks fine.

[jimingham]

LGTM

[JDevlieghere]

As intended, this is uncovering a non-deterministic issue in LLDB when running fork-follow-child-softbp.test. I'm able to reproduce it locally (on Linux) but I haven't figured out the cause yet.

[jimingham]

As intended, this is uncovering a non-deterministic issue in LLDB when running fork-follow-child-softbp.test. I'm able to reproduce it locally (on Linux) but I haven't figured out the cause yet.

It's actually pretty encouraging that we only saw one test failure from this change!

[jimingham]

Only getting a new symbol & execution context if the process is stopped seems right.
You might have GetSelectedContext also return a "was-updated" or something. I can imagine, for instance, that I would want the status line to be slightly greyed out if you couldn't fetch the state when you went to update - to indicate that it is not current. So that would be a useful bit of information.

Other than that, this looks good.