Disable exit-on-SIGPIPE in lldb

[vsk]

Summary

Occasionally, during test teardown, LLDB writes to a closed pipe.
Sometimes the communication is inherently unreliable, so LLDB tries to
avoid being killed due to SIGPIPE. Actually, it explicitly calls
signal(SIGPIPE, SIG_IGN). However, LLVM's default SIGPIPE behavior is
to exit with IO_ERR. Opt LLDB out of that.

I expect that this will resolve some LLDB test suite flakiness (tests
randomly failing with IO_ERR) that we've seen since r344372.

rdar://55750240

[jfb]

lldb/tools/driver/Driver.cpp

864

Could this instead pass a lambda, like other signal handlers? So sigpipe would call any user-specified lambda, and by default would instead call exit.

[vsk]

lldb/tools/driver/Driver.cpp

864

Ah, sure. That would make things a bit more customizable.

[vsk]

• Allow setting a SIGPIPE handler.

[jfb]

More of an FYI, @jordan_rose might be interested in this.

[jordan_rose]

llvm/lib/Support/Unix/Signals.inc

372–373

Should it be doing the same sort of exchange as the interrupt function?

[vsk]

llvm/lib/Support/Unix/Signals.inc

372–373

The interrupt handler is apparently meant to be disabled after it’s used once (not sure why, the docs don’t say). As lldb is long-lived, I expect it should survive multiple SIGPIPEs.

[JDevlieghere]

Thanks for fixing this, Vedant!

[jordan_rose]

llvm/lib/Support/Unix/Signals.inc

372–373

I guess the SIGINT one makes sense so that you can kill a stuck process with ^C^C.

[vsk]

The death tests are flaky. I've noticed two issues:

1. When run under lit, the DisableExitOnSIGPIPE doesn't actually exit when it receives SIGPIPE. Dtrace suggests that the unit test process inherits an "ignore" sigaction from its parent.
2. When run in parallel, exiting causes the unit test to run atexit destructors for other unit tests lumped into the SupportTests binary. One of these is a condition_variable destructor and it hangs. Also, gtest warns: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test detected 21 threads.

So I plan to give up on testing "EnableExitOnSIGPIPE" and will write a non-death test to get some coverage.

[vsk]

Replace the death tests with a non-death test. I plan to commit this later in the day if there aren't any objections.

[labath]

In D69148#1714785, @vsk wrote:

The death tests are flaky. I've noticed two issues:

1. When run under lit, the DisableExitOnSIGPIPE doesn't actually exit when it receives SIGPIPE. Dtrace suggests that the unit test process inherits an "ignore" sigaction from its parent.
2. When run in parallel, exiting causes the unit test to run atexit destructors for other unit tests lumped into the SupportTests binary. One of these is a condition_variable destructor and it hangs. Also, gtest warns: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test detected 21 threads.

So I plan to give up on testing "EnableExitOnSIGPIPE" and will write a non-death test to get some coverage.

gtest gives you a (somewhat rudimentary) mechanism of ensuring death tests run in a single threaded fashion. It consists of tacking "DeathTest" at the end of your test case name (like TEST(SignalDeathTest, Whatever)). These kinds of tests are run first, hopefully before the tests which spin up various other threads.

As for the parent "ignore" action (it's more likely to be a signal mask, actually), this can be reset with an appropriate sigprocmask(2) call.

lldb/tools/driver/Driver.cpp

850

I don't think this line does anything anymore.

[vsk]

In D69148#1715532, @labath wrote:

In D69148#1714785, @vsk wrote:

The death tests are flaky. I've noticed two issues:

1. When run under lit, the DisableExitOnSIGPIPE doesn't actually exit when it receives SIGPIPE. Dtrace suggests that the unit test process inherits an "ignore" sigaction from its parent.
2. When run in parallel, exiting causes the unit test to run atexit destructors for other unit tests lumped into the SupportTests binary. One of these is a condition_variable destructor and it hangs. Also, gtest warns: Death tests use fork(), which is unsafe particularly in a threaded context. For this test, Google Test detected 21 threads.

So I plan to give up on testing "EnableExitOnSIGPIPE" and will write a non-death test to get some coverage.

gtest gives you a (somewhat rudimentary) mechanism of ensuring death tests run in a single threaded fashion. It consists of tacking "DeathTest" at the end of your test case name (like TEST(SignalDeathTest, Whatever)). These kinds of tests are run first, hopefully before the tests which spin up various other threads.

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.)

As for the parent "ignore" action (it's more likely to be a signal mask, actually), this can be reset with an appropriate sigprocmask(2) call.

Thanks, that's a great catch.

lldb/tools/driver/Driver.cpp

850

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).

[labath]

In 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.

lldb/tools/driver/Driver.cpp

850

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.

[labath]

In D69148#1717453, @labath wrote:

I'm not too worried about adding those tests here.

I'm not too worried about *not* adding those tests here.

[vsk]

I admit I hadn't paid enough attention to the order in which signal handlers were registered in lldb.

lldb/tools/driver/Driver.cpp

850

Point taken. I agree that when a SIGPIPE is received, it doesn't make sense to reset the signal handler state as it was prior to llvm's handler registration. Do you think it'd be reasonable to:

• In lldb, force llvm to register its handlers, then immediately ignore SIGPIPE.
• Revert the SetPipeSignalFunction changes, as they would no longer be required.

[labath]

lldb/tools/driver/Driver.cpp

850

As far as workarounds go, I think that's the best we can do know (+ add a big comment explaining what we're doing there).

I think that overall the llvm signal handling needs a lot more work, but as that would probably require buy-in from various actors, I don't think that it should be you who has to fix all that.

FTR, I find the current signal handling very strange, and completely unsuitable for something which claims to be a library. I don't believe a library should install any signal handler unless it's explicitly asked to, or, at the very least, it should provide a mechanism to opt out of this.
I would make this handler installation a part of InitLLVM, or something similar, and make it possible to customize its behavior a lot more. The current implementation is very clearly tailored for one-shot non-interactive tools that immediately exit when encountering a problem. Besides SIGPIPE (which even the non-interactive tools might want to ignore), it's handling of SIGINT is also definitely not what lldb wants to do (forward it to the debugged process). There are even some JIT tools that might want to catch & recover from SIGSEGVs, but this implementation makes that very hard to do because of the whole race about who installs the signal handlers last.

[vsk]

Reverted in d0bd3fc88be.

[nickdesaulniers]

Sounds like another approach was landed in https://reviews.llvm.org/D69403, and a follow up in https://reviews.llvm.org/D70277, and 9a3f892d018238dce5181e458905311db8e682f5.

lldb/tools/driver/Driver.cpp

if (error.Fail()) {
     WithColor::error() << "initialization failed: " << error.GetCString()
                        << '\n';
     return 1;
   }
   SBHostOS::ThreadCreated("<lldb.driver.main-thread>");
 
   signal(SIGINT, sigint_handler);
 #if !defined(_MSC_VER)
   signal(SIGPIPE, SIG_IGN);
   signal(SIGWINCH, sigwinch_handler);
   signal(SIGTSTP, sigtstp_handler);
   signal(SIGCONT, sigcont_handler);
 #endif
 
+  // Occasionally, during test teardown, LLDB writes to a closed pipe.
+  // Sometimes the communication is inherently unreliable, so LLDB tries to
+  // avoid being killed due to SIGPIPE. However, LLVM's default SIGPIPE behavior
+  // is to exit with IO_ERR. Opt LLDB out of that.
+  //
+  // We don't disable LLVM's signal handling entirely because we still want
+  // pretty stack traces, and file cleanup (for when, say, the clang embedded
+  // in LLDB leaves behind temporary objects).
+  llvm::sys::SetPipeSignalFunction(nullptr);
+
   int exit_code = 0;
   // Create a scope for driver so that the driver object will destroy itself
   // before SBDebugger::Terminate() is called.
   {
     Driver driver;
 
     bool exiting = false;
     SBError error(driver.ProcessArgs(input_args, exiting));

llvm/include/llvm/Support/Signals.h

namespace sys {
   /// On POSIX systems, this will be SIGUSR1; on systems that have it, SIGINFO
   /// will also be used (typically ctrl-t).
   ///
   /// Note that signal handlers are not allowed to call any non-reentrant
   /// functions.  An null function pointer disables the current installed
   /// function.  Note also that the handler may be executed on a different
   /// thread on some platforms.
   void SetInfoSignalFunction(void (*Handler)());
+
+  /// Registers a function to be called when a "pipe" signal is delivered to
+  /// the process.
+  ///
+  /// The "pipe" signal typically indicates a failed write to a pipe (SIGPIPE).
+  /// The default installed handler calls `exit(EX_IOERR)`, causing the process
+  /// to immediately exit with an IO error exit code.
+  ///
+  /// This function is only applicable on POSIX systems.
+  void SetPipeSignalFunction(void (*Handler)());
+
 } // End sys namespace
 } // End llvm namespace
 
 #endif

llvm/lib/Support/Unix/Signals.inc

 #endif
 #endif
 
 using namespace llvm;
 
 static RETSIGTYPE SignalHandler(int Sig);  // defined below.
 static RETSIGTYPE InfoSignalHandler(int Sig);  // defined below.
 
+static void DefaultPipeSignalFunction() {
+  exit(EX_IOERR);
+}
+
 using SignalHandlerFunctionType = void (*)();
 /// The function to call if ctrl-c is pressed.
 static std::atomic<SignalHandlerFunctionType> InterruptFunction =
     ATOMIC_VAR_INIT(nullptr);
 static std::atomic<SignalHandlerFunctionType> InfoSignalFunction =
     ATOMIC_VAR_INIT(nullptr);
+static std::atomic<SignalHandlerFunctionType> PipeSignalFunction =
+    ATOMIC_VAR_INIT(DefaultPipeSignalFunction);
 
 namespace {
 /// Signal-safe removal of files.
 /// Inserting and erasing from the list isn't signal-safe, but removal of files
 /// themselves is signal-safe. Memory is freed when the head is freed, deletion
 /// is therefore not signal-safe either.
 class FileToRemoveList {
   std::atomic<char *> Filename = ATOMIC_VAR_INIT(nullptr);
sigprocmask(SIG_UNBLOCK, &SigMask, nullptr);
 
     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, from sysexits.h.
       if (Sig == SIGPIPE)
-        exit(EX_IOERR);
+        if (SignalHandlerFunctionType CurrentPipeFunction = PipeSignalFunction)
+          CurrentPipeFunction();
 
       raise(Sig);   // Execute the default handler.
       return;
    }
   }
 
   // Otherwise if it is a fault (like SEGV) run any handler.
   llvm::sys::RunSignalHandlers();
void llvm::sys::SetInterruptFunction(void (*IF)()) {
   RegisterHandlers();
 }
 
 void llvm::sys::SetInfoSignalFunction(void (*Handler)()) {
   InfoSignalFunction.exchange(Handler);
   RegisterHandlers();
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  PipeSignalFunction.exchange(Handler);
+  RegisterHandlers();
+}
+
 // The public API
 bool llvm::sys::RemoveFileOnSignal(StringRef Filename,
                                    std::string* ErrMsg) {
   // Ensure that cleanup will occur as soon as one file is added.
   static ManagedStatic<FilesToRemoveCleanup> FilesToRemoveCleanup;
   *FilesToRemoveCleanup;
   FileToRemoveList::insert(FilesToRemove, Filename.str());
   RegisterHandlers();

llvm/lib/Support/Windows/Signals.inc

void llvm::sys::SetInterruptFunction(void (*IF)()) {
   InterruptFunction = IF;
   LeaveCriticalSection(&CriticalSection);
 }
 
 void llvm::sys::SetInfoSignalFunction(void (*Handler)()) {
   // Unimplemented.
 }
 
+void llvm::sys::SetPipeSignalFunction(void (*Handler)()) {
+  // Unimplemented.
+}
 
 /// Add a function to be called when a signal is delivered to the process. The
 /// handler can have a cookie passed to it to identify what instance of the
 /// handler it is.
 void llvm::sys::AddSignalHandler(sys::SignalHandlerCallback FnPtr,
                                  void *Cookie) {
   insertSignalHandler(FnPtr, Cookie);
   RegisterHandler();

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   ParallelTest.cpp
   Path.cpp
   ProcessTest.cpp
   ProgramTest.cpp
   RegexTest.cpp
   ReverseIterationTest.cpp
   ReplaceFileTest.cpp
   ScaledNumberTest.cpp
+  SignalsTest.cpp
   SourceMgrTest.cpp
   SpecialCaseListTest.cpp
   StringPool.cpp
   SwapByteOrderTest.cpp
   SymbolRemappingReaderTest.cpp
   TarWriterTest.cpp
   TargetParserTest.cpp
   TaskQueueTest.cpp

llvm/unittests/Support/SignalsTest.cpp

+//========- unittests/Support/SignalsTest.cpp - Signal handling test =========//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#if !defined(_WIN32)
+#include <unistd.h>
+#include <sysexits.h>
+#endif // !defined(_WIN32)
+
+#include "llvm/Support/Signals.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+#if !defined(_WIN32)
+TEST(SignalTest, IgnoreMultipleSIGPIPEs) {
+  // Ignore SIGPIPE.
+  signal(SIGPIPE, SIG_IGN);
+
+  // Disable exit-on-SIGPIPE.
+  sys::SetPipeSignalFunction(nullptr);
+
+  // Create unidirectional read/write pipes.
+  int fds[2];
+  int err = pipe(fds);
+  if (err != 0)
+    return; // If we can't make pipes, this isn't testing anything.
+
+  // Close the read pipe.
+  close(fds[0]);
+
+  // Attempt to write to the write pipe. Currently we're asserting that the
+  // write fails, which isn't great.
+  //
+  // What we really want is a death test that checks that this block exits
+  // with a special exit "success" code, as opposed to unexpectedly exiting due
+  // to a kill-by-SIGNAL or due to the default SIGPIPE handler.
+  //
+  // Unfortunately llvm's unit tests aren't set up to support death tests well.
+  // For one, death tests are flaky in a multithreaded context. And sigactions
+  // inherited from llvm-lit interfere with what's being tested.
+  const void *buf = (const void *)&fds;
+  err = write(fds[1], buf, 1);
+  ASSERT_EQ(err, -1);
+  err = write(fds[1], buf, 1);
+  ASSERT_EQ(err, -1);
+}
+#endif // !defined(_WIN32)