EndSourceFile() for preprocessor before diagnostic client

[dbartol]

The comment for DiagnosticConsumer::BeginSourceFile() states that "diagnostics with source range information are required to only be emitted in between BeginSourceFile() and EndSourceFile().". While working on some upcoming changes to the static analyzer, we hit some crashes when diagnostics were reported from the EndOfMainFile callback in the preprocessor. This turned out to be because FrontEndAction::EndSourceFile() notifies the diagnostic clients of the end of the source file before it notifies the preprocessor. Thus, the diagnostics from the preprocessor callback are reported when the diagnostic client is no longer expecting any diagnostics.

The fix is to swap the order of the EndSourceFile() calls so that the preprocessor is notified first.

I've added asserts to the ClangTidyDiagnosticConsumer to catch unexpected diagnostics outside of a source file. Before swapping the order of the calls as described above, this causes several failures in the clang-tidy regression tests. With the swap, there are no failures in check-all.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: Dave Bartolomeo (dbartol)

Changes

The comment for DiagnosticConsumer::BeginSourceFile() states that "diagnostics with source range information are required to only be emitted in between BeginSourceFile() and EndSourceFile().". While working on some upcoming changes to the static analyzer, we hit some crashes when diagnostics were reported from the EndOfMainFile callback in the preprocessor. This turned out to be because FrontEndAction::EndSourceFile() notifies the diagnostic clients of the end of the source file before it notifies the preprocessor. Thus, the diagnostics from the preprocessor callback are reported when the diagnostic client is no longer expecting any diagnostics.

The fix is to swap the order of the EndSourceFile() calls so that the preprocessor is notified first.

I've added asserts to the ClangTidyDiagnosticConsumer to catch unexpected diagnostics outside of a source file. Before swapping the order of the calls as described above, this causes several failures in the clang-tidy regression tests. With the swap, there are no failures in check-all.

---

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

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp (+2)
• (modified) clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h (+16)
• (modified) clang/lib/Frontend/FrontendAction.cpp (+5-3)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index a71813a103df3..5f140703c47d8 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -360,6 +360,8 @@ getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix) {
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
+  assert(InSourceFile || Info.getLocation().isInvalid());  // A diagnostic should not be reported outside of a BeginSourceFile()/EndSourceFile() pair if it has a source location.
+
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
     return;
 
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index a8851e794f24b..2bf5a65fb8264 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -292,6 +292,21 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
+  void BeginSourceFile(const LangOptions &LangOpts,
+                               const Preprocessor *PP = nullptr) override {
+    DiagnosticConsumer::BeginSourceFile(LangOpts, PP);
+    
+    assert(!InSourceFile);
+    InSourceFile = true;
+  }
+    
+  void EndSourceFile() override {
+    assert(InSourceFile);
+    InSourceFile = false;
+      
+    DiagnosticConsumer::EndSourceFile();
+  }
+
   // Retrieve the diagnostics that were captured.
   std::vector<ClangTidyError> take();
 
@@ -326,6 +341,7 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
   bool LastErrorRelatesToUserCode = false;
   bool LastErrorPassesLineFilter = false;
   bool LastErrorWasIgnored = false;
+  bool InSourceFile = false;
 };
 
 } // end namespace tidy
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index ef7ae27a2694a..f5996a8e1e88b 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -1243,13 +1243,15 @@ llvm::Error FrontendAction::Execute() {
 void FrontendAction::EndSourceFile() {
   CompilerInstance &CI = getCompilerInstance();
 
-  // Inform the diagnostic client we are done with this source file.
-  CI.getDiagnosticClient().EndSourceFile();
-
   // Inform the preprocessor we are done.
   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();
+
   // Finalize the action.
   EndSourceFileAction();
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[dbartol]

Similar (but old) change: 88d99e4

[zwuis]

Please add a tag to the beginning of the PR title in square brackets like other PRs. E.g. [clang-tidy].

[zwuis]

// ...
assert(...);

Do not write long comment on same line as the code.