[CrashRecovery] Use SEH __try instead of VEH when available

[rnk]

Summary

It avoids problems when other libraries raise exceptions. In particular,
OutputDebugString raises an exception that the debugger is supposed to
catch and suppress. VEH kicks in first right now, and that is entirely
incorrect.

Unfortunately, GCC does not support SEH, so I've kept the old buggy VEH
codepath around. We could fix it with SetUnhandledExceptionFilter, but
that is not per-thread, so a well-behaved library shouldn't set it.

[zturner]

llvm/lib/Support/CrashRecoveryContext.cpp

165–166

I legitimately chuckled at this comment. But for those who aren't as familiar, can you maybe expand this comment to say why it's better?

182

I'm probably just drawing a blank here, but "or clang"? You implemented SEH for clang. Plus, clang has _MSC_VER defined. Do you mean "clang without -fms-compatibility"?

272–273

This function is copied down below. Can we limit it to to only one copy of the function in the source file, using some preprocessor defs?

[rnk]

• address comments

[zturner]

looks good with suggested fixes.

llvm/lib/Support/CrashRecoveryContext.cpp

257

Since we've changed this value from 1 to 0, the comment above should be fixed, perhaps with an explanation of why going last is preferable.

llvm/unittests/Support/CrashRecoveryTest.cpp

79

Just call the test something else? CallOutputDebugString?

[rnk]

Thanks! Looks like I forgot to send my replies when I re-uploaded my last patch, so they're dated.

llvm/lib/Support/CrashRecoveryContext.cpp

182

I meant clang configured for mingw, so all that SEH jazz is probably disabled.

257

Let's just set it back to 1. I wasn't trying to change behavior here.

272–273

Sure. I copied it to avoid nested #if's, which get pretty annoying fast.

llvm/lib/Support/CrashRecoveryContext.cpp

 }
 
 static ManagedStatic<sys::Mutex> gCrashRecoveryContextMutex;
 static bool gCrashRecoveryEnabled = false;
 
 static ManagedStatic<sys::ThreadLocal<const CrashRecoveryContext>>
        tlIsRecoveringFromCrash;
 
+static void installExceptionOrSignalHandlers();
+static void uninstallExceptionOrSignalHandlers();
+
 CrashRecoveryContextCleanup::~CrashRecoveryContextCleanup() {}
 
 CrashRecoveryContext::~CrashRecoveryContext() {
   // Reclaim registered resources.
   CrashRecoveryContextCleanup *i = head;
   const CrashRecoveryContext *PC = tlIsRecoveringFromCrash->get();
   tlIsRecoveringFromCrash->set(this);
   while (i) {
CrashRecoveryContext *CrashRecoveryContext::GetCurrent() {
 
   const CrashRecoveryContextImpl *CRCI = CurrentContext->get();
   if (!CRCI)
     return nullptr;
 
   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)
     return;
   if (head)
     head->prev = cleanup;
   cleanup->next = head;
   head = cleanup;
CrashRecoveryContext::unregisterCleanup(CrashRecoveryContextCleanup *cleanup) {
   else {
     cleanup->prev->next = cleanup->next;
     if (cleanup->next)
       cleanup->next->prev = cleanup->prev;
   }
   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() {}
 
-// 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.
+bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
+  bool Result = true;
+  __try {
+    Fn();
+  } __except (1) { // Catch any exception.
+    Result = false;
+  }
+  return Result;
+}
+
+#else // !_MSC_VER
+
+#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)
 {
 #ifdef DBG_PRINTEXCEPTION_WIDE_C
   constexpr ULONG WideDbgPrintValue = DBG_PRINTEXCEPTION_WIDE_C;
 #else
   constexpr ULONG WideDbgPrintValue = 0x4001000AL;
 #endif

 
 // Because the Enable and Disable calls are static, it means that
 // there may not actually be an Impl available, or even a current
 // CrashRecoveryContext at all.  So we make use of a thread-local
 // exception table.  The handles contained in here will either be
 // 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
   // our handler).  This 1) isn't likely, and 2) shouldn't cause problems.
-  PVOID handle = ::AddVectoredExceptionHandler(1, ExceptionHandler);
+  PVOID handle = ::AddVectoredExceptionHandler(0, ExceptionHandler);
   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
     ::RemoveVectoredExceptionHandler(currentHandle);
 
     // Reset the handle in our thread-local set.
     sCurrentExceptionHandle.set(NULL);
   }
 }
 
-#else
+#else // !LLVM_ON_WIN32
 
 // Generic POSIX implementation.
 //
 // This implementation relies on synchronous signals being delivered to the
 // current thread. We use a thread local object to keep track of the active
 // crash recovery context, and install signal handlers to invoke HandleCrash on
 // the active object.
 //
 // This implementation does not to attempt to chain signal handlers in any
static void CrashRecoverySignalHandler(int Signal) {
   sigemptyset(&SigMask);
   sigaddset(&SigMask, Signal);
   sigprocmask(SIG_UNBLOCK, &SigMask, nullptr);
 
   if (CRCI)
     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;
   Handler.sa_flags = 0;
   sigemptyset(&Handler.sa_mask);
 
   for (unsigned i = 0; i != NumSignals; ++i) {
     sigaction(Signals[i], &Handler, &PrevActions[i]);
   }
 }
 
-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.
   if (gCrashRecoveryEnabled) {
     assert(!Impl && "Crash recovery context already initialized!");
     CrashRecoveryContextImpl *CRCI = new CrashRecoveryContextImpl(this);
     Impl = CRCI;
 
     if (setjmp(CRCI->JumpBuffer) != 0) {
       return false;
     }
   }
 
   Fn();
   return true;
 }
 
+#endif // !_MSC_VER
+
 void CrashRecoveryContext::HandleCrash() {
   CrashRecoveryContextImpl *CRCI = (CrashRecoveryContextImpl *) Impl;
   assert(CRCI && "Crash recovery context never initialized!");
   CRCI->HandleCrash();
 }
 
 // FIXME: Portability.
 static void setThreadBackgroundPriority() {

llvm/unittests/Support/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_llvm_unittest(SupportTests
   AlignOfTest.cpp
   AllocatorTest.cpp
   ARMAttributeParser.cpp
   ArrayRecyclerTest.cpp
   BinaryStreamTest.cpp
   BlockFrequencyTest.cpp
   BranchProbabilityTest.cpp
   CachePruningTest.cpp
+  CrashRecoveryTest.cpp
   Casting.cpp
   Chrono.cpp
   CommandLineTest.cpp
   CompressionTest.cpp
   ConvertUTFTest.cpp
   DataExtractorTest.cpp
   DebugTest.cpp
   DwarfTest.cpp

llvm/unittests/Support/CrashRecoveryTest.cpp

+//===- 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");
+}
+
+// Don't let Windows Unicode macros change the name of our test.
+#undef OutputDebugString
+
+TEST(CrashRecoveryTest, OutputDebugString) {
+  llvm::CrashRecoveryContext::Enable();
+  EXPECT_TRUE(CrashRecoveryContext().RunSafely(outputString));
+}
+
+#endif