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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

Thanks for analyzing this!

In D70568#1763769 <https://reviews.llvm.org/D70568#1763769>, @hans wrote:

> - About making CrashRecoveryContext::Enable() the default, that seems somewhat orthogonal to this change. You mention the use case of running several compilations in parallel, but I don't think that's a scenario that happens today? If doing this is necessary, I think it would be good to break it out into a separate patch.


Even for non-parallel compilations, we still need to enable the `CrashRecoveryContext` inside the new `CC1Command`, just before calling `RunSafely()`.
If you look at a previous diff <https://reviews.llvm.org/D69825?id=227779> of D69825 <https://reviews.llvm.org/D69825>, in `clang/lib/Driver/Job.cpp, L389` there is:

  static bool CRCEnabled{};
  if (!CRCEnabled) {
    llvm::CrashRecoveryContext::Enable();
    CRCEnabled = true;
  }

Which I removed, because I find it a bit awkward to "enable" something that should be decided internally by CrashRecoveryContext (this Enable API sounds more like internal behaviour leaking outside of CrashRecoveryContext).
The purpose of Enable/Disable is either for debugging (when enabling `LIBCLANG_DISABLE_CRASH_RECOVERY`) or possibly bubbling up the crash, as described in D23662 <https://reviews.llvm.org/D23662>.
Also consider the scenario where we sequentially compile several TUs: `clang-cl a.cpp b.cpp c.cpp`. Only the first call to `CC1Command::Execute()` should call `llvm::CrashRecoveryContext::Enable()`.
It is orthogonal, I can send a separate patch. Should we instead call `llvm::CrashRecoveryContext::Enable()` in D69825 <https://reviews.llvm.org/D69825>, when `clang.exe` starts?

> - The main purpose of the patch is the new "EnableExceptionHandler" mode for CrashRecoveryContext. I find the name a bit confusing, because on Posix there's no exception handler, and on Win32 an exception handler is used to implement CrashRecoverContext::RunSafely, but that's a different one.
> 
>   What the mode really does is print a stack trace, write a minidump (on windows), and run cleanups. Perhaps a better name for that would be "DumpStackAndCleanupOnFailure"? Maybe they could be separate flags even.

Will do.

> 
> 
> - The mechanism for doing the stack dump and cleanups looks a bit scary to me. That's sys::InvokeExceptionHandler, a name which doesn't really make sense on Posix where it calls SignalHandler, and passes in the RetCode argument as the signal parameter. On Win32 it calls LLVMUnhandledExceptionFilter, and it does this inside the SEH filter function, although I think it will always return EXCEPTION_EXECUTE_HANDLER, so it could just be called inside the exception handler instead?
> 
>   On Posix, the call to SignalHandler looks scary, because it does a bunch of signal stuff, and e.g. on S390 hosts it raises the signal again to make sure to crash (which I don't think we want inside a CrashRecoveryContext).
> 
>   I wish the stack dumping and cleanup could be factored out into something that doesn't involve all the signal stuff, and that could have a clean interface that CrashRecoveryContext could simply call when it catches a crash. Maybe sys::DumpStackAndCleanup(...).  Do you think this would be possible?

I'll make it possible :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70568