Index: include/llvm/Support/Signals.h =================================================================== --- include/llvm/Support/Signals.h +++ include/llvm/Support/Signals.h @@ -56,10 +56,12 @@ // 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 Index: lib/Support/Signals.cpp =================================================================== --- lib/Support/Signals.cpp +++ lib/Support/Signals.cpp @@ -36,19 +36,42 @@ using namespace llvm; -static cl::opt +// Use explicit storage to avoid accessing cl::opt in a signal handler. +static bool DisableSymbolicationFlag = false; +static cl::opt DisableSymbolication("disable-symbolication", cl::desc("Disable symbolizing crash backtraces."), - cl::init(false), cl::Hidden); - -static ManagedStatic>> - CallBacksToRun; -void sys::RunSignalHandlers() { - if (!CallBacksToRun.isConstructed()) - return; - for (auto &I : *CallBacksToRun) - I.first(I.second); - CallBacksToRun->clear(); + cl::location(DisableSymbolicationFlag), cl::Hidden); + +// 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 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; + } + llvm_unreachable("too many signal callbacks already registered"); } static bool findModulesAndOffsets(void **StackTrace, int Depth, @@ -68,7 +91,7 @@ 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. Index: lib/Support/Unix/Signals.inc =================================================================== --- lib/Support/Unix/Signals.inc +++ lib/Support/Unix/Signals.inc @@ -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 @@ static RETSIGTYPE SignalHandler(int Sig); // defined below. -static ManagedStatic > SignalsMutex; - /// The function to call if ctrl-c is pressed. -static void (*InterruptFunction)() = nullptr; +using InterruptFunctionType = void (*)(); +static std::atomic 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 Filename = ATOMIC_VAR_INIT(nullptr); + std::atomic 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 &Head, + const std::string &Filename) { + // Insert the new file at the end of the list. + FileToRemoveList *NewHead = new FileToRemoveList(Filename); + std::atomic *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 &Head, + const std::string &Filename) { + // Use a lock to avoid concurrent erase: the comparison would access + // free'd memory. + static ManagedStatic> Lock; + sys::SmartScopedLock 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); + } + } + } + } -static ManagedStatic> FilesToRemove; + // Signal-safe. + static void removeAllFiles(std::atomic &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 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; @@ -94,13 +222,12 @@ #endif }; -static unsigned NumRegisteredSignals = 0; +static std::atomic 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() {} #endif -static void RegisterHandlers() { - sys::SmartScopedLock 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> SignalHandlerRegistrationMutex; + sys::SmartScopedLock 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 @@ 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 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& 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 @@ sigprocmask(SIG_UNBLOCK, &SigMask, nullptr); { - unique_lock> 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 @@ } void llvm::sys::RunInterruptHandlers() { - sys::SmartScopedLock Guard(*SignalsMutex); RemoveFilesToRemove(); } void llvm::sys::SetInterruptFunction(void (*IF)()) { - { - sys::SmartScopedLock Guard(*SignalsMutex); - InterruptFunction = IF; - } + InterruptFunction.exchange(IF); RegisterHandlers(); } // The public API bool llvm::sys::RemoveFileOnSignal(StringRef Filename, std::string* ErrMsg) { - { - sys::SmartScopedLock Guard(*SignalsMutex); - FilesToRemove->push_back(Filename); - } - + // Ensure that cleanup will occur as soon as one file is added. + static ManagedStatic FilesToRemoveCleanup; + *FilesToRemoveCleanup; + FileToRemoveList::insert(FilesToRemove, Filename.str()); RegisterHandlers(); return false; } // The public API void llvm::sys::DontRemoveFileOnSignal(StringRef Filename) { - sys::SmartScopedLock Guard(*SignalsMutex); - std::vector::reverse_iterator RI = - find(reverse(*FilesToRemove), Filename); - std::vector::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(); } Index: lib/Support/Windows/Signals.inc =================================================================== --- lib/Support/Windows/Signals.inc +++ lib/Support/Windows/Signals.inc @@ -560,8 +560,9 @@ /// 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); }