[clang] [clang-tools-extra] [clang][FrontendAction] Extract EndSourceFileForDiagnostics to fix clangd assertion on preprocessed inputs (#197662) (PR #209157)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Macro Terra (hongtaihu)

<details>
<summary>Changes</summary>

### Summary

Empty `.i` files (and `-x cpp-output` with `.c`/`.cc`/`.m`/`.mm`) crash clangd with `"Compiler instance has no preprocessor!"` during shutdown, because `EndSourceFileAction()` unconditionally accesses the preprocessor for preprocessed inputs, but the preprocessor has been detached by `~ParsedAST()`.

### Root Cause

`ParsedAST::~ParsedAST()` detaches the preprocessor from `CompilerInstance` (via `setPreprocessor(nullptr)`) before calling `EndSourceFile()`, to prevent a duplicate end-of-file notification. But `EndSourceFile()` calls `EndSourceFileAction()`, whose default implementation unconditionally calls `getPreprocessor()` for preprocessed inputs — hitting the assertion.

The existing workaround in `~ParsedAST` was already marked `XXX: This is messy` — it manually detached the preprocessor to avoid repeated EOF. This patch replaces that with a clean flag-based approach.

### Fix

Extract the preprocessor and diagnostic-client end-of-file notification into a new public method `EndSourceFileForDiagnostics()`, guarded by an `EndOfFileSignaled` flag to prevent double notification.

- `EndSourceFile()` now calls `EndSourceFileForDiagnostics()` then proceeds with cleanup as before. External behavior unchanged.
- clangd calls `EndSourceFileForDiagnostics()` at build time to flush clang-tidy diagnostics, then `EndSourceFile()` at destruction handles the rest. The flag prevents repeated EOF, so the `setPreprocessor(nullptr)` hack is no longer needed.
- Since the preprocessor is never detached, `EndSourceFileAction()` can safely access it.

### Testing

`ParsedASTTests.PreprocessedInputLifecycle` covers both the empty `.i` case and `-x c++-cpp-output`.

Fixes #<!-- -->197662.


---
Full diff: https://github.com/llvm/llvm-project/pull/209157.diff


4 Files Affected:

- (modified) clang-tools-extra/clangd/ParsedAST.cpp (+5-14) 
- (modified) clang-tools-extra/clangd/unittests/ParsedASTTests.cpp (+11) 
- (modified) clang/include/clang/Frontend/FrontendAction.h (+9) 
- (modified) clang/lib/Frontend/FrontendAction.cpp (+15-6) 


``````````diff
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index df56420cd7f24..5cfdc29d0a990 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -752,15 +752,12 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     CTFinder.matchAST(Clang->getASTContext());
   }
 
-  // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF.
-  // However Action->EndSourceFile() would destroy the ASTContext!
-  // So just inform the preprocessor of EOF, while keeping everything alive.
-  PP.EndSourceFile();
+  // Clang-tidy checks flush some diagnostics at EOF, but we need to retain the
+  // AST until ParsedAST is destroyed.
+  Action->EndSourceFileForDiagnostics();
   // UnitDiagsConsumer is local, we can not store it in CompilerInstance that
   // has a longer lifetime.
   Clang->getDiagnostics().setClient(new IgnoreDiagnostics);
-  // CompilerInstance won't run this callback, do it directly.
-  ASTDiags.EndSourceFile();
 
   std::vector<Diag> Diags = CompilerInvocationDiags;
   // FIXME: Also skip generation of diagnostics altogether to speed up ast
@@ -787,14 +784,8 @@ ParsedAST::ParsedAST(ParsedAST &&Other) = default;
 ParsedAST &ParsedAST::operator=(ParsedAST &&Other) = default;
 
 ParsedAST::~ParsedAST() {
-  if (Action) {
-    // We already notified the PP of end-of-file earlier, so detach it first.
-    // We must keep it alive until after EndSourceFile(), Sema relies on this.
-    auto PP = Clang->getPreprocessorPtr(); // Keep PP alive for now.
-    Clang->setPreprocessor(nullptr);       // Detach so we don't send EOF again.
-    Action->EndSourceFile();               // Destroy ASTContext and Sema.
-    // Now Sema is gone, it's safe for PP to go out of scope.
-  }
+  if (Action)
+    Action->EndSourceFile(); // Destroy ASTContext and Sema.
 }
 
 ASTContext &ParsedAST::getASTContext() { return Clang->getASTContext(); }
diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index f9752d5d44f97..3b07f31230547 100644
--- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -266,6 +266,17 @@ TEST(ParsedASTTest, NoCrashOnTokensWithTidyCheck) {
   EXPECT_EQ(T.expandedTokens().drop_back().back().text(SM), "}");
 }
 
+TEST(ParsedASTTest, PreprocessedInputLifecycle) {
+  TestTU TU;
+  TU.Filename = "TestTU.i";
+  TU.Code = "";
+  TU.build();
+
+  TU.Filename = "TestTU.cc";
+  TU.ExtraArgs = {"-x", "c++-cpp-output"};
+  TU.build();
+}
+
 TEST(ParsedASTTest, CanBuildInvocationWithUnknownArgs) {
   MockFS FS;
   FS.Files = {{testPath("foo.cpp"), "void test() {}"}};
diff --git a/clang/include/clang/Frontend/FrontendAction.h b/clang/include/clang/Frontend/FrontendAction.h
index 08c5fbc78f8ae..1b7f13f55a8d6 100644
--- a/clang/include/clang/Frontend/FrontendAction.h
+++ b/clang/include/clang/Frontend/FrontendAction.h
@@ -38,6 +38,7 @@ class FrontendAction {
   FrontendInputFile CurrentInput;
   std::unique_ptr<ASTUnit> CurrentASTUnit;
   CompilerInstance *Instance;
+  bool EndOfFileSignaled = false;
   friend class ASTMergeAction;
   friend class WrapperFrontendAction;
 
@@ -244,6 +245,14 @@ class FrontendAction {
   /// Set the source manager's main input file, and run the action.
   llvm::Error Execute();
 
+  /// Notify the preprocessor and diagnostic client that the source file has
+  /// ended, without destroying per-file frontend state.
+  ///
+  /// This is useful for clients that need end-of-file diagnostics while
+  /// retaining the AST. The matching EndSourceFile() performs the remaining
+  /// cleanup and will not notify them a second time.
+  void EndSourceFileForDiagnostics();
+
   /// Perform any per-file post processing, deallocate per-file
   /// objects, and run statistics and output file cleanup code.
   virtual void EndSourceFile();
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 55e3877384622..f6f0ff344517f 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -831,6 +831,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
   assert(!Input.isEmpty() && "Unexpected empty filename!");
   setCurrentInput(Input);
   setCompilerInstance(&CI);
+  EndOfFileSignaled = false;
 
   bool HasBegunSourceFile = false;
   bool ReplayASTFile = Input.getKind().getFormat() == InputKind::Precompiled &&
@@ -1362,17 +1363,25 @@ llvm::Error FrontendAction::Execute() {
   return llvm::Error::success();
 }
 
-void FrontendAction::EndSourceFile() {
+void FrontendAction::EndSourceFileForDiagnostics() {
   CompilerInstance &CI = getCompilerInstance();
 
-  // Inform the preprocessor we are done.
+  if (EndOfFileSignaled)
+    return;
+
+  // Inform the preprocessor and diagnostic client we are done with this source
+  // file. Do this in order so that end-of-file preprocessor callbacks can
+  // report diagnostics.
   if (CI.hasPreprocessor())
     CI.getPreprocessor().EndSourceFile();
-
-  // Inform the diagnostic client we are done with this source file.
-  // Do this after notifying the preprocessor, so that end-of-file preprocessor
-  // callbacks can report diagnostics.
   CI.getDiagnosticClient().EndSourceFile();
+  EndOfFileSignaled = true;
+}
+
+void FrontendAction::EndSourceFile() {
+  CompilerInstance &CI = getCompilerInstance();
+
+  EndSourceFileForDiagnostics();
 
   // Finalize the action.
   EndSourceFileAction();

``````````

</details>


https://github.com/llvm/llvm-project/pull/209157