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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
