[Lldb-commits] [PATCH] D104886: [lldb] Fix that the embedded Python REPL crashes if it receives SIGINT

Mon Nov 15 00:11:51 PST 2021

labath added a comment.

In D104886#3127411 <https://reviews.llvm.org/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).

================
Comment at: lldb/test/API/iohandler/sigint/TestIOHandlerPythonREPLSigint.py:57-59
+    # 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
----------------
This is probably a consequence of (a bug in) our libedit/readline compatibility workarounds in `source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp`

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104886/new/

https://reviews.llvm.org/D104886