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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Sorry for the slow response. This is a pretty complicated patch, so I needed some time to wrap my head around it. I'm still not entirely sure I got it right, but here are some high-level comments.

- 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.

- 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.

- 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70568