[Lldb-commits] [lldb] 8311620 - [lldb] Fix lock inversion between statusline mutex and output mutex (#135956)

[via lldb-commits]

Author: Jonas Devlieghere
Date: 2025-04-17T17:57:00+02:00
New Revision: 83116209331ad6a5a45f1b8735ad5bce9e6ef761

URL: https://github.com/llvm/llvm-project/commit/83116209331ad6a5a45f1b8735ad5bce9e6ef761
DIFF: https://github.com/llvm/llvm-project/commit/83116209331ad6a5a45f1b8735ad5bce9e6ef761.diff

LOG: [lldb] Fix lock inversion between statusline mutex and output mutex (#135956)

Fix a deadlock between the statusline mutex (in Debugger) and the output
file mutex (in LockedStreamFile). The deadlock occurs when the main
thread is calling the statusline callback while holding the output mutex
in Editline, while the default event thread is trying to update the
statusline.

Extend the uncritical section so we can redraw the statusline there.
The loop in Editline::GetCharacter should be unnecessary. It would only
loop if we had a successful read with length zero, which shouldn't be
possible or when we can't convert a partial UTF-8 character, in which
case we bail out.

rdar://149251156

Added: 
    

Modified: 
    lldb/source/Host/common/Editline.cpp
    lldb/test/API/functionalities/statusline/TestStatusline.py

Removed: 
    


################################################################################
diff  --git a/lldb/source/Host/common/Editline.cpp b/lldb/source/Host/common/Editline.cpp
index 29abaf7c65f28..ff71cd0cdb241 100644
--- a/lldb/source/Host/common/Editline.cpp
+++ b/lldb/source/Host/common/Editline.cpp
@@ -567,9 +567,6 @@ int Editline::GetCharacter(EditLineGetCharType *c) {
     m_needs_prompt_repaint = false;
   }
 
-  if (m_redraw_callback)
-    m_redraw_callback();
-
   if (m_multiline_enabled) {
     // Detect when the number of rows used for this input line changes due to
     // an edit
@@ -585,54 +582,56 @@ int Editline::GetCharacter(EditLineGetCharType *c) {
     m_current_line_rows = new_line_rows;
   }
 
+  if (m_terminal_size_has_changed)
+    ApplyTerminalSizeChange();
+
+  // This mutex is locked by our caller (GetLine). Unlock it while we read a
+  // character (blocking operation), so we do not hold the mutex
+  // indefinitely. This gives a chance for someone to interrupt us. After
+  // Read returns, immediately lock the mutex again and check if we were
+  // interrupted.
+  m_locked_output.reset();
+
+  if (m_redraw_callback)
+    m_redraw_callback();
+
   // Read an actual character
-  while (true) {
-    lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess;
-    char ch = 0;
-
-    if (m_terminal_size_has_changed)
-      ApplyTerminalSizeChange();
-
-    // This mutex is locked by our caller (GetLine). Unlock it while we read a
-    // character (blocking operation), so we do not hold the mutex
-    // indefinitely. This gives a chance for someone to interrupt us. After
-    // Read returns, immediately lock the mutex again and check if we were
-    // interrupted.
-    m_locked_output.reset();
-    int read_count =
-        m_input_connection.Read(&ch, 1, std::nullopt, status, nullptr);
-    m_locked_output.emplace(m_output_stream_sp->Lock());
-    if (m_editor_status == EditorStatus::Interrupted) {
-      while (read_count > 0 && status == lldb::eConnectionStatusSuccess)
-        read_count =
-            m_input_connection.Read(&ch, 1, std::nullopt, status, nullptr);
-      lldbassert(status == lldb::eConnectionStatusInterrupted);
-      return 0;
-    }
+  lldb::ConnectionStatus status = lldb::eConnectionStatusSuccess;
+  char ch = 0;
+  int read_count =
+      m_input_connection.Read(&ch, 1, std::nullopt, status, nullptr);
+
+  // Re-lock the output mutex to protected m_editor_status here and in the
+  // switch below.
+  m_locked_output.emplace(m_output_stream_sp->Lock());
+  if (m_editor_status == EditorStatus::Interrupted) {
+    while (read_count > 0 && status == lldb::eConnectionStatusSuccess)
+      read_count =
+          m_input_connection.Read(&ch, 1, std::nullopt, status, nullptr);
+    lldbassert(status == lldb::eConnectionStatusInterrupted);
+    return 0;
+  }
 
-    if (read_count) {
-      if (CompleteCharacter(ch, *c))
-        return 1;
-    } else {
-      switch (status) {
-      case lldb::eConnectionStatusSuccess: // Success
-        break;
+  if (read_count) {
+    if (CompleteCharacter(ch, *c))
+      return 1;
+    return 0;
+  }
 
-      case lldb::eConnectionStatusInterrupted:
-        llvm_unreachable("Interrupts should have been handled above.");
-
-      case lldb::eConnectionStatusError:        // Check GetError() for details
-      case lldb::eConnectionStatusTimedOut:     // Request timed out
-      case lldb::eConnectionStatusEndOfFile:    // End-of-file encountered
-      case lldb::eConnectionStatusNoConnection: // No connection
-      case lldb::eConnectionStatusLostConnection: // Lost connection while
-                                                  // connected to a valid
-                                                  // connection
-        m_editor_status = EditorStatus::EndOfInput;
-        return 0;
-      }
-    }
+  switch (status) {
+  case lldb::eConnectionStatusSuccess:
+    llvm_unreachable("Success should have resulted in positive read_count.");
+  case lldb::eConnectionStatusInterrupted:
+    llvm_unreachable("Interrupts should have been handled above.");
+  case lldb::eConnectionStatusError:
+  case lldb::eConnectionStatusTimedOut:
+  case lldb::eConnectionStatusEndOfFile:
+  case lldb::eConnectionStatusNoConnection:
+  case lldb::eConnectionStatusLostConnection:
+    m_editor_status = EditorStatus::EndOfInput;
   }
+
+  return 0;
 }
 
 const char *Editline::Prompt() {

diff  --git a/lldb/test/API/functionalities/statusline/TestStatusline.py b/lldb/test/API/functionalities/statusline/TestStatusline.py
index 747a7a14e0629..489c0371b11d8 100644
--- a/lldb/test/API/functionalities/statusline/TestStatusline.py
+++ b/lldb/test/API/functionalities/statusline/TestStatusline.py
@@ -77,3 +77,20 @@ def test_no_color(self):
                 "\x1b[7m",
             ],
         )
+
+    def test_deadlock(self):
+        """Regression test for lock inversion between the statusline mutex and
+        the output mutex."""
+        self.build()
+        self.launch(extra_args=["-o", "settings set use-color false"])
+        self.child.expect("(lldb)")
+
+        # Change the terminal dimensions.
+        terminal_height = 10
+        terminal_width = 60
+        self.child.setwinsize(terminal_height, terminal_width)
+
+        exe = self.getBuildArtifact("a.out")
+
+        self.expect("file {}".format(exe), ["Current executable"])
+        self.expect("help", ["Debugger commands"])