[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.

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;