From 0f42e5be40e783f2754c11f6d7aeefb4a3640e5f Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Wed, 17 May 2017 17:15:00 +0000 Subject: [PATCH] Revert "[CrashRecovery] Use SEH __try instead of VEH when available" This reverts commit r303274, it appears to break some clang tests. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@303275 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Support/CrashRecoveryContext.cpp | 114 ++++++++++-------------- unittests/Support/CMakeLists.txt | 1 - unittests/Support/CrashRecoveryTest.cpp | 83 ----------------- 3 files changed, 48 insertions(+), 150 deletions(-) delete mode 100644 unittests/Support/CrashRecoveryTest.cpp diff --git a/lib/Support/CrashRecoveryContext.cpp b/lib/Support/CrashRecoveryContext.cpp index 1b0d452829b..6ebde46203f 100644 --- a/lib/Support/CrashRecoveryContext.cpp +++ b/lib/Support/CrashRecoveryContext.cpp @@ -78,9 +78,6 @@ static bool gCrashRecoveryEnabled = false; static ManagedStatic> tlIsRecoveringFromCrash; -static void installExceptionOrSignalHandlers(); -static void uninstallExceptionOrSignalHandlers(); - CrashRecoveryContextCleanup::~CrashRecoveryContextCleanup() {} CrashRecoveryContext::~CrashRecoveryContext() { @@ -116,23 +113,6 @@ CrashRecoveryContext *CrashRecoveryContext::GetCurrent() { 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) @@ -160,51 +140,27 @@ CrashRecoveryContext::unregisterCleanup(CrashRecoveryContextCleanup *cleanup) { delete cleanup; } -#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. +#ifdef LLVM_ON_WIN32 -static void installExceptionOrSignalHandlers() {} -static void uninstallExceptionOrSignalHandlers() {} - -bool CrashRecoveryContext::RunSafely(function_ref Fn) { - bool Result = true; - __try { - Fn(); - } __except (1) { // Catch any exception. - Result = false; - } - return Result; -} - -#else // !_MSC_VER +#include "Windows/WindowsSupport.h" -#if defined(LLVM_ON_WIN32) -// This is a non-MSVC compiler, probably mingw gcc or clang without -// -fms-extensions. Use vectored exception handling (VEH). +// 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. // -// 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. +// Vectored exception handling is built on top of SEH, and so it +// works on a per-thread basis. // // 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. - -#include "Windows/WindowsSupport.h" +// +// 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). static LONG CALLBACK ExceptionHandler(PEXCEPTION_POINTERS ExceptionInfo) { @@ -247,7 +203,14 @@ static LONG CALLBACK ExceptionHandler(PEXCEPTION_POINTERS ExceptionInfo) // non-NULL, valid VEH handles, or NULL. static sys::ThreadLocal sCurrentExceptionHandle; -static void installExceptionOrSignalHandlers() { +void CrashRecoveryContext::Enable() { + sys::ScopedLock L(*gCrashRecoveryContextMutex); + + if (gCrashRecoveryEnabled) + return; + + gCrashRecoveryEnabled = true; + // 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 @@ -256,7 +219,14 @@ static void installExceptionOrSignalHandlers() { sCurrentExceptionHandle.set(handle); } -static void uninstallExceptionOrSignalHandlers() { +void CrashRecoveryContext::Disable() { + sys::ScopedLock L(*gCrashRecoveryContextMutex); + + if (!gCrashRecoveryEnabled) + return; + + gCrashRecoveryEnabled = false; + PVOID currentHandle = const_cast(sCurrentExceptionHandle.get()); if (currentHandle) { // Now we can remove the vectored exception handler from the chain @@ -267,7 +237,7 @@ static void uninstallExceptionOrSignalHandlers() { } } -#else // !LLVM_ON_WIN32 +#else // Generic POSIX implementation. // @@ -319,7 +289,14 @@ static void CrashRecoverySignalHandler(int Signal) { const_cast(CRCI)->HandleCrash(); } -static void installExceptionOrSignalHandlers() { +void CrashRecoveryContext::Enable() { + sys::ScopedLock L(*gCrashRecoveryContextMutex); + + if (gCrashRecoveryEnabled) + return; + + gCrashRecoveryEnabled = true; + // Setup the signal handler. struct sigaction Handler; Handler.sa_handler = CrashRecoverySignalHandler; @@ -331,13 +308,20 @@ static void installExceptionOrSignalHandlers() { } } -static void uninstallExceptionOrSignalHandlers() { +void CrashRecoveryContext::Disable() { + sys::ScopedLock L(*gCrashRecoveryContextMutex); + + if (!gCrashRecoveryEnabled) + return; + + gCrashRecoveryEnabled = false; + // Restore the previous signal handlers. for (unsigned i = 0; i != NumSignals; ++i) sigaction(Signals[i], &PrevActions[i], nullptr); } -#endif // !LLVM_ON_WIN32 +#endif bool CrashRecoveryContext::RunSafely(function_ref Fn) { // If crash recovery is disabled, do nothing. @@ -355,8 +339,6 @@ bool CrashRecoveryContext::RunSafely(function_ref Fn) { return true; } -#endif // !_MSC_VER - void CrashRecoveryContext::HandleCrash() { CrashRecoveryContextImpl *CRCI = (CrashRecoveryContextImpl *) Impl; assert(CRCI && "Crash recovery context never initialized!"); diff --git a/unittests/Support/CMakeLists.txt b/unittests/Support/CMakeLists.txt index e7f2f515d76..f8d3c1c9a8c 100644 --- a/unittests/Support/CMakeLists.txt +++ b/unittests/Support/CMakeLists.txt @@ -11,7 +11,6 @@ add_llvm_unittest(SupportTests BlockFrequencyTest.cpp BranchProbabilityTest.cpp CachePruningTest.cpp - CrashRecoveryTest.cpp Casting.cpp Chrono.cpp CommandLineTest.cpp diff --git a/unittests/Support/CrashRecoveryTest.cpp b/unittests/Support/CrashRecoveryTest.cpp deleted file mode 100644 index dbb0db57679..00000000000 --- a/unittests/Support/CrashRecoveryTest.cpp +++ /dev/null @@ -1,83 +0,0 @@ -//===- 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 -#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 -- 2.50.1