Signal handling should be signal-safe

[jfb]

Summary

Before this patch, signal handling wasn't signal safe. This leads to real-world
crashes. It used ManagedStatic inside of signals, this can allocate and can lead
to unexpected state when a signal occurs during llvm_shutdown (because
llvm_shutdown destroys the ManagedStatic). It also used cl::opt without custom
backing storage. Some de-allocation was performed as well. Acquiring a lock in a
signal handler is also a great way to deadlock.

We can't just disable signals on llvm_shutdown because the signals might do
useful work during that shutdown. We also can't just disable llvm_shutdown for
programs (instead of library uses of clang) because we'd have to then mark the
pointers as not leaked and make sure all the ManagedStatic uses are OK to leak
and remain so.

Move all of the code to lock-free datastructures instead, and avoid having any
of them in an inconsistent state. I'm not trying to be fancy, I'm not using any
explicit memory order because this code isn't hot. The only purpose of the
atomics is to guarantee that a signal firing on the same or a different thread
doesn't see an inconsistent state and crash. In some cases we might miss some
state (for example, we might fail to delete a temporary file), but that's fine.

Note that I haven't touched any of the backtrace support despite it not
technically being totally signal-safe. When that code is called we know
something bad is up and we don't expect to continue execution, so calling
something that e.g. sets errno is the least of our problems.

A similar patch should be applied to lib/Support/Windows/Signals.inc, but that
can be done separately.

rdar://problem/28010281

[dexonsmith]

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

lib/Support/Unix/Signals.inc

90 ↗

(On Diff #146722)

You've said that removing is not signal-safe, but removal itself is; that's not computing for me. It looks like insert and unregister are not signal-save, but removeAll is signal-safe -- should you use that terminology here?

109 ↗

(On Diff #146722)

I think current style guidelines for functions are lowerCamelCase, so this should be "insert".

121–122 ↗

(On Diff #146722)

"unregister" instead "Unregister"

133 ↗

(On Diff #146722)

"removeAll" instead of "RemoveAll"

166 ↗

(On Diff #146722)

Current style doesn't have us repeating the name of what's being documented.

If you're just matching the style in the file you can do a prep NFC commit that fixes the style elsewhere, but either way let's avoid the redundancy in the new code.

256 ↗

(On Diff #146722)

Why did you lambda-fy this? (If it's unrelated to the patch, can you split it out into a separate NFC commit?)

Also, I would prefer spelling this as registerHandler (lower camel case) since it's callable.

[steven_wu]

I think it is easier to unregister all the signal handlers in the beginning of the llvm_shutdown() since you should be done with all the work when you call llvm_shutdown(). This can also allow safe access ManagedStatics in signal handler, which I don't know if we have more of those other than the lock you are trying to fix. The only difference might be that if an interrupt is triggered when freeing ManagedStatics, llvm signal handler will not be used.

[jfb]

(Will address code changes separately)

In D46858#1098806, @dexonsmith wrote:

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

Hmm I don't think so, at least not a reliable test: most of the time the test wouldn't hit a dangerous interleaving. We could try to make the bad concurrent interleaving more likely, or loop a bunch so it's more likely to hit the race because it tries more, but I think fundamentally it would be a flaky "success" test, and mostly a test that does nothing. Were the code to start being broken, it would become a flaky failure.

In D46858#1098833, @steven_wu wrote:

I think it is easier to unregister all the signal handlers in the beginning of the llvm_shutdown() since you should be done with all the work when you call llvm_shutdown().

We might want to do this, but I'm worried that llvm_shutdown (and the ManagedStatic dtors it calls) could be used for more than just memory reclamation. If that's the case then it could crash, and then that wouldn't call signal handlers for e.g. stack traces on platforms that wanted it. If it starts taking a while then the CTRL-C handler wouldn't run (which might be fine... Presumably the tmp files are gone?).

I'm OK doing this, but I want to point out potential caveats and see what others think. It would definitely simplify some of the issues.

This can also allow safe access ManagedStatics in signal handler, which I don't know if we have more of those other than the lock you are trying to fix. The only difference might be that if an interrupt is triggered when freeing ManagedStatics, llvm signal handler will not be used.

Using ManagedStatic from a signal handler is fraught with peril:

• It'll call new on first use. We can guard on isConstructed(), but that's problematic if llvm_shutdown is running (assuming we keep signals on when llvm_shutdown runs). We can force ourselves to always call new when the handler is first installed, but that's a bit fragile (but totally OK).
• We'll then get a mutex, and we can't grab that mutex from the signal handler, because it the thread handling the signal held the mutex we get a (single-threaded!) deadlock.
• We could spin a dedicated signal handler thread, but that seems like overkill.
• We could spin a dedicated "do signal unsafe work" thread and post the work to it from the handler, but that's also overkill and would also require lock-free magic (plus waiting for the thread to finish its work before exiting).

It seems pretty simple to have lock-free stuff instead. ManagedStatic gives the feeling that everything is OK, just use a std::vector and all, but it's really not! Imagine that vector was in the process of growing and moving its content when the signal handler kicks in... You'd observe invalid state for sure. Lock-free code, while tricky and slightly ugly, makes it pretty obvious that everything is not OK, and you can't just use std::vector (or even std::atomic<std::pair<int, int>> because that's not even a thing).

[dexonsmith]

In D46858#1098865, @jfb wrote:

(Will address code changes separately)

In D46858#1098806, @dexonsmith wrote:

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

Hmm I don't think so, at least not a reliable test: most of the time the test wouldn't hit a dangerous interleaving. We could try to make the bad concurrent interleaving more likely, or loop a bunch so it's more likely to hit the race because it tries more, but I think fundamentally it would be a flaky "success" test, and mostly a test that does nothing. Were the code to start being broken, it would become a flaky failure.

Could you manufacture a deterministic (and dangerous) interleaving, by having the signal handler raise a signal on itself partway through something?

[jfb]

I committed the two requests NFC changes. Will address other changes tomorrow.

In D46858#1098873, @dexonsmith wrote:

In D46858#1098865, @jfb wrote:

(Will address code changes separately)

In D46858#1098806, @dexonsmith wrote:

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

Hmm I don't think so, at least not a reliable test: most of the time the test wouldn't hit a dangerous interleaving. We could try to make the bad concurrent interleaving more likely, or loop a bunch so it's more likely to hit the race because it tries more, but I think fundamentally it would be a flaky "success" test, and mostly a test that does nothing. Were the code to start being broken, it would become a flaky failure.

Could you manufacture a deterministic (and dangerous) interleaving, by having the signal handler raise a signal on itself partway through something?

I don't think that'll do what we want: we want regular code to raise at a dangerous spot. We *can* raise from the signal handler, but that's not the race that was causing us trouble.

[dexonsmith]

In D46858#1098875, @jfb wrote:

I committed the two requests NFC changes. Will address other changes tomorrow.

SGTM.

In D46858#1098873, @dexonsmith wrote:

In D46858#1098865, @jfb wrote:

(Will address code changes separately)

In D46858#1098806, @dexonsmith wrote:

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

Hmm I don't think so, at least not a reliable test: most of the time the test wouldn't hit a dangerous interleaving. We could try to make the bad concurrent interleaving more likely, or loop a bunch so it's more likely to hit the race because it tries more, but I think fundamentally it would be a flaky "success" test, and mostly a test that does nothing. Were the code to start being broken, it would become a flaky failure.

Could you manufacture a deterministic (and dangerous) interleaving, by having the signal handler raise a signal on itself partway through something?

I don't think that'll do what we want: we want regular code to raise at a dangerous spot. We *can* raise from the signal handler, but that's not the race that was causing us trouble.

I wondered if that would be a similar enough race that it would require the same sort of careful work, and thus could protect against "simplifying" the code later. But if that won't do it I suppose we'll just have to trust people to read comments.

[dexonsmith]

Since you've answered my question about testing, this LGTM after you address the other feedback. Thanks for splitting out the two commits.

[jfb]

• Rebase, address comments.

[jfb]

Actually I had a bit of time and addressed all outstanding comments from @dexonsmith.

On de-registering signal handlers at llvm_shutdown time: how about I post a separate patch for it? Seems like it could be good, but I want to have a focused patch to do so where we can have other chime in on the effect it could have.WDYT @steven_wu?

Leaving this open for a bit in case other want to chime in.

[jfb]

• Misc updates, cleanup

[jfb]

Had to revert because some bots don't have double-pointer-wide atomics.

[jfb]

• Fix double-wide CAS

lib/Support/Signals.cpp

53 ↗

(On Diff #147114)

This is the update, avoids double-wide CAS by just using this flag.

[dexonsmith]

I found a couple more nits to pick while reading the new version of the lock-free code. LGTM once you fix those.

lib/Support/Signals.cpp

64 ↗

(On Diff #147114)

Reverse condition and early continue to reduce nesting?

80 ↗

(On Diff #147114)

Reverse condition and early continue to reduce nesting?

87 ↗

(On Diff #147114)

The optimizer is free to remove this loop exit. I think report_fatal_error would be more appropriate in this case.

lib/Support/Unix/Signals.inc

133–134 ↗

(On Diff #147114)

Early continues to reduce nesting?

[jfb]

• Address comments

[jfb]

Comments addressed.

[jfb]

Update landed in r332496.

llvm/trunk/include/llvm/Support/Signals.h

namespace sys {
   void DisableSystemDialogsOnCrash();
 
   /// Print the stack trace using the given \c raw_ostream object.
   void PrintStackTrace(raw_ostream &OS);
 
   // Run all registered signal handlers.
   void RunSignalHandlers();
 
+  using SignalHandlerCallback = void (*)(void *);
+
   /// Add a function to be called when an abort/kill 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 AddSignalHandler(void (*FnPtr)(void *), void *Cookie);
+  void AddSignalHandler(SignalHandlerCallback FnPtr, void *Cookie);
 
   /// This function registers a function to be called when the user "interrupts"
   /// the program (typically by pressing ctrl-c).  When the user interrupts the
   /// program, the specified interrupt function is called instead of the program
   /// being killed, and the interrupt function automatically disabled.  Note
   /// that interrupt functions are not allowed to call any non-reentrant
   /// functions.  An null interrupt function pointer disables the current
   /// installed function.  Note also that the handler may be executed on a
   /// different thread on some platforms.
   /// Register a function to be called when ctrl-c is pressed.
   void SetInterruptFunction(void (*IF)());
 } // End sys namespace
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/Support/Signals.cpp

 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only TRULY operating system
 //===          independent code.
 //===----------------------------------------------------------------------===//
 
 using namespace llvm;
 
-static cl::opt<bool>
+// Use explicit storage to avoid accessing cl::opt in a signal handler.
+static bool DisableSymbolicationFlag = false;
+static cl::opt<bool, true>
     DisableSymbolication("disable-symbolication",
                          cl::desc("Disable symbolizing crash backtraces."),
-                         cl::init(false), cl::Hidden);
+                         cl::location(DisableSymbolicationFlag), cl::Hidden);
 
-static ManagedStatic<std::vector<std::pair<void (*)(void *), void *>>>
-    CallBacksToRun;
-void sys::RunSignalHandlers() {
-  if (!CallBacksToRun.isConstructed())
+// Callbacks to run in signal handler must be lock-free because a signal handler
+// could be running as we add new callbacks. We don't add unbounded numbers of
+// callbacks, an array is therefore sufficient.
+// Assume two pointers are always lock-free.
+struct LLVM_ALIGNAS(sizeof(void *) * 2) CallbackAndCookie {
+  sys::SignalHandlerCallback Callback;
+  void *Cookie;
+};
+static constexpr size_t MaxSignalHandlerCallbacks = 8;
+static std::atomic<CallbackAndCookie> CallBacksToRun[MaxSignalHandlerCallbacks];
+
+void sys::RunSignalHandlers() { // Signal-safe.
+  for (size_t I = 0; I < MaxSignalHandlerCallbacks; ++I) {
+    CallbackAndCookie DoNothing{nullptr, nullptr};
+    auto Entry = CallBacksToRun[I].exchange(DoNothing);
+    if (Entry.Callback)
+      (*Entry.Callback)(Entry.Cookie);
+  }
+}
+
+static void insertSignalHandler(sys::SignalHandlerCallback FnPtr,
+                                void *Cookie) { // Signal-safe.
+  CallbackAndCookie Entry = CallbackAndCookie{FnPtr, Cookie};
+  for (size_t I = 0; I < MaxSignalHandlerCallbacks; ++I) {
+    CallbackAndCookie Expected{nullptr, nullptr};
+    if (CallBacksToRun[I].compare_exchange_strong(Expected, Entry))
       return;
-  for (auto &I : *CallBacksToRun)
-    I.first(I.second);
-  CallBacksToRun->clear();
+  }
+  llvm_unreachable("too many signal callbacks already registered");
 }
 
 static bool findModulesAndOffsets(void **StackTrace, int Depth,
                                   const char **Modules, intptr_t *Offsets,
                                   const char *MainExecutableName,
                                   StringSaver &StrPool);
 
 /// Format a pointer value as hexadecimal. Zero pad it out so its always the
 /// same width.
 static FormattedNumber format_ptr(void *PC) {
   // Each byte is two hex digits plus 2 for the 0x prefix.
   unsigned PtrWidth = 2 + 2 * sizeof(void *);
   return format_hex((uint64_t)PC, PtrWidth);
 }
 
 /// Helper that launches llvm-symbolizer and symbolizes a backtrace.
 LLVM_ATTRIBUTE_USED
 static bool printSymbolizedStackTrace(StringRef Argv0, void **StackTrace,
                                       int Depth, llvm::raw_ostream &OS) {
-  if (DisableSymbolication)
+  if (DisableSymbolicationFlag)
     return false;
 
   // Don't recursively invoke the llvm-symbolizer binary.
   if (Argv0.find("llvm-symbolizer") != std::string::npos)
     return false;
 
   // FIXME: Subtract necessary number from StackTrace entries to turn return addresses
   // into actual instruction addresses.

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

 //===- Signals.cpp - Generic Unix Signals Implementation -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines some helpful functions for dealing with the possibility of
 // Unix signals occurring while your program is running.
 //
 //===----------------------------------------------------------------------===//
+//
+// This file is extremely careful to only do signal-safe things while in a
+// signal handler. In particular, memory allocation and acquiring a mutex
+// while in a signal handler should never occur. ManagedStatic isn't usable from
+// a signal handler for 2 reasons:
+//
+//  1. Creating a new one allocates.
+//  2. The signal handler could fire while llvm_shutdown is being processed, in
+//     which case the ManagedStatic is in an unknown state because it could
+//     already have been destroyed, or be in the process of being destroyed.
+//
+// Modifying the behavior of the signal handlers (such as registering new ones)
+// can acquire a mutex, but all this guarantees is that the signal handler
+// behavior is only modified by one thread at a time. A signal handler can still
+// fire while this occurs!
+//
+// Adding work to a signal handler requires lock-freedom (and assume atomics are
+// always lock-free) because the signal handler could fire while new work is
+// being added.
+//
+//===----------------------------------------------------------------------===//
 
 #include "Unix.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Config/config.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Format.h"

 #undef HAVE__UNWIND_BACKTRACE
 #endif
 #endif
 
 using namespace llvm;
 
 static RETSIGTYPE SignalHandler(int Sig);  // defined below.
 
-static ManagedStatic<sys::SmartMutex<true> > SignalsMutex;
-
 /// The function to call if ctrl-c is pressed.
-static void (*InterruptFunction)() = nullptr;
+using InterruptFunctionType = void (*)();
+static std::atomic<InterruptFunctionType> InterruptFunction =
+    ATOMIC_VAR_INIT(nullptr);
+
+/// 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);
+  std::atomic<FileToRemoveList *> Next = ATOMIC_VAR_INIT(nullptr);
+
+  FileToRemoveList() = default;
+  // Not signal-safe.
+  FileToRemoveList(const std::string &str) : Filename(strdup(str.c_str())) {}
+
+public:
+  // Not signal-safe.
+  ~FileToRemoveList() {
+    if (FileToRemoveList *N = Next.exchange(nullptr))
+      delete N;
+    if (char *F = Filename.exchange(nullptr))
+      free(F);
+  }
+
+  // Not signal-safe.
+  static void insert(std::atomic<FileToRemoveList *> &Head,
+                     const std::string &Filename) {
+    // Insert the new file at the end of the list.
+    FileToRemoveList *NewHead = new FileToRemoveList(Filename);
+    std::atomic<FileToRemoveList *> *InsertionPoint = &Head;
+    FileToRemoveList *OldHead = nullptr;
+    while (!InsertionPoint->compare_exchange_strong(OldHead, NewHead)) {
+      InsertionPoint = &OldHead->Next;
+      OldHead = nullptr;
+    }
+  }
+
+  // Not signal-safe.
+  static void erase(std::atomic<FileToRemoveList *> &Head,
+                    const std::string &Filename) {
+    // Use a lock to avoid concurrent erase: the comparison would access
+    // free'd memory.
+    static ManagedStatic<sys::SmartMutex<true>> Lock;
+    sys::SmartScopedLock<true> Writer(*Lock);
+
+    for (FileToRemoveList *Current = Head.load(); Current;
+         Current = Current->Next.load()) {
+      if (char *OldFilename = Current->Filename.load()) {
+        if (OldFilename == Filename) {
+          // Leave an empty filename.
+          OldFilename = Current->Filename.exchange(nullptr);
+          // The filename might have become null between the time we
+          // compared it and we exchanged it.
+          if (OldFilename)
+            free(OldFilename);
+        }
+      }
+    }
+  }
+
+  // Signal-safe.
+  static void removeAllFiles(std::atomic<FileToRemoveList *> &Head) {
+    // If cleanup were to occur while we're removing files we'd have a bad time.
+    // Make sure we're OK by preventing cleanup from doing anything while we're
+    // removing files. If cleanup races with us and we win we'll have a leak,
+    // but we won't crash.
+    FileToRemoveList *OldHead = Head.exchange(nullptr);
+
+    for (FileToRemoveList *currentFile = OldHead; currentFile;
+         currentFile = currentFile->Next.load()) {
+      // If erasing was occuring while we're trying to remove files we'd look
+      // at free'd data. Take away the path and put it back when done.
+      if (char *path = currentFile->Filename.exchange(nullptr)) {
+        // Get the status so we can determine if it's a file or directory. If we
+        // can't stat the file, ignore it.
+        struct stat buf;
+        if (stat(path, &buf) != 0)
+          continue;
+
+        // If this is not a regular file, ignore it. We want to prevent removal
+        // of special files like /dev/null, even if the compiler is being run
+        // with the super-user permissions.
+        if (!S_ISREG(buf.st_mode))
+          continue;
+
+        // Otherwise, remove the file. We ignore any errors here as there is
+        // nothing else we can do.
+        unlink(path);
 
-static ManagedStatic<std::vector<std::string>> FilesToRemove;
+        // We're done removing the file, erasing can safely proceed.
+        currentFile->Filename.exchange(path);
+      }
+    }
+
+    // We're done removing files, cleanup can safely proceed.
+    Head.exchange(OldHead);
+  }
+};
+static std::atomic<FileToRemoveList *> FilesToRemove = ATOMIC_VAR_INIT(nullptr);
+
+/// Clean up the list in a signal-friendly manner.
+/// Recall that signals can fire during llvm_shutdown. If this occurs we should
+/// either clean something up or nothing at all, but we shouldn't crash!
+struct FilesToRemoveCleanup {
+  // Not signal-safe.
+  ~FilesToRemoveCleanup() {
+    FileToRemoveList *Head = FilesToRemove.exchange(nullptr);
+    if (Head)
+      delete Head;
+  }
+};
 
 static StringRef Argv0;
 
 // Signals that represent requested termination. There's no bug or failure, or
 // if there is, it's not our direct responsibility. For whatever reason, our
 // continued execution is no longer desirable.
 static const int IntSigs[] = {
   SIGHUP, SIGINT, SIGPIPE, SIGTERM, SIGUSR1, SIGUSR2

 #ifdef SIGXFSZ
   , SIGXFSZ
 #endif
 #ifdef SIGEMT
   , SIGEMT
 #endif
 };
 
-static unsigned NumRegisteredSignals = 0;
+static std::atomic<unsigned> NumRegisteredSignals = ATOMIC_VAR_INIT(0);
 static struct {
   struct sigaction SA;
   int SigNo;
 } RegisteredSignalInfo[array_lengthof(IntSigs) + array_lengthof(KillSigs)];
 
-
 #if defined(HAVE_SIGALTSTACK)
 // Hold onto both the old and new alternate signal stack so that it's not
 // reported as a leak. We don't make any attempt to remove our alt signal
 // stack if we remove our signal handlers; that can't be done reliably if
 // someone else is also trying to do the same thing.
 static stack_t OldAltStack;
 static void* NewAltStackPointer;
 
static void CreateSigAltStack() {
   AltStack.ss_size = AltStackSize;
   if (sigaltstack(&AltStack, &OldAltStack) != 0)
     free(AltStack.ss_sp);
 }
 #else
 static void CreateSigAltStack() {}
 #endif
 
-static void RegisterHandlers() {
-  sys::SmartScopedLock<true> Guard(*SignalsMutex);
+static void RegisterHandlers() { // Not signal-safe.
+  // The mutex prevents other threads from registering handlers while we're
+  // doing it. We also have to protect the handlers and their count because
+  // a signal handler could fire while we're registeting handlers.
+  static ManagedStatic<sys::SmartMutex<true>> SignalHandlerRegistrationMutex;
+  sys::SmartScopedLock<true> Guard(*SignalHandlerRegistrationMutex);
 
   // If the handlers are already registered, we're done.
-  if (NumRegisteredSignals != 0) return;
+  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) {
-    assert(NumRegisteredSignals < array_lengthof(RegisteredSignalInfo) &&
+    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[NumRegisteredSignals].SA);
-    RegisteredSignalInfo[NumRegisteredSignals].SigNo = Signal;
+    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; i != e; ++i)
+  for (unsigned i = 0, e = NumRegisteredSignals.load(); i != e; ++i) {
     sigaction(RegisteredSignalInfo[i].SigNo,
               &RegisteredSignalInfo[i].SA, nullptr);
-  NumRegisteredSignals = 0;
+    --NumRegisteredSignals;
+  }
 }
 
-
-/// Process the FilesToRemove list. This function should be called with the
-/// SignalsMutex lock held. NB: This must be an async signal safe function. It
-/// cannot allocate or free memory, even in debug builds.
+/// Process the FilesToRemove list.
 static void RemoveFilesToRemove() {
-  // Avoid constructing ManagedStatic in the signal handler.
-  // If FilesToRemove is not constructed, there are no files to remove.
-  if (!FilesToRemove.isConstructed())
-    return;
-
-  // We avoid iterators in case of debug iterators that allocate or release
-  // memory.
-  std::vector<std::string>& FilesToRemoveRef = *FilesToRemove;
-  for (unsigned i = 0, e = FilesToRemoveRef.size(); i != e; ++i) {
-    const char *path = FilesToRemoveRef[i].c_str();
-
-    // Get the status so we can determine if it's a file or directory. If we
-    // can't stat the file, ignore it.
-    struct stat buf;
-    if (stat(path, &buf) != 0)
-      continue;
-
-    // If this is not a regular file, ignore it. We want to prevent removal of
-    // special files like /dev/null, even if the compiler is being run with the
-    // super-user permissions.
-    if (!S_ISREG(buf.st_mode))
-      continue;
-
-    // Otherwise, remove the file. We ignore any errors here as there is nothing
-    // else we can do.
-    unlink(path);
-  }
+  FileToRemoveList::removeAllFiles(FilesToRemove);
 }
 
 // The signal handler that runs.
 static RETSIGTYPE SignalHandler(int Sig) {
   // Restore the signal behavior to default, so that the program actually
   // crashes when we return and the signal reissues.  This also ensures that if
   // we crash in our signal handler that the program will terminate immediately
   // instead of recursing in the signal handler.
   UnregisterHandlers();
 
   // Unmask all potentially blocked kill signals.
   sigset_t SigMask;
   sigfillset(&SigMask);
   sigprocmask(SIG_UNBLOCK, &SigMask, nullptr);
 
   {
-    unique_lock<sys::SmartMutex<true>> Guard(*SignalsMutex);
     RemoveFilesToRemove();
 
     if (std::find(std::begin(IntSigs), std::end(IntSigs), Sig)
         != std::end(IntSigs)) {
-      if (InterruptFunction) {
-        void (*IF)() = InterruptFunction;
-        Guard.unlock();
-        InterruptFunction = nullptr;
-        IF();        // run the interrupt function.
-        return;
-      }
+      if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
+        return OldInterruptFunction();
 
-      Guard.unlock();
       raise(Sig);   // Execute the default handler.
       return;
    }
   }
 
   // Otherwise if it is a fault (like SEGV) run any handler.
   llvm::sys::RunSignalHandlers();
 
 #ifdef __s390__
   // On S/390, certain signals are delivered with PSW Address pointing to
   // *after* the faulting instruction.  Simply returning from the signal
   // handler would continue execution after that point, instead of
   // re-raising the signal.  Raise the signal manually in those cases.
   if (Sig == SIGILL || Sig == SIGFPE || Sig == SIGTRAP)
     raise(Sig);
 #endif
 }
 
 void llvm::sys::RunInterruptHandlers() {
-  sys::SmartScopedLock<true> Guard(*SignalsMutex);
   RemoveFilesToRemove();
 }
 
 void llvm::sys::SetInterruptFunction(void (*IF)()) {
-  {
-    sys::SmartScopedLock<true> Guard(*SignalsMutex);
-    InterruptFunction = IF;
-  }
+  InterruptFunction.exchange(IF);
   RegisterHandlers();
 }
 
 // The public API
 bool llvm::sys::RemoveFileOnSignal(StringRef Filename,
                                    std::string* ErrMsg) {
-  {
-    sys::SmartScopedLock<true> Guard(*SignalsMutex);
-    FilesToRemove->push_back(Filename);
-  }
-
+  // Ensure that cleanup will occur as soon as one file is added.
+  static ManagedStatic<FilesToRemoveCleanup> FilesToRemoveCleanup;
+  *FilesToRemoveCleanup;
+  FileToRemoveList::insert(FilesToRemove, Filename.str());
   RegisterHandlers();
   return false;
 }
 
 // The public API
 void llvm::sys::DontRemoveFileOnSignal(StringRef Filename) {
-  sys::SmartScopedLock<true> Guard(*SignalsMutex);
-  std::vector<std::string>::reverse_iterator RI =
-      find(reverse(*FilesToRemove), Filename);
-  std::vector<std::string>::iterator I = FilesToRemove->end();
-  if (RI != FilesToRemove->rend())
-    I = FilesToRemove->erase(RI.base()-1);
+  FileToRemoveList::erase(FilesToRemove, Filename.str());
 }
 
 /// 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(void (*FnPtr)(void *), void *Cookie) {
-  CallBacksToRun->push_back(std::make_pair(FnPtr, Cookie));
+void llvm::sys::AddSignalHandler(sys::SignalHandlerCallback FnPtr,
+                                 void *Cookie) { // Signal-safe.
+  insertSignalHandler(FnPtr, Cookie);
   RegisterHandlers();
 }
 
 #if defined(HAVE_BACKTRACE) && ENABLE_BACKTRACES && HAVE_LINK_H &&    \
     (defined(__linux__) || defined(__FreeBSD__) ||                             \
      defined(__FreeBSD_kernel__) || defined(__NetBSD__))
 struct DlIteratePhdrData {
   void **StackTrace;

llvm/trunk/lib/Support/Windows/Signals.inc

void llvm::sys::SetInterruptFunction(void (*IF)()) {
   InterruptFunction = IF;
   LeaveCriticalSection(&CriticalSection);
 }
 
 
 /// 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(void (*FnPtr)(void *), void *Cookie) {
-  CallBacksToRun->push_back(std::make_pair(FnPtr, Cookie));
+void llvm::sys::AddSignalHandler(sys::SignalHandlerCallback FnPtr,
+                                 void *Cookie) {
+  insertSignalHandler(FnPtr, Cookie);
   RegisterHandler();
   LeaveCriticalSection(&CriticalSection);
 }
 
 static void Cleanup() {
   if (CleanupExecuted)
     return;