hans added a comment. In D70568#1763824 <https://reviews.llvm.org/D70568#1763824>, @aganea wrote:
> 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? Yes, maybe enabling it on startup is the best way to do it. 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
