hans added a comment.
> This is a support patch for D69825 <https://reviews.llvm.org/D69825>.
Thanks for breaking this out!
> It handles an exception in the same way as the global exception handler (same
> side-effect, print call stack & cleanup), and can return the exception code.
I'm not familiar with this code, so I was struggling with the patch title:
"Possibly use exception handler in the Crash Recovery Context in the same way
as global exceptions".
Do I understand correctly that the main point is to get a stack trace when
CrashRecoveryContext::RunSafely() fails, instead of just returning an error?
> It enables CrashRecoveryContext exceptions by default (instead of disabled
> before) - however the exception handlers are lazily installed on the first
> creation of a CrashRecoveryContext.
Why does it now need to be enabled by default?
================
Comment at: llvm/include/llvm/Support/CrashRecoveryContext.h:26
/// Clients make use of this code by first calling
/// CrashRecoveryContext::Enable(), and then executing unsafe operations via a
/// CrashRecoveryContext object. For example:
----------------
I guess this needs an update, if it's now being enabled by default?
================
Comment at: llvm/include/llvm/Support/CrashRecoveryContext.h:104
+
+ /// In case of a crash, this is the crash identifier
+ int RetCode = 0;
----------------
ultra nit: period at the end would be nice :)
================
Comment at: llvm/include/llvm/Support/Signals.h:116
+ /// ignored and is always zero.
+ signed InvokeExceptionHandler(int &RetCode, void *ExceptionInfo);
} // End sys namespace
----------------
Any reason not to use "int" for the return type here? "signed" is unusual as a
type in LLVM I think.
================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:82
+static std::atomic<bool> gCrashRecoveryEnabled{ true };
static ManagedStatic<std::mutex> gCrashRecoveryContextMutex;
----------------
" = true" would be more common in LLVM code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70568/new/
https://reviews.llvm.org/D70568
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
