Thanks Vedant! Minor: there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?
I'm wondering if the code shouldn't read more like:

if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE) {
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

  if (Sig == SIGPIPE)
    if (auto OldOneShotPipeFunction =
            OneShotPipeSignalFunction.exchange(nullptr))
      return OldOneShotPipeFunction();

  raise(Sig); // Execute the default handler.
  return;
}

In rG9a3f892d018238dce5181e458905311db8e682f5#856804, @aganea wrote:

Thanks Vedant! Minor: there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?
I'm wondering if the code shouldn't read more like:

if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE) {
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

  if (Sig == SIGPIPE)
    if (auto OldOneShotPipeFunction =
            OneShotPipeSignalFunction.exchange(nullptr))
      return OldOneShotPipeFunction();

  raise(Sig); // Execute the default handler.
  return;
}

@aganea I genuinely apologize for losing track of your feedback. I didn't realize I had missed this until Duncan pinged me, after the recent commentary on D70277.

there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?

That's the correct assessment. RunSignalHandlers runs handlers added by AddSignalHandler, which is for "when an abort/kill signal is delivered to the process": that's not a great fit for SIGPIPE. OTOH, your proposed change would do away with a nice guarantee made in the SetOneShopPipeSignalHandler documentation, which states: "The LLVM signal handling code will not install any handler for the pipe signal unless one is provided with this API". Wdyt of re-raising the signal when the SIGPIPE/IntSigs handlers are missing?:

if (Sig == SIGPIPE)
  if (auto OldOneShotPipeFunction =
          OneShotPipeSignalFunction.exchange(nullptr))
    return OldOneShotPipeFunction();

bool IsIntSig = llvm::is_contained(IntSigs, Sig);
if (IsIntSig)
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

if (Sig == SIGPIPE || IsIntSig) {
  raise(Sig);   // Execute the default handler.
  return;
}

FWIW, that logic look corrects to me.

Seems good to me too! Thank you!

Thanks, I've landed this change as bf401256edd0.