[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 cfe-commits at lists.llvm.org
Tue Dec 3 16:37:50 PST 2019


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





More information about the cfe-commits mailing list