[clang] [clang] Make serialized diagnostics more reliable (PR #100681)

[Michael Spencer via cfe-commits]

https://github.com/Bigcheese created https://github.com/llvm/llvm-project/pull/100681

`SDiagsWriter` currently requires clients to call
`DiagnosticConsumer::finish()` before it is destroyed if `CompilerInstance::ExecuteAction()` has not been called.

The problem with this is that it makes it much harder for clients to properly call `finish()` for cases such as errors from running the driver, or clients that want to emit diagnostics after executing an action, as they need to be careful to not call it twice.

This patch removes the call in `CompilerInstance::ExecuteAction()`, and instead adds a call to finish in `SDiagsWriter`'s destructor if it was not already called, but leaves the explicit calls that occur when DisableFree is set, as that skips most destructors.

No test because the issues this caused were in not yet upstreamed code, and there's reasonably good existing test coverage of the cases that have previously been hit upstream.

rdar://119221394

>From 94a78debba0ec86c833795962ffe92a769a6b5d7 Mon Sep 17 00:00:00 2001
From: Michael Spencer <bigcheesegs at gmail.com>
Date: Thu, 25 Jul 2024 18:11:23 -0700
Subject: [PATCH] [clang] Make serialized diagnostics more reliable

`SDiagsWriter` currently requires clients to call
`DiagnosticConsumer::finish()` before it is destroyed if
`CompilerInstance::ExecuteAction()` has not been called.

The problem with this is that it makes it much harder for clients to
properly call `finish()` for cases such as errors from running the
driver, or clients that want to emit diagnostics after executing an
action, as they need to be careful to not call it twice.

This patch removes the call in `CompilerInstance::ExecuteAction()`,
and instead adds a call to finish in `SDiagsWriter`'s destructor if
it was not already called, but leaves the explicit calls that occur
when DisableFree is set, as that skips most destructors.

No test because the issues this caused were in not yet upstreamed
code.
---
 clang/include/clang/Basic/Diagnostic.h             |  8 ++++++--
 clang/lib/Frontend/CompilerInstance.cpp            |  6 ------
 clang/lib/Frontend/SerializedDiagnosticPrinter.cpp | 10 ++++++++--
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 0c7836c2ea569..dea1c2b0f3913 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -1776,8 +1776,12 @@ class DiagnosticConsumer {
   /// BeginSourceFile() are inaccessible.
   virtual void EndSourceFile() {}
 
-  /// Callback to inform the diagnostic client that processing of all
-  /// source files has ended.
+  /// Callback to inform the diagnostic client that processing of all source
+  /// files has ended, and that no more diagnostics will be emitted.
+  ///
+  /// This is only called when the DiagnosticConsumer's destructor would not
+  /// otherwise be called. Subclasses that require this to be called must ensure
+  /// that in their destructor.
   virtual void finish() {}
 
   /// Indicates whether the diagnostics handled by this
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 6242b5a7d9fe3..69cea2a9141a6 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1016,12 +1016,6 @@ bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
   // better. We generally expect frontend actions to be invoked with (nearly)
   // DesiredStackSpace available.
   noteBottomOfStack();
-
-  auto FinishDiagnosticClient = llvm::make_scope_exit([&]() {
-    // Notify the diagnostic client that all files were processed.
-    getDiagnosticClient().finish();
-  });
-
   raw_ostream &OS = getVerboseOutputStream();
 
   if (!Act.PrepareToExecute(*this))
diff --git a/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp b/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
index 0887b5a504f05..41ac6956ea1a7 100644
--- a/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
+++ b/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
@@ -129,7 +129,7 @@ class SDiagsMerger : SerializedDiagnosticReader {
   void writeRecordWithBlob(unsigned ID, RecordData &Record, StringRef Blob);
 };
 
-class SDiagsWriter : public DiagnosticConsumer {
+class SDiagsWriter final : public DiagnosticConsumer {
   friend class SDiagsRenderer;
   friend class SDiagsMerger;
 
@@ -149,7 +149,13 @@ class SDiagsWriter : public DiagnosticConsumer {
     EmitPreamble();
   }
 
-  ~SDiagsWriter() override {}
+  ~SDiagsWriter() override {
+    // Not all uses of DiagnosticConsumer call finish, and not all uses invoke
+    // the destructor. This makes calling finish optional for cases where it
+    // is properly destructed.
+    if (!IsFinishing)
+      finish();
+  }
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;