[PATCH] D70568: [Support] Possibly use exception handler in the Crash Recovery Context in the same way as global exceptions

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.


================
Comment at: clang/lib/Lex/Pragma.cpp:1108
       DebugOverflowStack();
-    } else if (II->isStr("handle_crash")) {
-      llvm::CrashRecoveryContext *CRC =llvm::CrashRecoveryContext::GetCurrent();
----------------
This was added in http://llvm.org/r111449 and looks like it was just a debugging aid. Perhaps you could just remove it in a separate change (that is, without waiting for the rest of this change).


================
Comment at: llvm/include/llvm/Support/CrashRecoveryContext.h:103
+
+  /// Selects whether handling of exceptions should be done in the same way as
+  /// for global exceptions. When this is active, a crash would print the
----------------
The exception terminology only really applies on Windows. Maybe "Selects whether handling of failures should be done in the same way as for regular crashes. When this is active, ...".


================
Comment at: llvm/include/llvm/Support/Signals.h:115
+  /// - create a core/mini dump of the exception context whenever possible
+  void CleanupOnSignal(uintptr_t ExceptContext);
 } // End sys namespace
----------------
What is ExceptContext and what does CleanupOnSignal do with it?


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:65
 
-  void HandleCrash() {
+  void HandleCrash(int RetCode, uintptr_t ExceptContext) {
     // Eliminate the current context entry, to avoid re-entering in case the
----------------
It would be good to have a comment that explains the RetCode and ExceptContext parameters.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:95
 
+static void Install() {
+  if (gCrashRecoveryEnabledCount <= 0)
----------------
Style nit: function names should start with lower-case letter. (The current code is not very consistent, but at least for new names we should try.)


Also, could this just just be part of (un)installExceptionOrSignalHandlers()? It Seems like it's essentially just doing some extra checks around calls to those, and it seems unfortunate to have both "install()" and "installExceptionOrSignalHandlers()", but one should not be called directly etc.


And on a higher level, it worries me that the set of possible states (enabled/disabled, installed/uninstalled) is so large. I guess this is because CrashRecoveryContexts can be nested and enabled individually?

Could we use asserts to make it clearer what states should be possible? For example, in Install() you do "if (gCrashRecoveryEnabledCount <= 0) return". Why would we even hit this code if there are no enabled contexts? Etc.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:152
 
-void CrashRecoveryContext::Enable() {
-  std::lock_guard<std::mutex> L(*gCrashRecoveryContextMutex);
-  // FIXME: Shouldn't this be a refcount or something?
-  if (gCrashRecoveryEnabled)
-    return;
-  gCrashRecoveryEnabled = true;
-  installExceptionOrSignalHandlers();
-}
+void CrashRecoveryContext::Enable() { ++gCrashRecoveryEnabledCount; }
 
----------------
Don't we need to install it here?


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:200
+// occur inside the __except evaluation block
+static inline int ExceptionFilter(bool DumpStackAndCleanup,
+                                  _EXCEPTION_POINTERS *Except) {
----------------
I guess the "inline" is not necessary?


================
Comment at: llvm/lib/Support/Unix/Signals.inc:212
 static const int IntSigs[] = {
-  SIGHUP, SIGINT, SIGTERM, SIGUSR2
+  SIGHUP, SIGINT, SIGTERM, SIGUSR2, SIGPIPE
 };
----------------
This seems unrelated? Could it be done in a separate patch?


================
Comment at: llvm/lib/Support/Unix/Signals.inc:356
+
+  if (!llvm::is_contained(IntSigs, Sig))
+    llvm::sys::RunSignalHandlers();
----------------
Isn't this redundant with the llvm::is_contained check above, after which we return?


================
Comment at: llvm/unittests/Support/CrashRecoveryTest.cpp:110
+  EXPECT_FALSE(CRC.RunSafely(nullDeref));
+  llvm::CrashRecoveryContext::Disable();
+  // refcount = 1
----------------
Isn't this a pretty surprising API, that after calling Disable(), it's still enabled?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70568/new/

https://reviews.llvm.org/D70568