[llvm] r303279 - Re-land r303274: "[CrashRecovery] Use SEH __try instead of VEH when available"
Reid Kleckner via llvm-commits
llvm-commits at lists.llvm.org
Wed May 17 11:16:17 PDT 2017
Author: rnk
Date: Wed May 17 13:16:17 2017
New Revision: 303279
URL: http://llvm.org/viewvc/llvm-project?rev=303279&view=rev
Log:
Re-land r303274: "[CrashRecovery] Use SEH __try instead of VEH when available"
We have to check gCrashRecoveryEnabled before using __try.
In other words, SEH works too well and we ended up recovering from
crashes in implicit module builds that we weren't supposed to. Only
libclang is supposed to enable CrashRecoveryContext to allow implicit
module builds to crash.
Added:
llvm/trunk/unittests/Support/CrashRecoveryTest.cpp
Modified:
llvm/trunk/lib/Support/CrashRecoveryContext.cpp
llvm/trunk/unittests/Support/CMakeLists.txt
Modified: llvm/trunk/lib/Support/CrashRecoveryContext.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CrashRecoveryContext.cpp?rev=303279&r1=303278&r2=303279&view=diff
==============================================================================
--- llvm/trunk/lib/Support/CrashRecoveryContext.cpp (original)
+++ llvm/trunk/lib/Support/CrashRecoveryContext.cpp Wed May 17 13:16:17 2017
@@ -78,6 +78,9 @@ static bool gCrashRecoveryEnabled = fals
static ManagedStatic<sys::ThreadLocal<const CrashRecoveryContext>>
tlIsRecoveringFromCrash;
+static void installExceptionOrSignalHandlers();
+static void uninstallExceptionOrSignalHandlers();
+
CrashRecoveryContextCleanup::~CrashRecoveryContextCleanup() {}
CrashRecoveryContext::~CrashRecoveryContext() {
@@ -113,6 +116,23 @@ CrashRecoveryContext *CrashRecoveryConte
return CRCI->CRC;
}
+void CrashRecoveryContext::Enable() {
+ sys::ScopedLock L(*gCrashRecoveryContextMutex);
+ // FIXME: Shouldn't this be a refcount or something?
+ if (gCrashRecoveryEnabled)
+ return;
+ gCrashRecoveryEnabled = true;
+ installExceptionOrSignalHandlers();
+}
+
+void CrashRecoveryContext::Disable() {
+ sys::ScopedLock L(*gCrashRecoveryContextMutex);
+ if (!gCrashRecoveryEnabled)
+ return;
+ gCrashRecoveryEnabled = false;
+ uninstallExceptionOrSignalHandlers();
+}
+
void CrashRecoveryContext::registerCleanup(CrashRecoveryContextCleanup *cleanup)
{
if (!cleanup)
@@ -140,27 +160,56 @@ CrashRecoveryContext::unregisterCleanup(
delete cleanup;
}
-#ifdef LLVM_ON_WIN32
+#if defined(_MSC_VER)
+// If _MSC_VER is defined, we must have SEH. Use it if it's available. It's way
+// better than VEH. Vectored exception handling catches all exceptions happening
+// on the thread with installed exception handlers, so it can interfere with
+// internal exception handling of other libraries on that thread. SEH works
+// exactly as you would expect normal exception handling to work: it only
+// catches exceptions if they would bubble out from the stack frame with __try /
+// __except.
-#include "Windows/WindowsSupport.h"
+static void installExceptionOrSignalHandlers() {}
+static void uninstallExceptionOrSignalHandlers() {}
+
+bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
+ if (!gCrashRecoveryEnabled) {
+ Fn();
+ return true;
+ }
+
+ bool Result = true;
+ __try {
+ Fn();
+ } __except (1) { // Catch any exception.
+ Result = false;
+ }
+ return Result;
+}
+
+#else // !_MSC_VER
-// On Windows, we can make use of vectored exception handling to
-// catch most crashing situations. Note that this does mean
-// we will be alerted of exceptions *before* structured exception
-// handling has the opportunity to catch it. But that isn't likely
-// to cause problems because nowhere in the project is SEH being
-// used.
+#if defined(LLVM_ON_WIN32)
+// This is a non-MSVC compiler, probably mingw gcc or clang without
+// -fms-extensions. Use vectored exception handling (VEH).
//
-// Vectored exception handling is built on top of SEH, and so it
-// works on a per-thread basis.
+// On Windows, we can make use of vectored exception handling to catch most
+// crashing situations. Note that this does mean we will be alerted of
+// exceptions *before* structured exception handling has the opportunity to
+// catch it. Unfortunately, this causes problems in practice with other code
+// running on threads with LLVM crash recovery contexts, so we would like to
+// eventually move away from VEH.
+//
+// Vectored works on a per-thread basis, which is an advantage over
+// SetUnhandledExceptionFilter. SetUnhandledExceptionFilter also doesn't have
+// any native support for chaining exception handlers, but VEH allows more than
+// one.
//
// The vectored exception handler functionality was added in Windows
// XP, so if support for older versions of Windows is required,
// it will have to be added.
-//
-// If we want to support as far back as Win2k, we could use the
-// SetUnhandledExceptionFilter API, but there's a risk of that
-// being entirely overwritten (it's not a chain).
+
+#include "Windows/WindowsSupport.h"
static LONG CALLBACK ExceptionHandler(PEXCEPTION_POINTERS ExceptionInfo)
{
@@ -203,14 +252,7 @@ static LONG CALLBACK ExceptionHandler(PE
// non-NULL, valid VEH handles, or NULL.
static sys::ThreadLocal<const void> sCurrentExceptionHandle;
-void CrashRecoveryContext::Enable() {
- sys::ScopedLock L(*gCrashRecoveryContextMutex);
-
- if (gCrashRecoveryEnabled)
- return;
-
- gCrashRecoveryEnabled = true;
-
+static void installExceptionOrSignalHandlers() {
// We can set up vectored exception handling now. We will install our
// handler as the front of the list, though there's no assurances that
// it will remain at the front (another call could install itself before
@@ -219,14 +261,7 @@ void CrashRecoveryContext::Enable() {
sCurrentExceptionHandle.set(handle);
}
-void CrashRecoveryContext::Disable() {
- sys::ScopedLock L(*gCrashRecoveryContextMutex);
-
- if (!gCrashRecoveryEnabled)
- return;
-
- gCrashRecoveryEnabled = false;
-
+static void uninstallExceptionOrSignalHandlers() {
PVOID currentHandle = const_cast<PVOID>(sCurrentExceptionHandle.get());
if (currentHandle) {
// Now we can remove the vectored exception handler from the chain
@@ -237,7 +272,7 @@ void CrashRecoveryContext::Disable() {
}
}
-#else
+#else // !LLVM_ON_WIN32
// Generic POSIX implementation.
//
@@ -289,14 +324,7 @@ static void CrashRecoverySignalHandler(i
const_cast<CrashRecoveryContextImpl*>(CRCI)->HandleCrash();
}
-void CrashRecoveryContext::Enable() {
- sys::ScopedLock L(*gCrashRecoveryContextMutex);
-
- if (gCrashRecoveryEnabled)
- return;
-
- gCrashRecoveryEnabled = true;
-
+static void installExceptionOrSignalHandlers() {
// Setup the signal handler.
struct sigaction Handler;
Handler.sa_handler = CrashRecoverySignalHandler;
@@ -308,20 +336,13 @@ void CrashRecoveryContext::Enable() {
}
}
-void CrashRecoveryContext::Disable() {
- sys::ScopedLock L(*gCrashRecoveryContextMutex);
-
- if (!gCrashRecoveryEnabled)
- return;
-
- gCrashRecoveryEnabled = false;
-
+static void uninstallExceptionOrSignalHandlers() {
// Restore the previous signal handlers.
for (unsigned i = 0; i != NumSignals; ++i)
sigaction(Signals[i], &PrevActions[i], nullptr);
}
-#endif
+#endif // !LLVM_ON_WIN32
bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
// If crash recovery is disabled, do nothing.
@@ -339,6 +360,8 @@ bool CrashRecoveryContext::RunSafely(fun
return true;
}
+#endif // !_MSC_VER
+
void CrashRecoveryContext::HandleCrash() {
CrashRecoveryContextImpl *CRCI = (CrashRecoveryContextImpl *) Impl;
assert(CRCI && "Crash recovery context never initialized!");
Modified: llvm/trunk/unittests/Support/CMakeLists.txt
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CMakeLists.txt?rev=303279&r1=303278&r2=303279&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/CMakeLists.txt (original)
+++ llvm/trunk/unittests/Support/CMakeLists.txt Wed May 17 13:16:17 2017
@@ -11,6 +11,7 @@ add_llvm_unittest(SupportTests
BlockFrequencyTest.cpp
BranchProbabilityTest.cpp
CachePruningTest.cpp
+ CrashRecoveryTest.cpp
Casting.cpp
Chrono.cpp
CommandLineTest.cpp
Added: llvm/trunk/unittests/Support/CrashRecoveryTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CrashRecoveryTest.cpp?rev=303279&view=auto
==============================================================================
--- llvm/trunk/unittests/Support/CrashRecoveryTest.cpp (added)
+++ llvm/trunk/unittests/Support/CrashRecoveryTest.cpp Wed May 17 13:16:17 2017
@@ -0,0 +1,83 @@
+//===- llvm/unittest/Support/CrashRecoveryTest.cpp ------------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/CrashRecoveryContext.h"
+#include "llvm/Support/Compiler.h"
+#include "gtest/gtest.h"
+
+#ifdef LLVM_ON_WIN32
+#define WIN32_LEAN_AND_MEAN
+#define NOGDI
+#include <windows.h>
+#endif
+
+using namespace llvm;
+using namespace llvm::sys;
+
+static int GlobalInt = 0;
+static void nullDeref() { *(volatile int *)nullptr = 0; }
+static void incrementGlobal() { ++GlobalInt; }
+static void llvmTrap() { LLVM_BUILTIN_TRAP; }
+
+TEST(CrashRecoveryTest, Basic) {
+ llvm::CrashRecoveryContext::Enable();
+ GlobalInt = 0;
+ EXPECT_TRUE(CrashRecoveryContext().RunSafely(incrementGlobal));
+ EXPECT_EQ(1, GlobalInt);
+ EXPECT_FALSE(CrashRecoveryContext().RunSafely(nullDeref));
+ EXPECT_FALSE(CrashRecoveryContext().RunSafely(llvmTrap));
+}
+
+struct IncrementGlobalCleanup : CrashRecoveryContextCleanup {
+ IncrementGlobalCleanup(CrashRecoveryContext *CRC)
+ : CrashRecoveryContextCleanup(CRC) {}
+ virtual void recoverResources() { ++GlobalInt; }
+};
+
+static void noop() {}
+
+TEST(CrashRecoveryTest, Cleanup) {
+ llvm::CrashRecoveryContext::Enable();
+ GlobalInt = 0;
+ {
+ CrashRecoveryContext CRC;
+ CRC.registerCleanup(new IncrementGlobalCleanup(&CRC));
+ EXPECT_TRUE(CRC.RunSafely(noop));
+ } // run cleanups
+ EXPECT_EQ(1, GlobalInt);
+
+ GlobalInt = 0;
+ {
+ CrashRecoveryContext CRC;
+ CRC.registerCleanup(new IncrementGlobalCleanup(&CRC));
+ EXPECT_FALSE(CRC.RunSafely(nullDeref));
+ } // run cleanups
+ EXPECT_EQ(1, GlobalInt);
+}
+
+#ifdef LLVM_ON_WIN32
+static void raiseIt() {
+ RaiseException(123, EXCEPTION_NONCONTINUABLE, 0, NULL);
+}
+
+TEST(CrashRecoveryTest, RaiseException) {
+ llvm::CrashRecoveryContext::Enable();
+ EXPECT_FALSE(CrashRecoveryContext().RunSafely(raiseIt));
+}
+
+static void outputString() {
+ OutputDebugStringA("output for debugger\n");
+}
+
+TEST(CrashRecoveryTest, CallOutputDebugString) {
+ llvm::CrashRecoveryContext::Enable();
+ EXPECT_TRUE(CrashRecoveryContext().RunSafely(outputString));
+}
+
+#endif
More information about the llvm-commits
mailing list