[lldb] Fix that the embedded Python REPL crashes if it receives SIGINT

[JDevlieghere]

Summary

When LLDB receives a SIGINT while running the embedded Python REPL it currently just
crashes in ScriptInterpreterPythonImpl::Interrupt with an error such as the one below:

Fatal Python error: PyThreadState_Get: the function must be called with the GIL held, but the GIL is released (the current Python thread state is NULL)

The faulty code that causes this error is this part of ScriptInterpreterPythonImpl::Interrupt:

PyThreadState *state = PyThreadState_GET();
if (!state)
  state = GetThreadState();
if (state) {
  long tid = state->thread_id;
  PyThreadState_Swap(state);
  int num_threads = PyThreadState_SetAsyncExc(tid, PyExc_KeyboardInterrupt);

The obvious fix I tried is to just acquire the GIL before this code is running which fixes the
crash but the KeyboardInterrupt we want to raise immediately is actually just queued
and would only be raised once the next line of input has been parsed (which e.g. won't
interrupt Python code that is currently waiting on a timer or IO from what I can see).
Also none of the functions we call here is marked as safe to be called from a signal
handler from what I can see, so we might still end up crashing here with some bad timing.

Python 3.2 introduced PyErr_SetInterrupt to solve this and the function takes care of
all the details and avoids doing anything that isn't safe to do inside a signal handler.
The only thing we need to do is to manually setup our own fake SIGINT handler that
behaves the same way as the standalone Python REPL signal handler (which raises
a KeyboardInterrupt).

From what I understand the old code used to work with Python 2 so I kept the old
code around until we officially drop support for Python 2.

There is a small gap here with Python 3.0->3.1 where we might still be crashing, but
those versions have reached their EOL more than a decade ago so I think we don't need
to bother about them.

[JDevlieghere]

Nice, thanks for figuring this one out. LGTM.

[teemperor]

• Removed unused include and revert some unintentional whitespace change.

[teemperor]

• Don't pretend to have handled the interrupt if we're not running Python at the moment.
• Make sure that Python doesn't overwrite the LLDB process signal handlers.
• Extend tests to cover aborting running Python code and sending it while Python is waiting for user input.

[teemperor]

Sending back as I changed quite a few things to handle more corner cases.

[JDevlieghere]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

3234–3236

IIRC signal handlers are per process, so there's no such thing as just changing the Python one. I feel a bit uneasy about swapping out signal handlers, but I don't know a better way to do this. Do you happen to know how this worked with Python 2? Was it already swapping out the signal handler behind our back?

[teemperor]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

3234–3236

Python maintains its own set of internal signal handler objects which is what we're setting below in the code snippet. Those Python-internal 'signal handlers' are what the SetInterrupt thingy above is calling. But cpython is also hooking up those Python object signal handlers to the process signal handlers when calling signal.signal. I'm trying to revert that change to our process signal handlers while still creating the Python signal handler objects (which we need so that we can call SetInterrupt). I don't think there is a way to just create the internal signal handlers.

In Python 2 we didn't create any signal handlers from Python. We just had no SIGINT signal handler at all beside the LLDB one. And the LLDB signal handler just tried to create the KeyboardInterrupt exception object. But that code was neither signal safe nor did it interrupt IO events, at least it's not so in Python3 (but the documentation is also rather minimalistic.

I'll update the docs here. Most of this is just a product of reading cpython's init/signal module implementation so it makes sense to better explain what's going on here.

[teemperor]

@labath raised some concerns in IRC about the setup code being run in a potential multithreaded env (which makes sense). Feel free to revert (not sure when/if I'll get around to address the issues)

[stella.stamenova]

This broke the build on Windows. The buildbot was, sadly, already red because of another test failure:

https://lab.llvm.org/buildbot/#/builders/83/builds/11931

[labath]

In D104886#3127411, @teemperor wrote:

@labath raised some concerns in IRC about the setup code being run in a potential multithreaded env (which makes sense). Feel free to revert (not sure when/if I'll get around to address the issues)

Yes, I multithreaded code (and code in libraries in particular) should be very careful what it's doing with signals and signal handlers. That said, after taking a closer look at this. I don't think this patch is as bad as it originally seemed to me. The signal handlers are only changed for a bried period, and most of the signal code in the patch is there to undo the handlers that python sets.

Probably the only thing bothering me now is that all of this work happens lazily, whenever we happen to use the python interpreter for the first time. It would be much better if this happened in the initialization phase, as most applications will likely still be single-threaded at that point (and even if they weren't, it any global effects of the SBDebugger::Ininitalize call would be less surprising (more predictable)). I wouldn't call that a blocker though, since it would require a "how we initialize python" discussion, and it does not add any technical debt there (it would be straightforward to move this to the initialize call).

lldb/test/API/iohandler/sigint/TestIOHandlerPythonREPLSigint.py

58–60

This is probably a consequence of (a bug in) our libedit/readline compatibility workarounds in source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp

[stella.stamenova]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

73

Were these not used on Windows at all before?

[stella.stamenova]

LGTM.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

 #define LLDBSwigPyInit PyInit__lldb
 
 #else
 extern "C" void init_lldb(void);
 
 #define LLDBSwigPyInit init_lldb
 #endif
 
+#if defined(_WIN32)
+// Don't mess with the signal handlers on Windows.
+#define LLDB_USE_PYTHON_SET_INTERRUPT 0
+#else
+// PyErr_SetInterrupt was introduced in 3.2.
+#define LLDB_USE_PYTHON_SET_INTERRUPT                                          \
+  (PY_MAJOR_VERSION == 3 && PY_MINOR_VERSION >= 2) || (PY_MAJOR_VERSION > 3)
+#endif
 
 static ScriptInterpreterPythonImpl *GetPythonInterpreter(Debugger &debugger) {
   ScriptInterpreter *script_interpreter =
       debugger.GetScriptInterpreter(true, lldb::eScriptLanguagePython);
   return static_cast<ScriptInterpreterPythonImpl *>(script_interpreter);
 }
 
 static bool g_initialized = false;
void ScriptInterpreterPythonImpl::ExecuteInterpreterLoop() {
 
   IOHandlerSP io_handler_sp(new IOHandlerPythonInterpreter(debugger, this));
   if (io_handler_sp) {
     debugger.RunIOHandlerAsync(io_handler_sp);
   }
 }
 
 bool ScriptInterpreterPythonImpl::Interrupt() {
+#if LLDB_USE_PYTHON_SET_INTERRUPT
+  // If the interpreter isn't evaluating any Python at the moment then return
+  // false to signal that this function didn't handle the interrupt and the
+  // next component should try handling it.
+  if (!IsExecutingPython())
+    return false;
+
+  // Tell Python that it should pretend to have received a SIGINT.
+  PyErr_SetInterrupt();
+  // PyErr_SetInterrupt has no way to return an error so we can only pretend the
+  // signal got successfully handled and return true.
+  // Python 3.10 introduces PyErr_SetInterruptEx that could return an error, but
+  // the error handling is limited to checking the arguments which would be
+  // just our (hardcoded) input signal code SIGINT, so that's not useful at all.
+  return true;
+#else
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_SCRIPT));
 
   if (IsExecutingPython()) {
     PyThreadState *state = PyThreadState_GET();
     if (!state)
       state = GetThreadState();
     if (state) {
       long tid = state->thread_id;
       PyThreadState_Swap(state);
       int num_threads = PyThreadState_SetAsyncExc(tid, PyExc_KeyboardInterrupt);
       LLDB_LOGF(log,
                 "ScriptInterpreterPythonImpl::Interrupt() sending "
                 "PyExc_KeyboardInterrupt (tid = %li, num_threads = %i)...",
                 tid, num_threads);
       return true;
     }
   }
   LLDB_LOGF(log,
             "ScriptInterpreterPythonImpl::Interrupt() python code not running, "
             "can't interrupt");
   return false;
+#endif
 }
 
 bool ScriptInterpreterPythonImpl::ExecuteOneLineWithReturn(
     llvm::StringRef in_string, ScriptInterpreter::ScriptReturnType return_type,
     void *ret_value, const ExecuteScriptOptions &options) {
 
   llvm::Expected<std::unique_ptr<ScriptInterpreterIORedirect>>
       io_redirect_or_error = ScriptInterpreterIORedirect::Create(

 std::unique_ptr<ScriptInterpreterLocker>
 ScriptInterpreterPythonImpl::AcquireInterpreterLock() {
   std::unique_ptr<ScriptInterpreterLocker> py_lock(new Locker(
       this, Locker::AcquireLock | Locker::InitSession | Locker::NoSTDIN,
       Locker::FreeLock | Locker::TearDownSession));
   return py_lock;
 }
 
+
+#if LLDB_USE_PYTHON_SET_INTERRUPT
+namespace {
+/// Saves the current signal handler for the specified signal and restores
+/// it at the end of the current scope.
+struct RestoreSignalHandlerScope {
+  /// The signal handler.
+  struct sigaction m_prev_handler;
+  int m_signal_code;
+  RestoreSignalHandlerScope(int signal_code) : m_signal_code(signal_code) {
+    // Initialize sigaction to their default state.
+    std::memset(&m_prev_handler, 0, sizeof(m_prev_handler));
+    // Don't install a new handler, just read back the old one.
+    struct sigaction *new_handler = nullptr;
+    int signal_err = ::sigaction(m_signal_code, new_handler, &m_prev_handler);
+    lldbassert(signal_err == 0 && "sigaction failed to read handler");
+  }
+  ~RestoreSignalHandlerScope() {
+    int signal_err = ::sigaction(m_signal_code, &m_prev_handler, nullptr);
+    lldbassert(signal_err == 0 && "sigaction failed to restore old handler");
+  }
+};
+} // namespace
+#endif
+
 void ScriptInterpreterPythonImpl::InitializePrivate() {
   if (g_initialized)
     return;
 
   g_initialized = true;
 
   LLDB_SCOPED_TIMER();
 
if (FileSpec file_spec = GetPythonDir())
     AddToSysPath(AddLocation::Beginning, file_spec.GetPath(false));
   if (FileSpec file_spec = HostInfo::GetShlibDir())
     AddToSysPath(AddLocation::Beginning, file_spec.GetPath(false));
 
   PyRun_SimpleString("sys.dont_write_bytecode = 1; import "
                      "lldb.embedded_interpreter; from "
                      "lldb.embedded_interpreter import run_python_interpreter; "
                      "from lldb.embedded_interpreter import run_one_line");
+
+#if LLDB_USE_PYTHON_SET_INTERRUPT
+  // Python will not just overwrite its internal SIGINT handler but also the
+  // one from the process. Backup the current SIGINT handler to prevent that
+  // Python deletes it.
+  RestoreSignalHandlerScope save_sigint(SIGINT);
+
+  // Setup a default SIGINT signal handler that works the same way as the
+  // normal Python REPL signal handler which raises a KeyboardInterrupt.
+  // Also make sure to not pollute the user's REPL with the signal module nor
+  // our utility function.
+  PyRun_SimpleString("def lldb_setup_sigint_handler():\n"
+                     "  import signal;\n"
+                     "  def signal_handler(sig, frame):\n"
+                     "    raise KeyboardInterrupt()\n"
+                     "  signal.signal(signal.SIGINT, signal_handler);\n"
+                     "lldb_setup_sigint_handler();\n"
+                     "del lldb_setup_sigint_handler\n");
+#endif
 }
 
 void ScriptInterpreterPythonImpl::AddToSysPath(AddLocation location,
                                                std::string path) {
   std::string path_copy;
 
   std::string statement;
   if (location == AddLocation::Beginning) {

lldb/test/API/iohandler/sigint/TestIOHandlerPythonREPLSigint.py

+"""
+Test sending SIGINT to the embedded Python REPL.
+"""
+
+import os
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestCase(PExpectTest):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def start_python_repl(self):
+        """ Starts up the embedded Python REPL."""
+        self.launch()
+        # Start the embedded Python REPL via the 'script' command.
+        self.child.send("script -l python --\n")
+        # Wait for the Python REPL prompt.
+        self.child.expect(">>>")
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    @skipIfWindows
+    def test_while_evaluating_code(self):
+        """ Tests SIGINT handling while Python code is being evaluated."""
+        self.start_python_repl()
+
+        # Start a long-running command that we try to abort with SIGINT.
+        # Note that we dont actually wait 10000s in this code as pexpect or
+        # lit will kill the test way before that.
+        self.child.send("import time; print('running' + 'now'); time.sleep(10000);\n")
+
+        # Make sure the command is actually being evaluated at the moment by
+        # looking at the string that the command is printing.
+        # Don't check for a needle that also occurs in the program itself to
+        # prevent that echoing will make this check pass unintentionally.
+        self.child.expect("runningnow")
+
+        # Send SIGINT to the LLDB process.
+        self.child.sendintr()
+
+        # This should get transformed to a KeyboardInterrupt which is the same
+        # behaviour as the standalone Python REPL. It should also interrupt
+        # the evaluation of our sleep statement.
+        self.child.expect("KeyboardInterrupt")
+        # Send EOF to quit the Python REPL.
+        self.child.sendeof()
+
+        self.quit()
+
+    # PExpect uses many timeouts internally and doesn't play well
+    # under ASAN on a loaded machine..
+    @skipIfAsan
+    # FIXME: On Linux the Python code that reads from stdin seems to block until
+    # it has finished reading a line before handling any queued signals.
+    @skipIfLinux
+    @skipIfWindows
+    def test_while_waiting_on_input(self):
+        """ Tests SIGINT handling while the REPL is waiting on input from
+        stdin."""
+        self.start_python_repl()
+
+        # Send SIGINT to the LLDB process.
+        self.child.sendintr()
+        # This should get transformed to a KeyboardInterrupt which is the same
+        # behaviour as the standalone Python REPL.
+        self.child.expect("KeyboardInterrupt")
+        # Send EOF to quit the Python REPL.
+        self.child.sendeof()
+
+        self.quit()