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

Mon Oct 21 16:33:49 PDT 2019

labath added a comment.

In D69148#1717361 <https://reviews.llvm.org/D69148#1717361>, @vsk wrote:

> A problem I'm encountering with this is that the static initializer from Signals.inc don't appear to be re-run between death tests. This makes the death tests pretty brittle, imo, as swapping the order of the tests can break them. Do you think the extra coverage is worth it? (See https://reviews.llvm.org/D69283 for what this looks like.)

Yeah, not being able to run each test in a separate process is a big problem here. I'm not too worried about adding those tests here. However, looking at those tests, and your inline comment, I get the impression you're not understanding how the various signal functions actually work. One definitely shouldn't be calling the `signal` function manually, and also using the llvm signal handler functions (at least, not without a very big comment explaining what he's doing). They both do the same thing -- set the global signal handler. And since the llvm handling is initialized lazily this sets us up for all sorts of unpredictible and confusing behavior. See the inline comment for details.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69148