[Lldb-commits] [PATCH] D69148: Disable exit-on-SIGPIPE in lldb

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/tools/driver/Driver.cpp:850
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
----------------
vsk wrote:
> labath wrote:
> > vsk wrote:
> > > labath wrote:
> > > > I don't think this line does anything anymore.
> > > I don't think that's true. LLVM still does `registerHandler(SIGPIPE, SignalKind::IsKill)` in `RegisterHandlers`. At least, removing the `signal(SIGPIPE, SIG_IGN)` calls causes the unit test to "fail" (i.e. the test RUNs but doesn't reach OK).
> > Ah, right, I see now. This does have a very subtle change in behavior -- it changes the signal handler that llvm restores *after* the signal has been received the first time (Signals.inc:355). Normally, it would put SIG_DFL there (which would kill the process). But with this, it puts SIG_IGN there. So this changes what we do when we receive SIGPIPE for the *second* time.
> > 
> > This means that the first SIGPIPE signal is ignored by the virtue of us calling `SetPipeSignalFunction(nullptr)`, but all subsequent SIGPIPEs are ignored thanks to this SIG_IGN. It also means this whole talk of "being able to survive multiple SIGPIPEs" is moot, as llvm does not allow that. In fact it looks like the very first occurence of SIGPIPE will cause _all_ signal handlers to be uninstalled. I think this is a very confusing situation to be in.
> > 
> > If we don't want to make llvm support handling/ignoring multiple SIGPIPEs correctly, then I think that a much better solution would be to just work around this in lldb by making sure the llvm handler is never invoked for SIGPIPE (this can be arranged by making sure `signal(SIGPIPE, SIG_IGN)` is called *after* llvm installs its handlers.
> Point taken. I agree that when a SIGPIPE is received, it doesn't make sense to reset the signal handler state as it was prior to llvm's handler registration. Do you think it'd be reasonable to:
> 
> - In lldb, force llvm to register its handlers, then immediately ignore SIGPIPE.
> - Revert the `SetPipeSignalFunction` changes, as they would no longer be required.
As far as workarounds go, I think that's the best we can do know (+ add a big comment explaining what we're doing there).

I think that overall the llvm signal handling needs a lot more work, but as that would probably require buy-in from various actors, I don't think that it should be you who has to fix all that.

FTR, I find the current signal handling very strange, and completely unsuitable for something which claims to be a library. I don't believe a library should install any signal handler unless it's explicitly asked to, or, at the very least, it should provide a mechanism to opt out of this.
I would make this handler installation a part of InitLLVM, or something similar, and make it possible to customize its behavior a lot more. The current implementation is very clearly tailored for one-shot non-interactive tools that immediately exit when encountering a problem. Besides SIGPIPE (which even the non-interactive tools might want to ignore), it's handling of SIGINT is also definitely not what lldb wants to do (forward it to the debugged process). There are even some JIT tools that might want to catch & recover from SIGSEGVs, but this implementation makes that very hard to do because of the whole race about who installs the signal handlers last.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69148