[PATCH] D33261: [CrashRecovery] Use SEH __try instead of VEH when available

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked an inline comment as done.
rnk added a comment.

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



================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:162
+#elif defined(LLVM_ON_WIN32)
+// This is a non-MSVC compiler, probably mingw gcc or clang. Use VEH.
 
----------------
zturner wrote:
> 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"?
I meant clang configured for mingw, so all that SEH jazz is probably disabled.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:247-261
+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;
+
----------------
zturner wrote:
> 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?
Sure. I copied it to avoid nested #if's, which get pretty annoying fast.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:257
   // 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);
----------------
zturner wrote:
> 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.
Let's just set it back to 1. I wasn't trying to change behavior here.


https://reviews.llvm.org/D33261