[llvm] r332496 - Signal handling should be signal-safe
JF Bastien via llvm-commits
llvm-commits at lists.llvm.org
Wed May 16 10:25:35 PDT 2018
Author: jfb
Date: Wed May 16 10:25:35 2018
New Revision: 332496
URL: http://llvm.org/viewvc/llvm-project?rev=332496&view=rev
Log:
Signal handling should be signal-safe
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.
Fix r332428 which I reverted in r332429. I originally used double-wide CAS
because I was lazy, but some platforms use a runtime function for that which
thankfully failed to link (it would have been bad for signal handlers
otherwise). I use a separate flag to guard the data instead.
<rdar://problem/28010281>
Reviewers: dexonsmith
Subscribers: steven_wu, llvm-commits
Modified:
llvm/trunk/include/llvm/Support/Signals.h
llvm/trunk/lib/Support/Signals.cpp
llvm/trunk/lib/Support/Unix/Signals.inc
llvm/trunk/lib/Support/Windows/Signals.inc
Modified: llvm/trunk/include/llvm/Support/Signals.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/Signals.h?rev=332496&r1=332495&r2=332496&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/Signals.h (original)
+++ llvm/trunk/include/llvm/Support/Signals.h Wed May 16 10:25:35 2018
@@ -56,10 +56,12 @@ namespace sys {
// 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
Modified: llvm/trunk/lib/Support/Signals.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Signals.cpp?rev=332496&r1=332495&r2=332496&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Signals.cpp (original)
+++ llvm/trunk/lib/Support/Signals.cpp Wed May 16 10:25:35 2018
@@ -36,19 +36,55 @@
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;
+// 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.
+struct CallbackAndCookie {
+ sys::SignalHandlerCallback Callback;
+ void *Cookie;
+ enum class Status { Empty, Initializing, Initialized, Executing };
+ std::atomic<Status> Flag;
+};
+static constexpr size_t MaxSignalHandlerCallbacks = 8;
+static CallbackAndCookie CallBacksToRun[MaxSignalHandlerCallbacks];
+
+// Signal-safe.
void sys::RunSignalHandlers() {
- if (!CallBacksToRun.isConstructed())
+ for (size_t I = 0; I < MaxSignalHandlerCallbacks; ++I) {
+ auto &RunMe = CallBacksToRun[I];
+ auto Expected = CallbackAndCookie::Status::Initialized;
+ auto Desired = CallbackAndCookie::Status::Executing;
+ if (!RunMe.Flag.compare_exchange_strong(Expected, Desired))
+ continue;
+ (*RunMe.Callback)(RunMe.Cookie);
+ RunMe.Callback = nullptr;
+ RunMe.Cookie = nullptr;
+ RunMe.Flag.store(CallbackAndCookie::Status::Empty);
+ }
+}
+
+// Signal-safe.
+static void insertSignalHandler(sys::SignalHandlerCallback FnPtr,
+ void *Cookie) {
+ for (size_t I = 0; I < MaxSignalHandlerCallbacks; ++I) {
+ auto &SetMe = CallBacksToRun[I];
+ auto Expected = CallbackAndCookie::Status::Empty;
+ auto Desired = CallbackAndCookie::Status::Initializing;
+ if (!SetMe.Flag.compare_exchange_strong(Expected, Desired))
+ continue;
+ SetMe.Callback = FnPtr;
+ SetMe.Cookie = Cookie;
+ SetMe.Flag.store(CallbackAndCookie::Status::Initialized);
return;
- for (auto &I : *CallBacksToRun)
- I.first(I.second);
- CallBacksToRun->clear();
+ }
+ report_fatal_error("too many signal callbacks already registered");
}
static bool findModulesAndOffsets(void **StackTrace, int Depth,
@@ -68,7 +104,7 @@ static FormattedNumber format_ptr(void *
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.
Modified: llvm/trunk/lib/Support/Unix/Signals.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Unix/Signals.inc?rev=332496&r1=332495&r2=332496&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Unix/Signals.inc (original)
+++ llvm/trunk/lib/Support/Unix/Signals.inc Wed May 16 10:25:35 2018
@@ -11,6 +11,27 @@
// 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"
@@ -60,12 +81,119 @@ 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)
+ continue;
+ // 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);
+
+ // 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);
-static ManagedStatic<std::vector<std::string>> FilesToRemove;
+/// 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;
@@ -94,13 +222,12 @@ static const int KillSigs[] = {
#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
@@ -132,18 +259,24 @@ static void CreateSigAltStack() {
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;
@@ -153,9 +286,8 @@ static void RegisterHandlers() {
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;
};
@@ -167,44 +299,16 @@ static void RegisterHandlers() {
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.
@@ -221,20 +325,13 @@ static RETSIGTYPE SignalHandler(int Sig)
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;
}
@@ -254,45 +351,36 @@ static RETSIGTYPE SignalHandler(int Sig)
}
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();
}
Modified: llvm/trunk/lib/Support/Windows/Signals.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Signals.inc?rev=332496&r1=332495&r2=332496&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Windows/Signals.inc (original)
+++ llvm/trunk/lib/Support/Windows/Signals.inc Wed May 16 10:25:35 2018
@@ -560,8 +560,9 @@ void llvm::sys::SetInterruptFunction(voi
/// 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);
}
More information about the llvm-commits
mailing list