[Lldb-commits] [lldb] Prevents a potential lock inversion in StatusLine when shutting down. (PR #179789)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: None (jimingham)

<details>
<summary>Changes</summary>

We have gotten reports of an occasional deadlock on shutdown.  The 
Driver::MainLoop thread is shutting down, and gets stuck here:

  Thread 0x3c017c
  start 
    main 
       Driver::MainLoop() 
         lldb::SBDebugger::RunCommandInterpreter(bool, bool) 
           
lldb_private::CommandInterpreter::RunCommandInterpreter(lldb_private::CommandInterpreterRunOptions&amp;)
            lldb_private::Debugger::RunIOHandlers()
              
lldb_private::Debugger::PopIOHandler(std::__1::shared_ptr&lt;lldb_private::IOHandler&gt;
 const&amp;)
                 lldb_private::Editline::Cancel()
                   lldb_private::LockableStreamFile::Lock()
                     std::__1::recursive_mutex::lock()

It has acquired the IO Handler list lock (in PopIOHandler) and is stuck trying 
to get the debugger output stream lock.

Meanwhile, the event-handler thread is doing:

  Thread 0x3c01d3
  thread_start
    _pthread_start
       lldb_private::HostThreadMacOSX::ThreadCreateTrampoline(void*) 
        lldb_private::HostNativeThreadBase::ThreadCreateTrampoline(void*)
          
std::__1::__function::__func&lt;lldb_private::Debugger::StartEventHandlerThread()::$_0,
 void* ()&gt;::operator()() 
            lldb_private::Debugger::DefaultEventHandler()
              lldb_private::Statusline::~Statusline()
                
lldb_private::Statusline::UpdateScrollWindow(lldb_private::Statusline::ScrollWindowMode)
                  lldb_private::Debugger::RefreshIOHandler() 
                    std::__1::recursive_mutex::lock()

UpdateScrollWindow gets the debugger output stream lock, and sends some data to 
the output, then it calls RefreshIOHandler while still holding the Debugger 
output stream lock.  That's the problem, since if it gets that lock before 
Editline::Cancel() completes, we'll get this deadlock.

The solution is simple, there's no reason why UpdateScrollWindow should hold 
the debugger output lock when it calls RefreshIOHandler.  If that refresh ends 
up needing to write to the debugger output, it can take the lock more narrowly 
at that point.  This fixed the deadlock because after yielding the output lock, 
the second thread waits on the first to get the IO Handler lock.  Meanwhile the 
first thread can now acquire the debugger output lock and finish the Cancel, 
and exit PopIOHandler which will allow the second thread to make progress in 
turn.

I couldn't figure out any way to test this...

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


1 Files Affected:

- (modified) lldb/source/Core/Statusline.cpp (+29-28) 


``````````diff
diff --git a/lldb/source/Core/Statusline.cpp b/lldb/source/Core/Statusline.cpp
index 47d48b8474e37..bdc649580637c 100644
--- a/lldb/source/Core/Statusline.cpp
+++ b/lldb/source/Core/Statusline.cpp
@@ -92,35 +92,36 @@ void Statusline::UpdateScrollWindow(ScrollWindowMode mode) {
     return;
 
   const unsigned reduced_scroll_rows = m_terminal_height - 1;
-  LockedStreamFile locked_stream = stream_sp->Lock();
-
-  switch (mode) {
-  case EnableStatusline:
-    // Move everything on the screen up.
-    locked_stream << '\n';
-    locked_stream.Printf(ANSI_UP_ROWS, 1);
-    // Reduce the scroll window.
-    locked_stream << ANSI_SAVE_CURSOR;
-    locked_stream.Printf(ANSI_SET_SCROLL_ROWS, reduced_scroll_rows);
-    locked_stream << ANSI_RESTORE_CURSOR;
-    break;
-  case DisableStatusline:
-    // Reset the scroll window.
-    locked_stream << ANSI_SAVE_CURSOR;
-    locked_stream.Printf(ANSI_SET_SCROLL_ROWS,
-                         static_cast<unsigned>(m_terminal_height));
-    locked_stream << ANSI_RESTORE_CURSOR;
-    // Clear the screen below to hide the old statusline.
-    locked_stream << ANSI_CLEAR_BELOW;
-    break;
-  case ResizeStatusline:
-    // Clear the screen and update the scroll window.
-    // FIXME: Find a better solution (#146919).
-    locked_stream << ANSI_CLEAR_SCREEN;
-    locked_stream.Printf(ANSI_SET_SCROLL_ROWS, reduced_scroll_rows);
-    break;
+  { // Scope for locked_stream:
+    LockedStreamFile locked_stream = stream_sp->Lock();
+
+    switch (mode) {
+    case EnableStatusline:
+      // Move everything on the screen up.
+      locked_stream << '\n';
+      locked_stream.Printf(ANSI_UP_ROWS, 1);
+      // Reduce the scroll window.
+      locked_stream << ANSI_SAVE_CURSOR;
+      locked_stream.Printf(ANSI_SET_SCROLL_ROWS, reduced_scroll_rows);
+      locked_stream << ANSI_RESTORE_CURSOR;
+      break;
+    case DisableStatusline:
+      // Reset the scroll window.
+      locked_stream << ANSI_SAVE_CURSOR;
+      locked_stream.Printf(ANSI_SET_SCROLL_ROWS,
+                           static_cast<unsigned>(m_terminal_height));
+      locked_stream << ANSI_RESTORE_CURSOR;
+      // Clear the screen below to hide the old statusline.
+      locked_stream << ANSI_CLEAR_BELOW;
+      break;
+    case ResizeStatusline:
+      // Clear the screen and update the scroll window.
+      // FIXME: Find a better solution (#146919).
+      locked_stream << ANSI_CLEAR_SCREEN;
+      locked_stream.Printf(ANSI_SET_SCROLL_ROWS, reduced_scroll_rows);
+      break;
+    }
   }
-
   m_debugger.RefreshIOHandler();
 }
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/179789
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits