benlangmuir created this revision.
benlangmuir added reviewers: steven_wu, akyrtzi.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
When we recover from a crash in a module compilation thread, we need to ensure
any output streams owned by the `ASTConsumer` (e.g. in
`RawPCHContainerGenerator`) are deleted before we call `clearOutputFiles()`.
This has the same theoretical issues with proxy streams that Duncan discusses
in the commit 2d133867833fe8eb
<https://reviews.llvm.org/rG2d133867833fe8eb20c11377ff1221f71afc1db3>. In
practice, this was observed as a use-after-free crash on a downstream branch
that uses such a proxy stream in this code path.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D129220
Files:
clang/lib/Frontend/CompilerInstance.cpp
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1235,8 +1235,7 @@
// Execute the action to actually build the module in-place. Use a separate
// thread so that we get a stack large enough.
- llvm::CrashRecoveryContext CRC;
- CRC.RunSafelyOnThread(
+ bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
[&]() {
GenerateModuleFromModuleMapAction Action;
Instance.ExecuteAction(Action);
@@ -1249,6 +1248,13 @@
diag::remark_module_build_done)
<< ModuleName;
+ // Clear the ASTConsumer if it hasn't been already, in case it owns streams
+ // that must be closed before clearing output files.
+ if (Crashed) {
+ Instance.setSema(nullptr);
+ Instance.setASTConsumer(nullptr);
+ }
+
// Delete any remaining temporary files related to Instance, in case the
// module generation thread crashed.
Instance.clearOutputFiles(/*EraseFiles=*/true);
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1235,8 +1235,7 @@
// Execute the action to actually build the module in-place. Use a separate
// thread so that we get a stack large enough.
- llvm::CrashRecoveryContext CRC;
- CRC.RunSafelyOnThread(
+ bool Crashed = !llvm::CrashRecoveryContext().RunSafelyOnThread(
[&]() {
GenerateModuleFromModuleMapAction Action;
Instance.ExecuteAction(Action);
@@ -1249,6 +1248,13 @@
diag::remark_module_build_done)
<< ModuleName;
+ // Clear the ASTConsumer if it hasn't been already, in case it owns streams
+ // that must be closed before clearing output files.
+ if (Crashed) {
+ Instance.setSema(nullptr);
+ Instance.setASTConsumer(nullptr);
+ }
+
// Delete any remaining temporary files related to Instance, in case the
// module generation thread crashed.
Instance.clearOutputFiles(/*EraseFiles=*/true);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
