[lldb] Handle signals in a separate thread in the driver

[JDevlieghere]

Handle signals in a separate thread in the driver so that we can stop worrying about signal safety of functions in libLLDB that may get called from a signal handler.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Handle signals in a separate thread in the driver so that we can stop worrying about signal safety of functions that might get called in libLLDB.

---

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

2 Files Affected:

• (modified) lldb/tools/driver/CMakeLists.txt (+1)
• (modified) lldb/tools/driver/Driver.cpp (+47-31)

diff --git a/lldb/tools/driver/CMakeLists.txt b/lldb/tools/driver/CMakeLists.txt
index 89884ecd0601b..fba61199091ea 100644
--- a/lldb/tools/driver/CMakeLists.txt
+++ b/lldb/tools/driver/CMakeLists.txt
@@ -22,6 +22,7 @@ add_lldb_tool(lldb
 
   LINK_LIBS
     liblldb
+    lldbHost
 
   LINK_COMPONENTS
     Option
diff --git a/lldb/tools/driver/Driver.cpp b/lldb/tools/driver/Driver.cpp
index 15cb0134fec8e..335c2863e9a7f 100644
--- a/lldb/tools/driver/Driver.cpp
+++ b/lldb/tools/driver/Driver.cpp
@@ -19,7 +19,8 @@
 #include "lldb/API/SBStringList.h"
 #include "lldb/API/SBStructuredData.h"
 #include "lldb/Host/Config.h"
-
+#include "lldb/Host/MainLoop.h"
+#include "lldb/Host/MainLoopBase.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
@@ -50,6 +51,8 @@
 
 using namespace lldb;
 using namespace llvm;
+using lldb_private::MainLoop;
+using lldb_private::MainLoopBase;
 
 namespace {
 using namespace llvm::opt;
@@ -89,6 +92,7 @@ static struct termios g_old_stdin_termios;
 static bool disable_color(const raw_ostream &OS) { return false; }
 
 static Driver *g_driver = nullptr;
+static MainLoop g_signal_loop;
 
 // In the Driver::MainLoop, we change the terminal settings.  This function is
 // added as an atexit handler to make sure we clean them up.
@@ -110,6 +114,8 @@ Driver::Driver()
 Driver::~Driver() {
   SBDebugger::Destroy(m_debugger);
   g_driver = nullptr;
+  g_signal_loop.AddPendingCallback(
+      [](MainLoopBase &loop) { loop.RequestTermination(); });
 }
 
 void Driver::OptionData::AddInitialCommand(std::string command,
@@ -637,48 +643,54 @@ void Driver::UpdateWindowSize() {
 }
 
 void sigwinch_handler(int signo) {
-  if (g_driver != nullptr)
-    g_driver->UpdateWindowSize();
+  g_signal_loop.AddPendingCallback([](MainLoopBase &loop) {
+    if (g_driver != nullptr)
+      g_driver->UpdateWindowSize();
+  });
 }
 
 void sigint_handler(int signo) {
 #ifdef _WIN32 // Restore handler as it is not persistent on Windows
   signal(SIGINT, sigint_handler);
 #endif
-  static std::atomic_flag g_interrupt_sent = ATOMIC_FLAG_INIT;
-  if (g_driver != nullptr) {
-    if (!g_interrupt_sent.test_and_set()) {
-      g_driver->GetDebugger().DispatchInputInterrupt();
-      g_interrupt_sent.clear();
-      return;
+  g_signal_loop.AddPendingCallback([signo](MainLoopBase &loop) {
+    static std::atomic_flag g_interrupt_sent = ATOMIC_FLAG_INIT;
+    if (g_driver != nullptr) {
+      if (!g_interrupt_sent.test_and_set()) {
+        g_driver->GetDebugger().DispatchInputInterrupt();
+        g_interrupt_sent.clear();
+        return;
+      }
     }
-  }
 
-  _exit(signo);
+    loop.RequestTermination();
+    _exit(signo);
+  });
 }
 
 #ifndef _WIN32
 static void sigtstp_handler(int signo) {
-  if (g_driver != nullptr)
-    g_driver->GetDebugger().SaveInputTerminalState();
-
-  // Unblock the signal and remove our handler.
-  sigset_t set;
-  sigemptyset(&set);
-  sigaddset(&set, signo);
-  pthread_sigmask(SIG_UNBLOCK, &set, nullptr);
-  signal(signo, SIG_DFL);
-
-  // Now re-raise the signal. We will immediately suspend...
-  raise(signo);
-  // ... and resume after a SIGCONT.
-
-  // Now undo the modifications.
-  pthread_sigmask(SIG_BLOCK, &set, nullptr);
-  signal(signo, sigtstp_handler);
-
-  if (g_driver != nullptr)
-    g_driver->GetDebugger().RestoreInputTerminalState();
+  g_signal_loop.AddPendingCallback([](MainLoopBase &loop) {
+    if (g_driver != nullptr)
+      g_driver->GetDebugger().SaveInputTerminalState();
+  });
+
+  g_signal_loop.AddPendingCallback([signo](MainLoopBase &loop) {
+    // Remove our handler.
+    signal(signo, SIG_DFL);
+
+    // Now re-raise the signal. We will immediately suspend...
+    raise(signo);
+    // ... and resume after a SIGCONT.
+
+    // Now undo the modifications.
+    signal(signo, sigtstp_handler);
+  });
+
+  g_signal_loop.AddPendingCallback([](MainLoopBase &loop) {
+    if (g_driver != nullptr)
+      g_driver->GetDebugger().RestoreInputTerminalState();
+  });
 }
 #endif
 
@@ -794,6 +806,10 @@ int main(int argc, char const *argv[]) {
   signal(SIGTSTP, sigtstp_handler);
 #endif
 
+  // Run the signal handling MainLoop on a separate thread.
+  std::thread signal_thread([] { g_signal_loop.Run(); });
+  signal_thread.detach();
+
   int exit_code = 0;
   // Create a scope for driver so that the driver object will destroy itself
   // before SBDebugger::Terminate() is called.

[JDevlieghere]

@labath let me know if this is what you had in mind. I'm sure there's a lot of code that could be cleaned up after this, but I want to get your feedback on the approach first. Depending on the amount of changes I might decide to do that as separate PRs.

[labath]

Yeah, I'm afraid this isn't the right way to use the MainLoop with signals. AddPendingCallback is not signal-safe. You need to actually let the main loop handle the signals (MainLoopPosix::RegisterSignal), as it knows how to do that safely. The function is not available on windows, so you'd need to #ifdef it, but that should be fine because the current code is conditionalized anyway.

The main exception to that is SIGINT, which is currently used on windows, although I'm not sure it actually works (I have a feeling that ^C just terminated my lldb process). For now, I think you could leave that alone, as I think it's not something that's needed for the status line code. The right way to fix that would probably be to add some generic MainLoop function, which maps to SIGINT on posix and SetConsoleCtrlHandler on windows.

With this setup, I also think the main loop doesn't have to be a global. I think it should be possible to declare it in the main function and then let it clean up after itself before main returns.

[labath]

SIGSTOP will not terminate the process. Maybe you meant SIGQUIT (aka ^\)? But that won't actually matter here because we don't have a handler for that signal, so I'm not sure what you're trying to achieve here..

[JDevlieghere]

I meant SIGINT (i.e. CTRl-C).

[JDevlieghere]

You need to actually let the main loop handle the signals (MainLoopPosix::RegisterSignal), as it knows how to do that safely. The function is not available on windows, so you'd need to #ifdef it, but that should be fine because the current code is conditionalized anyway.

That makes a lot of sense. I was looking at the interface for MainLoopBase. Cool, I'll rework the PR with your input.

[JDevlieghere]

@labath I originally used the MainLoop to handle SIGINT on non-Windows platforms and that didn't caused two test failures. I was able to reproduce the weird behavior by hand.

With the current SIGINT handler:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> 

I send a CTRL-C and a KeyboardInterrupt shows up immediately on screen:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> 
KeyboardInterrupt

With the MainLoop SIGINT handler:

❯ lldb
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>>  

I send a CTRL-C and nothing happens. The KeyboardInterrupt only shows up after I hit enter:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>>  
>>>
KeyboardInterrupt

Like you said, that doesn't work on Windows and we don't need that for the statusline so it's not on the critical path of this PR, but I'm curious if you can think of a reason this behaves oddly with the MainLoop implementation.

[labath]

Like you said, that doesn't work on Windows and we don't need that for the statusline so it's not on the critical path of this PR, but I'm curious if you can think of a reason this behaves oddly with the MainLoop implementation.

I suspect it's because of a different ordering of events. Probably somewhere inside python there's a loop like:

while(...) {
  ssize_t r = read(...);
  if (r == -1 && errno == EINTR && I_got_a_SIGINT()`
    raise KeyboardInterrupt;
}

This works if we call PyErr_SetInterrupt from inside the signal handler, because it can do whatever it needs before I_got_a_SIGINT checks the state. If we do it from another thread, then the reading thread will probably check the condition before we get a chance to set it (and so it only sees the interrupt the next time read returns, which is when you press Return).

If true, this is kind of problematic even right now, because it means the code relies on the signal handler running on the same thread as the read call, which is hard to guarantee, as it's not specified where the signal handler runs in a multithreaded application.

Linux prefers to run it on the main thread (and I wouldn't be surprised if other systems did the same), so it should mostly work if we run the python reading loop (which I think we do) on the main thread -- but I don't think it's a good idea to rely on that.

Interestingly, the SIGINT handler never runs on the main thread on windows (the OS spawns a new thread just for the sake of handling the signal), which I think is why we have this #ifdef here. It would be an interesting experiment to see what happens if you set LLDB_USE_PYTHON_SET_INTERRUPT to zero unconditionally.

[clayborg]

Signals can be listened for via a file descriptor as well. Not sure if this improves the ability to do things in signal handlers as the default way of catching signals has all sorts of limitations. If it does improve the safety and ability to do things in the signal handler, we might want to look into using signalfd?

https://man7.org/linux/man-pages/man2/signalfd.2.html

[labath]

I kinda like signalfd(2), although the implementation makes it kinda hard to use in environments you don't fully control. For one, it requires that the signal you're listening to is blocked on all threads in the process. If any thread unblocks it for any reason, you will miss the signal. And of course, it's linux extension, so we'd still need a fallback for other operating systems. The current MainLoop implementation is basically that -- the signal handler writes to the pipe to wake up the reading thread (it just write a single byte, not the actual siginfo structure, though we could change that if it was necessary).