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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
