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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

This is really cool; we've wanted this for a long time.

One concern I have is that I think this will interfere (effectively disable) automatic OS handling of these crashes, which means they won't be collated and reported anymore.  Can we make this configurable somehow?  (E.g., leave behind an `-ffork-cc1` `-fno-fork-cc1` and a CMake flag to pick?  Or just the CMake flag?)

(I also have a couple of minor nits inline.)



================
Comment at: llvm/lib/Support/CrashRecoveryContext.cpp:114
 
+CrashRecoveryContext::CrashRecoveryContext() : Impl(nullptr), head(nullptr) {
+  Install();
----------------
Can you move the `nullptr` default assignments back to the header, so it's still obvious how they are set up?


================
Comment at: llvm/lib/Support/Unix/Signals.inc:348
 
+signed sys::InvokeExceptionHandler(int &RetCode, void *) {
+  SignalHandler(RetCode);
----------------
I'm not sure I've seen `signed` in code before.  Might it be simpler to just say `int` for the return type?  Or is that somehow incorrect/confusing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70568