[Clang] When using SEH, create a impl instance for CrashRecoveryContext. NFCI.

[aganea]

Summary

Previously, the SEH codepath in CrashRecoveryContext didn't create a CrashRecoveryContextImpl. The other codepaths (VEH and Unix) were creating it.

When running with -fintegrated-cc1, this is needed to handle exit() as a jump to CrashRecoveryContext's exception filter, through a call to RaiseException. In that situation, we need a user-defined exception code, which is later interpreted as an exit() by the exception filter. Which in turn needs to set RetCode accordingly, inside the exception filter, and before calling HandleCrash().

This is a support patch for D73742.

[rnk]

Seems fine in principle. lgtm with the one issue fixed.

llvm/lib/Support/CrashRecoveryContext.cpp

196

I think optnone has to remain attached to the function using __try, not the filter.

219

So, here.

[hans]

lgtm with rnk's comment addressed

[aganea]

llvm/lib/Support/CrashRecoveryContext.cpp

196

Silly me, of course it's on the function using the __try. Thanks for catching this!

[hans]

Pushed to 10.x in 4759c6ef8b4ab8d17be3a5e6c96e890944bc515d

[tra]

Tensorflow folks report that the __try() here does not compile on windows:

ERROR: T:/tmp/nsz6drem/external/llvm-project/llvm/BUILD:3716:1: C++ compilation of rule '@llvm-project//llvm:support' failed (Exit 2)
external/llvm-project/llvm/lib/Support/CrashRecoveryContext.cpp(218): error C2712: Cannot use __try in functions that require object unwinding

Any ideas what could be wrong?

[aganea]

In D74078#1870669, @tra wrote:

Tensorflow folks report that the __try() here does not compile on windows:
Any ideas what could be wrong?

Usage of /EHsc most likely. Does it work on the previous revision of the code? If that's the case, could ask them to move the __try/__catch block into a separate function like it used to be, see if that works for them?

[rnk]

I repro'd this locally with /EHsc and came up with a fix: rGa349c09162a8260bdf691c4f7ab72a16c33975f6

[hans]

In D74078#1871015, @rnk wrote:

I repro'd this locally with /EHsc and came up with a fix: rGa349c09162a8260bdf691c4f7ab72a16c33975f6

Thanks! Pushed that to 10.x in d8a6deab7a84a559a1ff9f2196dae68870af80bf

llvm/lib/Support/CrashRecoveryContext.cpp

 #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
+#include <windows.h> // for GetExceptionInformation
 #endif
 #if LLVM_ON_UNIX
 #include <sysexits.h> // EX_IOERR
 #endif
 
 using namespace llvm;
 
 namespace {
struct CrashRecoveryContextImpl {
   // per CrashRecoveryContext and CurrentContext is a thread-local, so only one
   // CrashRecoveryContextImpl is active per thread and this is always null.
   const CrashRecoveryContextImpl *Next;
 
   CrashRecoveryContext *CRC;
   ::jmp_buf JumpBuffer;
   volatile unsigned Failed : 1;
   unsigned SwitchedThread : 1;
+  unsigned ValidJumpBuffer : 1;
 
 public:
-  CrashRecoveryContextImpl(CrashRecoveryContext *CRC) : CRC(CRC),
-                                                        Failed(false),
-                                                        SwitchedThread(false) {
+  CrashRecoveryContextImpl(CrashRecoveryContext *CRC)
+      : CRC(CRC), Failed(false), SwitchedThread(false), ValidJumpBuffer(false) {
     Next = CurrentContext->get();
     CurrentContext->set(this);
   }
   ~CrashRecoveryContextImpl() {
     if (!SwitchedThread)
       CurrentContext->set(Next);
   }
 
void HandleCrash(int RetCode, uintptr_t Context) {
     Failed = true;
 
     if (CRC->DumpStackAndCleanupOnFailure)
       sys::CleanupOnSignal(Context);
 
     CRC->RetCode = RetCode;
 
     // Jump back to the RunSafely we were called under.
+    if (ValidJumpBuffer)
       longjmp(JumpBuffer, 1);
+
+    // Otherwise let the caller decide of the outcome of the crash. Currently
+    // this occurs when using SEH on Windows with MSVC or clang-cl.
   }
 };
-
 }
 
 static ManagedStatic<std::mutex> gCrashRecoveryContextMutex;
 static bool gCrashRecoveryEnabled = false;
 
 static ManagedStatic<sys::ThreadLocal<const CrashRecoveryContext>>
        tlIsRecoveringFromCrash;
 

 // catches exceptions if they would bubble out from the stack frame with __try /
 // __except.
 
 static void installExceptionOrSignalHandlers() {}
 static void uninstallExceptionOrSignalHandlers() {}
 
 // We need this function because the call to GetExceptionInformation() can only
 // occur inside the __except evaluation block
-static int ExceptionFilter(bool DumpStackAndCleanup,
-                           _EXCEPTION_POINTERS *Except) {
-  if (DumpStackAndCleanup)
-    sys::CleanupOnSignal((uintptr_t)Except);
-  return EXCEPTION_EXECUTE_HANDLER;
-}
-
 #if defined(__clang__) && defined(_M_IX86)
 // Work around PR44697.
 __attribute__((optnone))
-static bool InvokeFunctionCall(function_ref<void()> Fn,
-                               bool DumpStackAndCleanup, int &RetCode) {
-#else
-static bool InvokeFunctionCall(function_ref<void()> Fn,
-                               bool DumpStackAndCleanup, int &RetCode) {
 #endif
-  __try {
-    Fn();
-  } __except (ExceptionFilter(DumpStackAndCleanup, GetExceptionInformation())) {
-    RetCode = GetExceptionCode();
-    return false;
+static int
+ExceptionFilter(_EXCEPTION_POINTERS *Except) {
+  // Lookup the current thread local recovery object.
+  const CrashRecoveryContextImpl *CRCI = CurrentContext->get();
+
+  if (!CRCI) {
+    // Something has gone horribly wrong, so let's just tell everyone
+    // to keep searching
+    CrashRecoveryContext::Disable();
+    return EXCEPTION_CONTINUE_SEARCH;
   }
-  return true;
+
+  int RetCode = (int)Except->ExceptionRecord->ExceptionCode;
+
+  // Handle the crash
+  const_cast<CrashRecoveryContextImpl *>(CRCI)->HandleCrash(
+      RetCode, reinterpret_cast<uintptr_t>(Except));
+
+  return EXCEPTION_EXECUTE_HANDLER;
 }
 
 bool CrashRecoveryContext::RunSafely(function_ref<void()> Fn) {
   if (!gCrashRecoveryEnabled) {
     Fn();
     return true;
   }
-  return InvokeFunctionCall(Fn, DumpStackAndCleanupOnFailure, RetCode);
+  assert(!Impl && "Crash recovery context already initialized!");
+  Impl = new CrashRecoveryContextImpl(this);
+  __try {
+    Fn();
+  } __except (ExceptionFilter(GetExceptionInformation())) {
+    return false;
+  }
+  return true;
 }
 
 #else // !_MSC_VER
 
 #if defined(_WIN32)
 // This is a non-MSVC compiler, probably mingw gcc or clang without
 // -fms-extensions. Use vectored exception handling (VEH).
 //

 
 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;
 
+    CRCI->ValidJumpBuffer = true;
     if (setjmp(CRCI->JumpBuffer) != 0) {
       return false;
     }
   }
 
   Fn();
   return true;
 }