[PATCH] D129220: [clang] Cleanup ASTContext before output files in crash recovery for modules

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 442706.
benlangmuir added a comment.

Updated per out-of-band feedback from @steven_wu :

- Added an assert to `clearOutputFiles` that the `ASTConsumer` is removed. Normally this would be done in `FrontendAction::EndSourceFile`.  This should help avoid regressions and also means it is covered by existing tests.
- Updated `FailureCleanup` in `FrontendAction::BeginSourceFile` to reset the `ASTConsumer`. This is needed due to the assertion, but I don't think it changes much in practice since there would also be no output files at this stage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129220

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp


Index: clang/lib/Frontend/FrontendAction.cpp
===================================================================
--- clang/lib/Frontend/FrontendAction.cpp
+++ clang/lib/Frontend/FrontendAction.cpp
@@ -581,6 +581,7 @@
   auto FailureCleanup = llvm::make_scope_exit([&]() {
     if (HasBegunSourceFile)
       CI.getDiagnosticClient().EndSourceFile();
+    CI.setASTConsumer(nullptr);
     CI.clearOutputFiles(/*EraseFiles=*/true);
     CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
     setCurrentInput(FrontendInputFile());
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -757,6 +757,8 @@
 // Output Files
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
+  // The ASTConsumer can own streams that write to the output files.
+  assert(!hasASTConsumer() && "ASTConsumer should be reset");
   // Ignore errors that occur when trying to discard the temp file.
   for (OutputFile &OF : OutputFiles) {
     if (EraseFiles) {
@@ -1235,8 +1237,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,9 +1250,15 @@
                                             diag::remark_module_build_done)
     << ModuleName;
 
-  // Delete any remaining temporary files related to Instance, in case the
-  // module generation thread crashed.
-  Instance.clearOutputFiles(/*EraseFiles=*/true);
+  if (Crashed) {
+    // Clear the ASTConsumer if it hasn't been already, in case it owns streams
+    // that must be closed before clearing output files.
+    Instance.setSema(nullptr);
+    Instance.setASTConsumer(nullptr);
+
+    // Delete any remaining temporary files related to Instance.
+    Instance.clearOutputFiles(/*EraseFiles=*/true);
+  }
 
   // If \p AllowPCMWithCompilerErrors is set return 'success' even if errors
   // occurred.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129220.442706.patch
Type: text/x-patch
Size: 2232 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220706/358d7d7c/attachment.bin>