Page MenuHomePhabricator

[Support] exit with custom return code for SIGPIPE
ClosedPublic

Authored by nickdesaulniers on Oct 8 2018, 2:35 PM.

Details

Summary

We tell the user to file a bug report on LLVM right now, and
SIGPIPE isn't LLVM's fault so our error message is wrong.

Allows frontends to detect SIGPIPE from writing to closed readers.
This can be seen commonly from piping into head, tee, or split.

Fixes PR25349, rdar://problem/14285346, b/77310947

Diff Detail

Repository
rL LLVM

Event Timeline

nickdesaulniers created this revision.Oct 8 2018, 2:35 PM
nickdesaulniers edited the summary of this revision. (Show Details)Oct 8 2018, 2:38 PM
jfb added a comment.Oct 8 2018, 2:48 PM

Can you clarify the description: we tell the user to file a bug report on LLVM right now, and sigpipe ins't LLVM's fault so our error message is wrong.

I'm not sure this is correct: we don't want to ignore SIGPIPE. We want to report the failure to the driver, but we want the driver to know it's a SIGPIPE failure. I think something like I describe here would work best: https://bugs.llvm.org/show_bug.cgi?id=25349#c1

lib/Support/Unix/Signals.inc
279 ↗(On Diff #168710)

I think this should now assert that Signal isn't SIGPIPE because it's explicitly set to ignore below.

300 ↗(On Diff #168710)

Can you comment that this ignore SIGPIPE? SIG_IGN isn't very self-documenting ;-)

lib/Support/raw_ostream.cpp
713 ↗(On Diff #168710)

I think ignoring it is wrong though: we'll continue executing with a partial file. We want to kill the process and fail, but not print out the usual report.

nickdesaulniers edited the summary of this revision. (Show Details)Oct 8 2018, 2:49 PM
  • use exit code 71 from SIGPIPE signal handler instead
  • don't mask EPIPE
nickdesaulniers marked 2 inline comments as done.Oct 8 2018, 3:17 PM
nickdesaulniers retitled this revision from [Support] Ignore SIGPIPE, don't error on EPIPE from raw_fd_ostream to [Support] exit with custom return code for SIGPIPE.
nickdesaulniers edited the summary of this revision. (Show Details)

And https://reviews.llvm.org/D53001 is the accompanying patch for Clang.

jfb added inline comments.Oct 8 2018, 3:41 PM
lib/Support/Unix/Signals.inc
339 ↗(On Diff #168715)

Can you make this number non-magical by defining it somewhere in a header file, and using it here as well as in clang?

majnemer added inline comments.
lib/Support/Unix/Signals.inc
339 ↗(On Diff #168715)

It might be good to use EX_IOERR as defined by sysexits.h aka 74 for this purpose.

  • prefer EX_IOERR from sysexits.h
nickdesaulniers marked 2 inline comments as done.Oct 8 2018, 4:15 PM

Thanks for the suggestions. Done.

jfb accepted this revision.Oct 8 2018, 4:15 PM
This revision is now accepted and ready to land.Oct 8 2018, 4:15 PM
This revision is now accepted and ready to land.Oct 8 2018, 4:15 PM
This revision was automatically updated to reflect the committed changes.

Hey Nick,

This change is causing problems in LLDB. We want to ignore SIGPIPE and this appears to be making that impossible, which doesn't sound fair for something like libSupport. I'm not at all familiar with this code, but can we at least check if the signal is supposed to be ignored here?

Thanks,
Jonas

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 4:18 PM
jfb added a comment.Mar 14 2019, 4:28 PM

Hey Nick,

This change is causing problems in LLDB. We want to ignore SIGPIPE and this appears to be making that impossible, which doesn't sound fair for something like libSupport. I'm not at all familiar with this code, but can we at least check if the signal is supposed to be ignored here?

I think you'll want to set up something like InterruptFunction: by default we'll do what Nick added, but if you ask we can do something else for SIGPIPE.

In D53000#1430211, @jfb wrote:

Hey Nick,

This change is causing problems in LLDB. We want to ignore SIGPIPE and this appears to be making that impossible, which doesn't sound fair for something like libSupport. I'm not at all familiar with this code, but can we at least check if the signal is supposed to be ignored here?

I think you'll want to set up something like InterruptFunction: by default we'll do what Nick added, but if you ask we can do something else for SIGPIPE.

Wouldn't that affect all signals? What about an atomic boolean to enable/disable this behavior? Like I said before, I personally don't think this it is expected from a library like Support to do this. It feels like this behavior should be opt-in.

jfb added a comment.Mar 15 2019, 2:21 PM
In D53000#1430211, @jfb wrote:

Hey Nick,

This change is causing problems in LLDB. We want to ignore SIGPIPE and this appears to be making that impossible, which doesn't sound fair for something like libSupport. I'm not at all familiar with this code, but can we at least check if the signal is supposed to be ignored here?

I think you'll want to set up something like InterruptFunction: by default we'll do what Nick added, but if you ask we can do something else for SIGPIPE.

Wouldn't that affect all signals? What about an atomic boolean to enable/disable this behavior? Like I said before, I personally don't think this it is expected from a library like Support to do this. It feels like this behavior should be opt-in.

I think we want something like InterruptFunction, where for clang (not as a library) we'll call abort, and for you we'd do nothing. Which one is the default doesn't really matter to me, and maybe makes more sense to do nothing. I think this is better than just a boolean.

In D53000#1431452, @jfb wrote:
In D53000#1430211, @jfb wrote:

Hey Nick,

This change is causing problems in LLDB. We want to ignore SIGPIPE and this appears to be making that impossible, which doesn't sound fair for something like libSupport. I'm not at all familiar with this code, but can we at least check if the signal is supposed to be ignored here?

I think you'll want to set up something like InterruptFunction: by default we'll do what Nick added, but if you ask we can do something else for SIGPIPE.

Wouldn't that affect all signals? What about an atomic boolean to enable/disable this behavior? Like I said before, I personally don't think this it is expected from a library like Support to do this. It feels like this behavior should be opt-in.

I think we want something like InterruptFunction, where for clang (not as a library) we'll call abort, and for you we'd do nothing. Which one is the default doesn't really matter to me, and maybe makes more sense to do nothing. I think this is better than just a boolean.

Oh yeah, that makes sense too.

On second thought, the InterruptFunction doesn't take an argument so there's no way to check whether the signal was a SIGPIPE. We could have the function take an argument but that wouldn't make any sense on Windows.