Page MenuHomePhabricator

[Signal] Allow llvm clients to opt into one-shot SIGPIPE handling
ClosedPublic

Authored by vsk on Nov 14 2019, 2:39 PM.

Details

Summary

Allow clients of the llvm library to opt-in to one-shot SIGPIPE
handling, instead of forcing them to undo llvm's SIGPIPE handler
registration (which is brittle).

The current behavior is preserved for all llvm-derived tools (except
lldb) by means of a default-true flag in the InitLLVM constructor.

This prevents "IO error" crashes in long-lived processes (lldb is the
motivating example) which both a) load llvm as a dynamic library and b)
*really* need to ignore SIGPIPE.

As llvm signal handlers can be installed when calling into libclang
(say, via RemoveFileOnSignal), thereby overriding a previous SIG_IGN for
SIGPIPE, there is no clean way to opt-out of "exit-on-SIGPIPE" in the
current model.

Diff Detail

Event Timeline

vsk created this revision.Nov 14 2019, 2:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 14 2019, 2:39 PM

As I said in the other discussion, though this is a (small) step in what I consider to be the right direction for this, it still seems like a workaround to me. But if everyone else is fine with that, then I won't stand in your way.

One thing I am not sure of is why you've chosen clang as the only recipient of the old SIGPIPE behavior. It seems like pretty much every non-interactive llvm utility would benefit from the previously-default SIGPIPE behavior. This is why I was talking about building this functionality into InitLLVM somehow -- one could make it so that InitLLVM installs the sigpipe handler by default, and lldb could pass some extra argument or something to disable that behavior.

llvm/lib/Support/Unix/Signals.inc
328

This bit is tricky. It means that SetOneShotPipeSignalFunction will take effect only if it is called before we install any signal handlers. Calling it later will not cause the handler to be installed. However, if one does set a handler function before signals are installed, then it is possible to change it with SetOneShotPipeSignalFunction. This makes its behavior a lot more complex than SetInfoSignalFunction, which it tries to emulate.

vsk updated this revision to Diff 229601.Nov 15 2019, 11:21 AM
vsk edited the summary of this revision. (Show Details)
  • Preserve the current behavior w.r.t SIGPIPE for all llvm-derived tools except lldb.
  • Document the trickiness re: registering a SIGPIPE handler before llvm sets up its signal handlers.
JDevlieghere accepted this revision.Nov 15 2019, 11:29 AM

LGTM if Pavel's happy :-)

This revision is now accepted and ready to land.Nov 15 2019, 11:29 AM
labath accepted this revision.Nov 18 2019, 1:02 AM

I wouldn't use the word "happy", but I think this is fine. :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2019, 10:32 AM
aganea added a subscriber: aganea.Dec 4 2019, 3:23 PM
aganea added inline comments.
llvm/lib/Support/Unix/Signals.inc
369

@vsk Question question: SIGPIPE isn't in the IntSigs array anymore (you've removed it at L209), so the above std::find will fail when Sig == SIGPIPE and this code will never be executed. Most likely this isn't the intended behavior, or I am missing something?

vsk marked an inline comment as done.Dec 4 2019, 7:39 PM
vsk added inline comments.
llvm/lib/Support/Unix/Signals.inc
369

Thanks for catching this, I've fixed this in 9a3f892d018.