[lldb] Synchronize access to m_statusline in the Debugger

[JDevlieghere]

Eliminate the potential for a race between the main thread, the default event handler thread and the signal handling thread, when accessing the m_statusline member.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Eliminate a potential race between the main thread and the default event handler thread when accessing the m_statusline member. Some methods could be called from a signal handler and are "allowed" to fail locking.

---

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

2 Files Affected:

• (modified) lldb/include/lldb/Core/Debugger.h (+1)
• (modified) lldb/source/Core/Debugger.cpp (+38-12)

diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h
index c79a75ab61564..478942960d636 100644
--- a/lldb/include/lldb/Core/Debugger.h
+++ b/lldb/include/lldb/Core/Debugger.h
@@ -751,6 +751,7 @@ class Debugger : public std::enable_shared_from_this<Debugger>,
   IOHandlerStack m_io_handler_stack;
   std::recursive_mutex m_io_handler_synchronous_mutex;
 
+  std::mutex m_statusline_mutex;
   std::optional<Statusline> m_statusline;
 
   llvm::StringMap<std::weak_ptr<LogHandler>> m_stream_handlers;
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 51029f91eb12d..2c15447b2a6bb 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -251,6 +251,7 @@ Status Debugger::SetPropertyValue(const ExecutionContext *exe_ctx,
                g_debugger_properties[ePropertyShowStatusline].name) {
       // 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())
         m_statusline.emplace(*this);
       else
@@ -391,8 +392,13 @@ bool Debugger::SetTerminalWidth(uint64_t term_width) {
 
   if (auto handler_sp = m_io_handler_stack.Top())
     handler_sp->TerminalSizeChanged();
-  if (m_statusline)
-    m_statusline->TerminalSizeChanged();
+
+  {
+    // This might get called from a signal handler.
+    std::unique_lock<std::mutex> lock(m_statusline_mutex, std::try_to_lock);
+    if (m_statusline)
+      m_statusline->TerminalSizeChanged();
+  }
 
   return success;
 }
@@ -409,8 +415,13 @@ bool Debugger::SetTerminalHeight(uint64_t term_height) {
 
   if (auto handler_sp = m_io_handler_stack.Top())
     handler_sp->TerminalSizeChanged();
-  if (m_statusline)
-    m_statusline->TerminalSizeChanged();
+
+  {
+    // This might get called from a signal handler.
+    std::unique_lock<std::mutex> lock(m_statusline_mutex, std::try_to_lock);
+    if (m_statusline)
+      m_statusline->TerminalSizeChanged();
+  }
 
   return success;
 }
@@ -1141,8 +1152,12 @@ void Debugger::SetErrorFile(FileSP file_sp) {
 }
 
 void Debugger::SaveInputTerminalState() {
-  if (m_statusline)
-    m_statusline->Disable();
+  {
+    // This might get called from a signal handler.
+    std::unique_lock<std::mutex> lock(m_statusline_mutex, std::try_to_lock);
+    if (m_statusline)
+      m_statusline->Disable();
+  }
   int fd = GetInputFile().GetDescriptor();
   if (fd != File::kInvalidDescriptor)
     m_terminal_state.Save(fd, true);
@@ -1150,11 +1165,16 @@ void Debugger::SaveInputTerminalState() {
 
 void Debugger::RestoreInputTerminalState() {
   m_terminal_state.Restore();
-  if (m_statusline)
-    m_statusline->Enable();
+  {
+    // This might get called from a signal handler.
+    std::unique_lock<std::mutex> lock(m_statusline_mutex, std::try_to_lock);
+    if (m_statusline)
+      m_statusline->Enable();
+  }
 }
 
 void Debugger::RedrawStatusline(bool update) {
+  std::lock_guard<std::mutex> guard(m_statusline_mutex);
   if (m_statusline)
     m_statusline->Redraw(update);
 }
@@ -2032,8 +2052,11 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
   // are now listening to all required events so no events get missed
   m_sync_broadcaster.BroadcastEvent(eBroadcastBitEventThreadIsListening);
 
-  if (!m_statusline && StatuslineSupported())
-    m_statusline.emplace(*this);
+  if (StatuslineSupported()) {
+    std::lock_guard<std::mutex> guard(m_statusline_mutex);
+    if (!m_statusline)
+      m_statusline.emplace(*this);
+  }
 
   bool done = false;
   while (!done) {
@@ -2094,8 +2117,11 @@ lldb::thread_result_t Debugger::DefaultEventHandler() {
     }
   }
 
-  if (m_statusline)
-    m_statusline.reset();
+  {
+    std::lock_guard<std::mutex> guard(m_statusline_mutex);
+    if (m_statusline)
+      m_statusline.reset();
+  }
 
   return {};
 }

[JDevlieghere]

Rebased on top of #134956 which means we can now safely lock everywhere we access m_statusline.

[bulbazord]

LGTM! :)

[labath]

🚢