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

[Hans Wennborg via Phabricator via cfe-commits]

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