[Support] exit with custom return code for SIGPIPE

[nickdesaulniers]

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

[jfb]

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

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

300

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]

• use exit code 71 from SIGPIPE signal handler instead

[nickdesaulniers]

• don't mask EPIPE

[nickdesaulniers]

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

[jfb]

lib/Support/Unix/Signals.inc

339

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]

lib/Support/Unix/Signals.inc

339

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

[nickdesaulniers]

• prefer EX_IOERR from sysexits.h

[nickdesaulniers]

Thanks for the suggestions. Done.

[JDevlieghere]

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

[jfb]

In D53000#1430182, @JDevlieghere 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.

[JDevlieghere]

In D53000#1430211, @jfb wrote:

In D53000#1430182, @JDevlieghere 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]

In D53000#1431436, @JDevlieghere wrote:

In D53000#1430211, @jfb wrote:

In D53000#1430182, @JDevlieghere 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.

[JDevlieghere]

In D53000#1431452, @jfb wrote:

In D53000#1431436, @JDevlieghere wrote:

In D53000#1430211, @jfb wrote:

In D53000#1430182, @JDevlieghere 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.

[JDevlieghere]

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.

lib/Support/Unix/Signals.inc

static void RegisterHandlers() { // Not signal-safe.
   // If the handlers are already registered, we're done.
   if (NumRegisteredSignals.load() != 0)
     return;
 
   // Create an alternate stack for signal handling. This is necessary for us to
   // be able to reliably handle signals due to stack overflow.
   CreateSigAltStack();
 
   auto registerHandler = [&](int Signal) {
     unsigned Index = NumRegisteredSignals.load();
     assert(Index < array_lengthof(RegisteredSignalInfo) &&
            "Out of space for signal handlers!");
 
     struct sigaction NewHandler;
 
     NewHandler.sa_handler = SignalHandler;
     NewHandler.sa_flags = SA_NODEFER | SA_RESETHAND | SA_ONSTACK;
     sigemptyset(&NewHandler.sa_mask);
 
     // Install the new handler, save the old one in RegisteredSignalInfo.
     sigaction(Signal, &NewHandler, &RegisteredSignalInfo[Index].SA);
     RegisteredSignalInfo[Index].SigNo = Signal;
     ++NumRegisteredSignals;
   };
 
   for (auto S : IntSigs)
     registerHandler(S);
   for (auto S : KillSigs)
     registerHandler(S);
 }
 
 static void UnregisterHandlers() {
   // Restore all of the signal handlers to how they were before we showed up.
   for (unsigned i = 0, e = NumRegisteredSignals.load(); i != e; ++i) {
     sigaction(RegisteredSignalInfo[i].SigNo,
               &RegisteredSignalInfo[i].SA, nullptr);
     --NumRegisteredSignals;
   }
static RETSIGTYPE SignalHandler(int Sig) {
   {
     RemoveFilesToRemove();
 
     if (std::find(std::begin(IntSigs), std::end(IntSigs), Sig)
         != std::end(IntSigs)) {
       if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
         return OldInterruptFunction();
 
+      // Send a special return code that drivers can check for.
+      if (Sig == SIGPIPE)
+        exit(71);
+
       raise(Sig);   // Execute the default handler.
       return;
    }
   }
 
   // Otherwise if it is a fault (like SEGV) run any handler.
   llvm::sys::RunSignalHandlers();