aganea updated this revision to Diff 232854.
aganea marked 8 inline comments as done.
aganea added a subscriber: vsk.
aganea added a comment.
Addressed comments, and:
- Removed `CrashRecoveryContext::HandleCrash()` and `#pragma clang
handle_crash` which wasn't covered by any test and most likely deprecated by
`#pragma clang crash`
- Added refcounting to `CrashRecoveryContext::Enable/Disable` as well as lazy
installation of the exception handlers in `CrashRecoveryContext`.
- Partially reverted rG4624e83ce7b124545b55e45ba13f2d900ed65654
<https://reviews.llvm.org/rG4624e83ce7b124545b55e45ba13f2d900ed65654>/llvm/lib/Support/Unix/Signals.inc
to simplify checks for IntSigs (and to fix the issue mentioned here
<https://reviews.llvm.org/rG9a3f892d018238dce5181e458905311db8e682f5#856804>).
@vsk
All tests pass on Windows/MSVC2019/Clang9/10 and Linux/Clang9/GCC9.1.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70568/new/
https://reviews.llvm.org/D70568
Files:
clang/lib/Lex/Pragma.cpp
llvm/include/llvm/Support/CrashRecoveryContext.h
llvm/include/llvm/Support/Signals.h
llvm/lib/Support/CrashRecoveryContext.cpp
llvm/lib/Support/Unix/Signals.inc
llvm/lib/Support/Windows/Signals.inc
llvm/unittests/Support/CrashRecoveryTest.cpp
Index: llvm/unittests/Support/CrashRecoveryTest.cpp
===================================================================
--- llvm/unittests/Support/CrashRecoveryTest.cpp
+++ llvm/unittests/Support/CrashRecoveryTest.cpp
@@ -8,6 +8,8 @@
#include "llvm/Support/Compiler.h"
#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Signals.h"
#include "gtest/gtest.h"
#ifdef _WIN32
@@ -23,6 +25,8 @@
static void nullDeref() { *(volatile int *)0x10 = 0; }
static void incrementGlobal() { ++GlobalInt; }
static void llvmTrap() { LLVM_BUILTIN_TRAP; }
+static void incrementGlobalWithCookie(void*) { ++GlobalInt; }
+
TEST(CrashRecoveryTest, Basic) {
llvm::CrashRecoveryContext::Enable();
@@ -31,6 +35,7 @@
EXPECT_EQ(1, GlobalInt);
EXPECT_FALSE(CrashRecoveryContext().RunSafely(nullDeref));
EXPECT_FALSE(CrashRecoveryContext().RunSafely(llvmTrap));
+ llvm::CrashRecoveryContext::Disable();
}
struct IncrementGlobalCleanup : CrashRecoveryContextCleanup {
@@ -58,6 +63,56 @@
EXPECT_FALSE(CRC.RunSafely(nullDeref));
} // run cleanups
EXPECT_EQ(1, GlobalInt);
+ llvm::CrashRecoveryContext::Disable();
+}
+
+TEST(CrashRecoveryTest, DumpStackCleanup) {
+ SmallString<128> Filename;
+ std::error_code EC = sys::fs::createTemporaryFile("crash", "test", Filename);
+ EXPECT_FALSE(EC);
+ sys::RemoveFileOnSignal(Filename);
+ llvm::sys::AddSignalHandler(incrementGlobalWithCookie, nullptr);
+ GlobalInt = 0;
+ llvm::CrashRecoveryContext::Enable();
+ {
+ CrashRecoveryContext CRC;
+ CRC.DumpStackAndCleanupOnFailure = true;
+ EXPECT_TRUE(CRC.RunSafely(noop));
+ }
+ EXPECT_TRUE(sys::fs::exists(Filename));
+ EXPECT_EQ(GlobalInt, 0);
+ {
+ CrashRecoveryContext CRC;
+ CRC.DumpStackAndCleanupOnFailure = true;
+ EXPECT_FALSE(CRC.RunSafely(nullDeref));
+ EXPECT_NE(CRC.RetCode, 0);
+ }
+ EXPECT_FALSE(sys::fs::exists(Filename));
+ EXPECT_EQ(GlobalInt, 1);
+ llvm::CrashRecoveryContext::Disable();
+}
+
+TEST(CrashRecoveryTest, RefCountEnable) {
+ // CrashRecoveryContext's internal ref count should be zero
+ CrashRecoveryContext CRC;
+ EXPECT_DEATH(CRC.RunSafely(nullDeref), "");
+
+ llvm::CrashRecoveryContext::Enable();
+ EXPECT_FALSE(CRC.RunSafely(nullDeref));
+ llvm::CrashRecoveryContext::Disable();
+
+ EXPECT_DEATH(CRC.RunSafely(nullDeref), "");
+
+ llvm::CrashRecoveryContext::Enable();
+ llvm::CrashRecoveryContext::Enable();
+ // refcount = 2
+ EXPECT_FALSE(CRC.RunSafely(nullDeref));
+ llvm::CrashRecoveryContext::Disable();
+ // refcount = 1
+ EXPECT_FALSE(CRC.RunSafely(nullDeref));
+ llvm::CrashRecoveryContext::Disable();
+ // refcount = 0
+ EXPECT_DEATH(CRC.RunSafely(nullDeref), "");
}
#ifdef _WIN32
@@ -68,6 +123,7 @@
TEST(CrashRecoveryTest, RaiseException) {
llvm::CrashRecoveryContext::Enable();
EXPECT_FALSE(CrashRecoveryContext().RunSafely(raiseIt));
+ llvm::CrashRecoveryContext::Disable();
}
static void outputString() {
@@ -77,6 +133,7 @@
TEST(CrashRecoveryTest, CallOutputDebugString) {
llvm::CrashRecoveryContext::Enable();
EXPECT_TRUE(CrashRecoveryContext().RunSafely(outputString));
+ llvm::CrashRecoveryContext::Disable();
}
#endif
Index: llvm/lib/Support/Windows/Signals.inc
===================================================================
--- llvm/lib/Support/Windows/Signals.inc
+++ llvm/lib/Support/Windows/Signals.inc
@@ -521,10 +521,13 @@
extern "C" VOID WINAPI RtlCaptureContext(PCONTEXT ContextRecord);
#endif
-void llvm::sys::PrintStackTrace(raw_ostream &OS) {
- STACKFRAME64 StackFrame = {};
- CONTEXT Context = {};
- ::RtlCaptureContext(&Context);
+static void LocalPrintStackTrace(raw_ostream &OS, PCONTEXT C) {
+ STACKFRAME64 StackFrame{};
+ CONTEXT Context{};
+ if (!C) {
+ ::RtlCaptureContext(&Context);
+ C = &Context;
+ }
#if defined(_M_X64)
StackFrame.AddrPC.Offset = Context.Rip;
StackFrame.AddrStack.Offset = Context.Rsp;
@@ -546,9 +549,12 @@
StackFrame.AddrStack.Mode = AddrModeFlat;
StackFrame.AddrFrame.Mode = AddrModeFlat;
PrintStackTraceForThread(OS, GetCurrentProcess(), GetCurrentThread(),
- StackFrame, &Context);
+ StackFrame, C);
}
+void llvm::sys::PrintStackTrace(raw_ostream &OS) {
+ LocalPrintStackTrace(OS, nullptr);
+}
void llvm::sys::SetInterruptFunction(void (*IF)()) {
RegisterHandler();
@@ -792,6 +798,10 @@
return std::error_code();
}
+void sys::CleanupOnSignal(uintptr_t ExceptContext) {
+ LLVMUnhandledExceptionFilter((LPEXCEPTION_POINTERS)ExceptContext);
+}
+
static LONG WINAPI LLVMUnhandledExceptionFilter(LPEXCEPTION_POINTERS ep) {
Cleanup();
@@ -810,42 +820,9 @@
<< "\n";
}
- // Initialize the STACKFRAME structure.
- STACKFRAME64 StackFrame = {};
-
-#if defined(_M_X64)
- StackFrame.AddrPC.Offset = ep->ContextRecord->Rip;
- StackFrame.AddrPC.Mode = AddrModeFlat;
- StackFrame.AddrStack.Offset = ep->ContextRecord->Rsp;
- StackFrame.AddrStack.Mode = AddrModeFlat;
- StackFrame.AddrFrame.Offset = ep->ContextRecord->Rbp;
- StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_IX86)
- StackFrame.AddrPC.Offset = ep->ContextRecord->Eip;
- StackFrame.AddrPC.Mode = AddrModeFlat;
- StackFrame.AddrStack.Offset = ep->ContextRecord->Esp;
- StackFrame.AddrStack.Mode = AddrModeFlat;
- StackFrame.AddrFrame.Offset = ep->ContextRecord->Ebp;
- StackFrame.AddrFrame.Mode = AddrModeFlat;
-#elif defined(_M_ARM64) || defined(_M_ARM)
- StackFrame.AddrPC.Offset = ep->ContextRecord->Pc;
- StackFrame.AddrPC.Mode = AddrModeFlat;
- StackFrame.AddrStack.Offset = ep->ContextRecord->Sp;
- StackFrame.AddrStack.Mode = AddrModeFlat;
-#if defined(_M_ARM64)
- StackFrame.AddrFrame.Offset = ep->ContextRecord->Fp;
-#else
- StackFrame.AddrFrame.Offset = ep->ContextRecord->R11;
-#endif
- StackFrame.AddrFrame.Mode = AddrModeFlat;
-#endif
-
- HANDLE hProcess = GetCurrentProcess();
- HANDLE hThread = GetCurrentThread();
- PrintStackTraceForThread(llvm::errs(), hProcess, hThread, StackFrame,
- ep->ContextRecord);
+ LocalPrintStackTrace(llvm::errs(), ep ? ep->ContextRecord : nullptr);
- _exit(ep->ExceptionRecord->ExceptionCode);
+ return EXCEPTION_EXECUTE_HANDLER;
}
static BOOL WINAPI LLVMConsoleCtrlHandler(DWORD dwCtrlType) {
Index: llvm/lib/Support/Unix/Signals.inc
===================================================================
--- llvm/lib/Support/Unix/Signals.inc
+++ llvm/lib/Support/Unix/Signals.inc
@@ -209,7 +209,7 @@
/// 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, SIGTERM, SIGUSR2
+ SIGHUP, SIGINT, SIGTERM, SIGUSR2, SIGPIPE
};
/// Signals that represent that we have a bug, and our prompt termination has
@@ -240,7 +240,7 @@
static const size_t NumSigs =
array_lengthof(IntSigs) + array_lengthof(KillSigs) +
- array_lengthof(InfoSigs) + 1 /* SIGPIPE */;
+ array_lengthof(InfoSigs);
static std::atomic<unsigned> NumRegisteredSignals = ATOMIC_VAR_INIT(0);
@@ -325,8 +325,6 @@
registerHandler(S, SignalKind::IsKill);
for (auto S : KillSigs)
registerHandler(S, SignalKind::IsKill);
- if (OneShotPipeSignalFunction)
- registerHandler(SIGPIPE, SignalKind::IsKill);
for (auto S : InfoSigs)
registerHandler(S, SignalKind::IsInfo);
}
@@ -345,6 +343,20 @@
FileToRemoveList::removeAllFiles(FilesToRemove);
}
+void sys::CleanupOnSignal(uintptr_t ExceptContext) {
+ int Sig = (int)ExceptContext;
+
+ if (llvm::is_contained(InfoSigs, Sig)) {
+ InfoSignalHandler(Sig);
+ return;
+ }
+
+ RemoveFilesToRemove();
+
+ if (!llvm::is_contained(IntSigs, Sig))
+ llvm::sys::RunSignalHandlers();
+}
+
// The signal handler that runs.
static RETSIGTYPE SignalHandler(int Sig) {
// Restore the signal behavior to default, so that the program actually
@@ -358,27 +370,21 @@
sigfillset(&SigMask);
sigprocmask(SIG_UNBLOCK, &SigMask, nullptr);
- {
- RemoveFilesToRemove();
+ sys::CleanupOnSignal(Sig);
+
+ if (llvm::is_contained(IntSigs, Sig)) {
+ if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
+ return OldInterruptFunction();
if (Sig == SIGPIPE)
if (auto OldOneShotPipeFunction =
OneShotPipeSignalFunction.exchange(nullptr))
return OldOneShotPipeFunction();
- if (std::find(std::begin(IntSigs), std::end(IntSigs), Sig)
- != std::end(IntSigs)) {
- if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
- return OldInterruptFunction();
-
- raise(Sig); // Execute the default handler.
- return;
- }
+ 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
Index: llvm/lib/Support/CrashRecoveryContext.cpp
===================================================================
--- llvm/lib/Support/CrashRecoveryContext.cpp
+++ llvm/lib/Support/CrashRecoveryContext.cpp
@@ -10,9 +10,17 @@
#include "llvm/Config/llvm-config.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Signals.h"
#include "llvm/Support/ThreadLocal.h"
#include <mutex>
#include <setjmp.h>
+#ifdef _WIN32
+#include <excpt.h> // for GetExceptionInformation
+#endif
+#if LLVM_ON_UNIX
+#include <sysexits.h> // EX_IOERR
+#endif
+
using namespace llvm;
namespace {
@@ -54,7 +62,7 @@
#endif
}
- void HandleCrash() {
+ void HandleCrash(int RetCode, uintptr_t ExceptContext) {
// Eliminate the current context entry, to avoid re-entering in case the
// cleanup code crashes.
CurrentContext->set(Next);
@@ -62,7 +70,10 @@
assert(!Failed && "Crash recovery context already failed!");
Failed = true;
- // FIXME: Stash the backtrace.
+ if (CRC->DumpStackAndCleanupOnFailure)
+ sys::CleanupOnSignal(ExceptContext);
+
+ CRC->RetCode = RetCode;
// Jump back to the RunSafely we were called under.
longjmp(JumpBuffer, 1);
@@ -71,8 +82,9 @@
}
+static std::atomic<int> gCrashRecoveryEnabledCount;
static ManagedStatic<std::mutex> gCrashRecoveryContextMutex;
-static bool gCrashRecoveryEnabled = false;
+static bool gCrashRecoveryInstalled;
static ManagedStatic<sys::ThreadLocal<const CrashRecoveryContext>>
tlIsRecoveringFromCrash;
@@ -80,8 +92,30 @@
static void installExceptionOrSignalHandlers();
static void uninstallExceptionOrSignalHandlers();
+static void Install() {
+ if (gCrashRecoveryEnabledCount <= 0)
+ return;
+ std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
+ if (gCrashRecoveryInstalled)
+ return;
+ installExceptionOrSignalHandlers();
+ gCrashRecoveryInstalled = true;
+}
+
+static void Uninstall(bool Force) {
+ if (!Force && gCrashRecoveryEnabledCount > 0)
+ return;
+ std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
+ if (!gCrashRecoveryInstalled)
+ return;
+ uninstallExceptionOrSignalHandlers();
+ gCrashRecoveryInstalled = false;
+}
+
CrashRecoveryContextCleanup::~CrashRecoveryContextCleanup() {}
+CrashRecoveryContext::CrashRecoveryContext() { Install(); }
+
CrashRecoveryContext::~CrashRecoveryContext() {
// Reclaim registered resources.
CrashRecoveryContextCleanup *i = head;
@@ -105,7 +139,7 @@
}
CrashRecoveryContext *CrashRecoveryContext::GetCurrent() {
- if (!gCrashRecoveryEnabled)
+ if (gCrashRecoveryEnabledCount <= 0)
return nullptr;
const CrashRecoveryContextImpl *CRCI = CurrentContext->get();
@@ -115,21 +149,11 @@
return CRCI->CRC;
}
-void CrashRecoveryContext::Enable() {
- std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
- // FIXME: Shouldn't this be a refcount or something?
- if (gCrashRecoveryEnabled)
- return;
- gCrashRecoveryEnabled = true;
- installExceptionOrSignalHandlers();
-}
+void CrashRecoveryContext::Enable() { ++gCrashRecoveryEnabledCount; }
void CrashRecoveryContext::Disable() {
- std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
- if (!gCrashRecoveryEnabled)
- return;
- gCrashRecoveryEnabled = false;
- uninstallExceptionOrSignalHandlers();
+ --gCrashRecoveryEnabledCount;
+ Uninstall(/*Force=*/ false);
}
void CrashRecoveryContext::registerCleanup(CrashRecoveryContextCleanup *cleanup)
@@ -171,19 +195,32 @@
static void installExceptionOrSignalHandlers() {}
static void uninstallExceptionOrSignalHandlers() {}
-bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
- if (!gCrashRecoveryEnabled) {
+// We need this function because the call to GetExceptionInformation() can only
+// occur inside the __except evaluation block
+static inline int ExceptionFilter(bool DumpStackAndCleanup,
+ _EXCEPTION_POINTERS *Except) {
+ if (DumpStackAndCleanup)
+ sys::CleanupOnSignal((uintptr_t)Except);
+ return EXCEPTION_EXECUTE_HANDLER;
+}
+
+static bool InvokeFunctionCall(function_ref<void()> Fn,
+ bool DumpStackAndCleanup, int &RetCode) {
+ __try {
Fn();
- return true;
+ } __except (ExceptionFilter(DumpStackAndCleanup, GetExceptionInformation())) {
+ RetCode = GetExceptionCode();
+ return false;
}
+ return true;
+}
- bool Result = true;
- __try {
+bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
+ if (gCrashRecoveryEnabledCount <= 0) {
Fn();
- } __except (1) { // Catch any exception.
- Result = false;
+ return true;
}
- return Result;
+ return InvokeFunctionCall(Fn, DumpStackAndCleanupOnFailure, RetCode);
}
#else // !_MSC_VER
@@ -229,7 +266,7 @@
if (!CRCI) {
// Something has gone horribly wrong, so let's just tell everyone
// to keep searching
- CrashRecoveryContext::Disable();
+ Uninstall(/*Force=*/true);
return EXCEPTION_CONTINUE_SEARCH;
}
@@ -237,7 +274,8 @@
// implementation if we so choose.
// Handle the crash
- const_cast<CrashRecoveryContextImpl*>(CRCI)->HandleCrash();
+ const_cast<CrashRecoveryContextImpl *>(CRCI)->HandleCrash(
+ (int)ExceptionInfo->ExceptionRecord->ExceptionCode, ExceptionInfo);
// Note that we don't actually get here because HandleCrash calls
// longjmp, which means the HandleCrash function never returns.
@@ -304,9 +342,7 @@
// Disable crash recovery and raise the signal again. The assumption here is
// that the enclosing application will terminate soon, and we won't want to
// attempt crash recovery again.
- //
- // This call of Disable isn't thread safe, but it doesn't actually matter.
- CrashRecoveryContext::Disable();
+ Uninstall(/*Force=*/ true);
raise(Signal);
// The signal will be thrown once the signal mask is restored.
@@ -319,8 +355,16 @@
sigaddset(&SigMask, Signal);
sigprocmask(SIG_UNBLOCK, &SigMask, nullptr);
+ // As per convention, -2 indicates a crash or timeout as opposed to failure to
+ // execute (see llvm/include/llvm/Support/Program.h)
+ int RetCode = -2;
+
+ // Don't consider a broken pipe as a crash (see clang/lib/Driver/Driver.cpp)
+ if (Signal == SIGPIPE)
+ RetCode = EX_IOERR;
+
if (CRCI)
- const_cast<CrashRecoveryContextImpl*>(CRCI)->HandleCrash();
+ const_cast<CrashRecoveryContextImpl *>(CRCI)->HandleCrash(RetCode, Signal);
}
static void installExceptionOrSignalHandlers() {
@@ -345,7 +389,7 @@
bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
// If crash recovery is disabled, do nothing.
- if (gCrashRecoveryEnabled) {
+ if (gCrashRecoveryEnabledCount > 0) {
assert(!Impl && "Crash recovery context already initialized!");
CrashRecoveryContextImpl *CRCI = new CrashRecoveryContextImpl(this);
Impl = CRCI;
@@ -361,12 +405,6 @@
#endif // !_MSC_VER
-void CrashRecoveryContext::HandleCrash() {
- CrashRecoveryContextImpl *CRCI = (CrashRecoveryContextImpl *) Impl;
- assert(CRCI && "Crash recovery context never initialized!");
- CRCI->HandleCrash();
-}
-
// FIXME: Portability.
static void setThreadBackgroundPriority() {
#ifdef __APPLE__
Index: llvm/include/llvm/Support/Signals.h
===================================================================
--- llvm/include/llvm/Support/Signals.h
+++ llvm/include/llvm/Support/Signals.h
@@ -106,6 +106,13 @@
/// On Unix systems, this function exits with an "IO error" exit code.
/// This is a no-op on Windows.
void DefaultOneShotPipeSignalHandler();
+
+ /// This function does the following:
+ /// - clean up any temporary files registered with RemoveFileOnSignal()
+ /// - dump the callstack from the exception context
+ /// - call any relevant interrupt/signal handlers
+ /// - create a core/mini dump of the exception context whenever possible
+ void CleanupOnSignal(uintptr_t ExceptContext);
} // End sys namespace
} // End llvm namespace
Index: llvm/include/llvm/Support/CrashRecoveryContext.h
===================================================================
--- llvm/include/llvm/Support/CrashRecoveryContext.h
+++ llvm/include/llvm/Support/CrashRecoveryContext.h
@@ -44,11 +44,11 @@
/// executed in any case, whether crash occurs or not. These actions may be used
/// to reclaim resources in the case of crash.
class CrashRecoveryContext {
- void *Impl;
- CrashRecoveryContextCleanup *head;
+ void *Impl = nullptr;
+ CrashRecoveryContextCleanup *head = nullptr;
public:
- CrashRecoveryContext() : Impl(nullptr), head(nullptr) {}
+ CrashRecoveryContext();
~CrashRecoveryContext();
/// Register cleanup handler, which is used when the recovery context is
@@ -97,9 +97,13 @@
return RunSafelyOnThread([&]() { Fn(UserData); }, RequestedStackSize);
}
- /// Explicitly trigger a crash recovery in the current process, and
- /// return failure from RunSafely(). This function does not return.
- void HandleCrash();
+ /// In case of a crash, this is the crash identifier.
+ int RetCode = 0;
+
+ /// Selects whether handling of exceptions should be done in the same way as
+ /// for global exceptions. When this is active, a crash would print the
+ /// callstack, clean-up any temporary files and create a coredump/minidump.
+ bool DumpStackAndCleanupOnFailure = false;
};
/// Abstract base class of cleanup handlers.
Index: clang/lib/Lex/Pragma.cpp
===================================================================
--- clang/lib/Lex/Pragma.cpp
+++ clang/lib/Lex/Pragma.cpp
@@ -1105,10 +1105,6 @@
M->dump();
} else if (II->isStr("overflow_stack")) {
DebugOverflowStack();
- } else if (II->isStr("handle_crash")) {
- llvm::CrashRecoveryContext *CRC =llvm::CrashRecoveryContext::GetCurrent();
- if (CRC)
- CRC->HandleCrash();
} else if (II->isStr("captured")) {
HandleCaptured(PP);
} else {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
