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

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
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
----------------
hans wrote:
> What is ExceptContext and what does CleanupOnSignal do with it?
Improved comment.


================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:95
 
+static void Install() {
+  if (gCrashRecoveryEnabledCount <= 0)
----------------
hans wrote:
> 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.
Reverted this part.


================
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; }
 
----------------
hans wrote:
> Don't we need to install it here?
Reverted this part.


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


================
Comment at: llvm/lib/Support/Unix/Signals.inc:356
+
+  if (!llvm::is_contained(IntSigs, Sig))
+    llvm::sys::RunSignalHandlers();
----------------
hans wrote:
> Isn't this redundant with the llvm::is_contained check above, after which we return?
Above it is checking for "Info" Sigs, whereas here it is "Int" Sigs.


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


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

https://reviews.llvm.org/D70568