[clang] Make serialized diagnostics more reliable

[Bigcheese]

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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Michael Spencer (Bigcheese)

Changes

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

---

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

3 Files Affected:

• (modified) clang/include/clang/Basic/Diagnostic.h (+6-2)
• (modified) clang/lib/Frontend/CompilerInstance.cpp (-6)
• (modified) clang/lib/Frontend/SerializedDiagnosticPrinter.cpp (+8-2)

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;

[Bigcheese]

Fixed cc1_main not calling finish with DisableFree.

[jansvoboda11]

Could we simplify this even further by removing DiagnosticConsumer::finish() entirely, moving the code in SDiagsWriter::finish() to SDiagsWriter::~SDiagsWriter() and ensuring the destructor gets called even with -disable-free?

Also, can you please create and link a downstream PR that adds a test case?

[Bigcheese]

Could we simplify this even further by removing DiagnosticConsumer::finish() entirely, moving the code in SDiagsWriter::finish() to SDiagsWriter::~SDiagsWriter() and ensuring the destructor gets called even with -disable-free?

This is doable, but I'm not sure it's better. It's a bit weird to be required to destroy a single member of a member of CompilerInstance, but not anything else.

Also, can you please create and link a downstream PR that adds a test case?

Sure, it's just invoking the libclang dependency scanning API with an invalid driver command line.

[jansvoboda11]

Could we simplify this even further by removing DiagnosticConsumer::finish() entirely, moving the code in SDiagsWriter::finish() to SDiagsWriter::~SDiagsWriter() and ensuring the destructor gets called even with -disable-free?

This is doable, but I'm not sure it's better. It's a bit weird to be required to destroy a single member of a member of CompilerInstance, but not anything else.

Ok, that's a fair point. Let's keep it as is then.

Also, can you please create and link a downstream PR that adds a test case?

Sure, it's just invoking the libclang dependency scanning API with an invalid driver command line.

Thanks. I'm happy to approve this PR once we have the test.

[benlangmuir]

There's another call to finish() in DependencyScanningWorker that should be removed if we do this.