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

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:145-146
+#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.
+
----------------
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?


================
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.
 
----------------
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"?


================
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;
+
----------------
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?


https://reviews.llvm.org/D33261