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

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked an inline comment as done.
vsk added a comment.

I admit I hadn't paid enough attention to the order in which signal handlers were registered in lldb.



================
Comment at: lldb/tools/driver/Driver.cpp:850
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69148