[llvm] r332429 - Revert "Signal handling should be signal-safe"
JF Bastien via llvm-commits
llvm-commits at lists.llvm.org
Tue May 15 21:36:37 PDT 2018
Author: jfb
Date: Tue May 15 21:36:37 2018
New Revision: 332429
URL: http://llvm.org/viewvc/llvm-project?rev=332429&view=rev
Log:
Revert "Signal handling should be signal-safe"
Some bots don't have double-pointer width compare-and-exchange. Revert for now.q
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=332429&r1=332428&r2=332429&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/Signals.h (original)
+++ llvm/trunk/include/llvm/Support/Signals.h Tue May 15 21:36:37 2018
@@ -56,12 +56,10 @@ 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(SignalHandlerCallback FnPtr, void *Cookie);
+ void AddSignalHandler(void (*FnPtr)(void *), 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=332429&r1=332428&r2=332429&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Signals.cpp (original)
+++ llvm/trunk/lib/Support/Signals.cpp Tue May 15 21:36:37 2018
@@ -36,42 +36,19 @@
using namespace llvm;
-// Use explicit storage to avoid accessing cl::opt in a signal handler.
-static bool DisableSymbolicationFlag = false;
-static cl::opt<bool, true>
+static cl::opt<bool>
DisableSymbolication("disable-symbolication",
cl::desc("Disable symbolizing crash backtraces."),
- cl::location(DisableSymbolicationFlag), cl::Hidden);
+ cl::init(false), 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<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;
- }
- llvm_unreachable("too many signal callbacks already registered");
+static ManagedStatic<std::vector<std::pair<void (*)(void *), void *>>>
+ CallBacksToRun;
+void sys::RunSignalHandlers() {
+ if (!CallBacksToRun.isConstructed())
+ return;
+ for (auto &I : *CallBacksToRun)
+ I.first(I.second);
+ CallBacksToRun->clear();
}
static bool findModulesAndOffsets(void **StackTrace, int Depth,
@@ -91,7 +68,7 @@ static FormattedNumber format_ptr(void *
LLVM_ATTRIBUTE_USED
static bool printSymbolizedStackTrace(StringRef Argv0, void **StackTrace,
int Depth, llvm::raw_ostream &OS) {
- if (DisableSymbolicationFlag)
+ if (DisableSymbolication)
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=332429&r1=332428&r2=332429&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Unix/Signals.inc (original)
+++ llvm/trunk/lib/Support/Unix/Signals.inc Tue May 15 21:36:37 2018
@@ -11,27 +11,6 @@
// 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"
@@ -81,119 +60,12 @@ using namespace llvm;
static RETSIGTYPE SignalHandler(int Sig); // defined below.
-/// The function to call if ctrl-c is pressed.
-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);
- }
+static ManagedStatic<sys::SmartMutex<true> > SignalsMutex;
- // 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);
-
- // 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);
+/// The function to call if ctrl-c is pressed.
+static void (*InterruptFunction)() = 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 ManagedStatic<std::vector<std::string>> FilesToRemove;
static StringRef Argv0;
@@ -222,12 +94,13 @@ static const int KillSigs[] = {
#endif
};
-static std::atomic<unsigned> NumRegisteredSignals = ATOMIC_VAR_INIT(0);
+static unsigned NumRegisteredSignals = 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
@@ -259,24 +132,18 @@ static void CreateSigAltStack() {
static void CreateSigAltStack() {}
#endif
-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);
+static void RegisterHandlers() {
+ sys::SmartScopedLock<true> Guard(*SignalsMutex);
// If the handlers are already registered, we're done.
- if (NumRegisteredSignals.load() != 0)
- return;
+ if (NumRegisteredSignals != 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) {
- unsigned Index = NumRegisteredSignals.load();
- assert(Index < array_lengthof(RegisteredSignalInfo) &&
+ assert(NumRegisteredSignals < array_lengthof(RegisteredSignalInfo) &&
"Out of space for signal handlers!");
struct sigaction NewHandler;
@@ -286,8 +153,9 @@ static void RegisterHandlers() { // Not
sigemptyset(&NewHandler.sa_mask);
// Install the new handler, save the old one in RegisteredSignalInfo.
- sigaction(Signal, &NewHandler, &RegisteredSignalInfo[Index].SA);
- RegisteredSignalInfo[Index].SigNo = Signal;
+ sigaction(Signal, &NewHandler,
+ &RegisteredSignalInfo[NumRegisteredSignals].SA);
+ RegisteredSignalInfo[NumRegisteredSignals].SigNo = Signal;
++NumRegisteredSignals;
};
@@ -299,16 +167,44 @@ static void RegisterHandlers() { // Not
static void UnregisterHandlers() {
// Restore all of the signal handlers to how they were before we showed up.
- for (unsigned i = 0, e = NumRegisteredSignals.load(); i != e; ++i) {
+ for (unsigned i = 0, e = NumRegisteredSignals; i != e; ++i)
sigaction(RegisteredSignalInfo[i].SigNo,
&RegisteredSignalInfo[i].SA, nullptr);
- --NumRegisteredSignals;
- }
+ NumRegisteredSignals = 0;
}
-/// Process the FilesToRemove list.
+
+/// 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.
static void RemoveFilesToRemove() {
- FileToRemoveList::removeAllFiles(FilesToRemove);
+ // 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);
+ }
}
// The signal handler that runs.
@@ -325,13 +221,20 @@ 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 (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
- return OldInterruptFunction();
+ if (InterruptFunction) {
+ void (*IF)() = InterruptFunction;
+ Guard.unlock();
+ InterruptFunction = nullptr;
+ IF(); // run the interrupt function.
+ return;
+ }
+ Guard.unlock();
raise(Sig); // Execute the default handler.
return;
}
@@ -351,36 +254,45 @@ static RETSIGTYPE SignalHandler(int Sig)
}
void llvm::sys::RunInterruptHandlers() {
+ sys::SmartScopedLock<true> Guard(*SignalsMutex);
RemoveFilesToRemove();
}
void llvm::sys::SetInterruptFunction(void (*IF)()) {
- InterruptFunction.exchange(IF);
+ {
+ sys::SmartScopedLock<true> Guard(*SignalsMutex);
+ InterruptFunction = IF;
+ }
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());
+ {
+ sys::SmartScopedLock<true> Guard(*SignalsMutex);
+ FilesToRemove->push_back(Filename);
+ }
+
RegisterHandlers();
return false;
}
// The public API
void llvm::sys::DontRemoveFileOnSignal(StringRef Filename) {
- FileToRemoveList::erase(FilesToRemove, Filename.str());
+ 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);
}
/// 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) { // Signal-safe.
- insertSignalHandler(FnPtr, Cookie);
+void llvm::sys::AddSignalHandler(void (*FnPtr)(void *), void *Cookie) {
+ CallBacksToRun->push_back(std::make_pair(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=332429&r1=332428&r2=332429&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Windows/Signals.inc (original)
+++ llvm/trunk/lib/Support/Windows/Signals.inc Tue May 15 21:36:37 2018
@@ -560,9 +560,8 @@ 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(sys::SignalHandlerCallback FnPtr,
- void *Cookie) {
- insertSignalHandler(FnPtr, Cookie);
+void llvm::sys::AddSignalHandler(void (*FnPtr)(void *), void *Cookie) {
+ CallBacksToRun->push_back(std::make_pair(FnPtr, Cookie));
RegisterHandler();
LeaveCriticalSection(&CriticalSection);
}
More information about the llvm-commits
mailing list