[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. It also adds a call where DisableFree would
otherwise prevent the destructor from getting called.

No test because the issues this caused were in not yet upstreamed
code.

Author: Bigcheese

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

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))

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;

clang/tools/driver/cc1_main.cpp

@@ -26,6 +26,7 @@
 #include "clang/Frontend/Utils.h"
 #include "clang/FrontendTool/Utils.h"
 #include "clang/Serialization/ObjectFilePCHContainerReader.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Config/llvm-config.h"
@@ -274,10 +275,21 @@ int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
                                   static_cast<void*>(&Clang->getDiagnostics()));
 
   DiagsBuffer->FlushDiagnostics(Clang->getDiagnostics());
-  if (!Success) {
+
+  auto FinishDiagnosticClient = [&]() {
+    // Notify the diagnostic client that all files were processed.
     Clang->getDiagnosticClient().finish();
+
+    // Our error handler depends on the Diagnostics object, which we're
+    // potentially about to delete. Uninstall the handler now so that any
+    // later errors use the default handling behavior instead.
+    llvm::remove_fatal_error_handler();
+  };
+  auto FinishDiagnosticClientScope =
+      llvm::make_scope_exit([&]() { FinishDiagnosticClient(); });
+
+  if (!Success)
     return 1;
-  }
 
   // Execute the frontend actions.
   {
@@ -314,10 +326,9 @@ int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
     }
   }
 
-  // Our error handler depends on the Diagnostics object, which we're
-  // potentially about to delete. Uninstall the handler now so that any
-  // later errors use the default handling behavior instead.
-  llvm::remove_fatal_error_handler();
+  // Call this before the Clang pointer is moved below.
+  FinishDiagnosticClient();
+  FinishDiagnosticClientScope.release();
 
   // When running with -disable-free, don't do any destruction or shutdown.
   if (Clang->getFrontendOpts().DisableFree) {