[driver] Ensure that we save & replay the serialized diagnostics file for caching purposes

Previously the diagnostics from the compilation would be missing from the produced serialized diagnostics file.
There remains a small issue that the "compile job cache" remarks are not written out to the serialized diagnostics file but
I consider this a minor issue compared to missing all of the compilation diagnostics.

However the way we handle diagnostics for caching is still sub-optimal, I extended a `FIXME` with a potential approach
to improve the handling.

Author: akyrtzi

clang/include/clang/Frontend/SerializedDiagnosticPrinter.h

@@ -31,9 +31,16 @@ namespace serialized_diags {
 /// This allows wrapper tools for Clang to get diagnostics from Clang
 /// (via libclang) without needing to parse Clang's command line output.
 ///
-std::unique_ptr<DiagnosticConsumer> create(StringRef OutputFile,
-                                           DiagnosticOptions *Diags,
-                                           bool MergeChildRecords = false);
+/// \param OS optional stream to output the serialized diagnostics buffer,
+/// instead of writing out directly to a file.
+/// FIXME: \p OS is temporary transition until we have structured diagnostics
+/// caching in which case we won't need to manage serialized diagnostics files
+/// explicitly for caching purposes and the changes to add \p OS in this
+/// function should be reverted.
+std::unique_ptr<DiagnosticConsumer>
+create(StringRef OutputFile, DiagnosticOptions *Diags,
+       bool MergeChildRecords = false,
+       std::unique_ptr<raw_ostream> OS = nullptr);
 
 } // end serialized_diags namespace
 } // end clang namespace

clang/lib/Frontend/SerializedDiagnosticPrinter.cpp

@@ -142,10 +142,11 @@ class SDiagsWriter : public DiagnosticConsumer {
         State(std::move(State)) {}
 
 public:
-  SDiagsWriter(StringRef File, DiagnosticOptions *Diags, bool MergeChildRecords)
+  SDiagsWriter(StringRef File, std::unique_ptr<raw_ostream> OS,
+               DiagnosticOptions *Diags, bool MergeChildRecords)
       : LangOpts(nullptr), OriginalInstance(true),
         MergeChildRecords(MergeChildRecords),
-        State(std::make_shared<SharedState>(File, Diags)) {
+        State(std::make_shared<SharedState>(File, std::move(OS), Diags)) {
     if (MergeChildRecords)
       RemoveOldDiagnostics();
     EmitPreamble();
@@ -244,9 +245,10 @@ class SDiagsWriter : public DiagnosticConsumer {
   /// State that is shared among the various clones of this diagnostic
   /// consumer.
   struct SharedState {
-    SharedState(StringRef File, DiagnosticOptions *Diags)
+    SharedState(StringRef File, std::unique_ptr<raw_ostream> OS,
+                DiagnosticOptions *Diags)
         : DiagOpts(Diags), Stream(Buffer), OutputFile(File.str()),
-          EmittedAnyDiagBlocks(false) {}
+          OutputStream(std::move(OS)), EmittedAnyDiagBlocks(false) {}
 
     /// Diagnostic options.
     IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
@@ -260,6 +262,9 @@ class SDiagsWriter : public DiagnosticConsumer {
     /// The name of the diagnostics file.
     std::string OutputFile;
 
+    /// Optional output stream instead of writing directly to a file.
+    std::unique_ptr<raw_ostream> OutputStream;
+
     /// The set of constructed record abbreviations.
     AbbreviationMap Abbrevs;
 
@@ -297,9 +302,12 @@ class SDiagsWriter : public DiagnosticConsumer {
 
 namespace clang {
 namespace serialized_diags {
-std::unique_ptr<DiagnosticConsumer>
-create(StringRef OutputFile, DiagnosticOptions *Diags, bool MergeChildRecords) {
-  return std::make_unique<SDiagsWriter>(OutputFile, Diags, MergeChildRecords);
+std::unique_ptr<DiagnosticConsumer> create(StringRef OutputFile,
+                                           DiagnosticOptions *Diags,
+                                           bool MergeChildRecords,
+                                           std::unique_ptr<raw_ostream> OS) {
+  return std::make_unique<SDiagsWriter>(OutputFile, std::move(OS), Diags,
+                                        MergeChildRecords);
 }
 
 } // end namespace serialized_diags
@@ -796,6 +804,15 @@ void SDiagsWriter::finish() {
         getMetaDiags()->Report(diag::warn_fe_serialized_diag_merge_failure);
   }
 
+  if (State->OutputStream) {
+    // Write the generated bitstream to "Out".
+    State->OutputStream->write((char *)&State->Buffer.front(),
+                               State->Buffer.size());
+    State->OutputStream->flush();
+    State->OutputStream.reset();
+    return;
+  }
+
   std::error_code EC;
   auto OS = std::make_unique<llvm::raw_fd_ostream>(State->OutputFile.c_str(),
                                                     EC, llvm::sys::fs::OF_None);

clang/test/CAS/depscan-with-error.c

@@ -1,4 +1,4 @@
-// REQUIRES: ansi-escape-sequences
+// REQUIRES: clang-cc1daemon, ansi-escape-sequences
 
 // RUN: rm -rf %t && mkdir -p %t
 

clang/test/CAS/fcache-compile-job-serialized-diagnostics.c

@@ -24,16 +24,37 @@
 // RUN: c-index-test -read-diagnostics %t/diags 2>&1 | FileCheck %s --check-prefix=SERIALIZED-HIT
 
 // CACHE-HIT: remark: compile job cache hit
-// CACHE-HIT: warning: call to undeclared function
+// CACHE-HIT: warning: some warning
 
-// CACHE-MISS: warning: call to undeclared function
+// CACHE-MISS: warning: some warning
 // CACHE-MISS-NOT: remark: compile job cache hit
 
-// FIXME: serialized diagnostics should match the text diagnostics rdar://85234207
-// SERIALIZED-HIT: warning: compile job cache hit for
+// FIXME: serialized diagnostics should include the "compile job cache" remark but without storing
+// it in a diagnostics file that we put in the CAS.
+// SERIALIZED-HIT: warning: some warning
 // SERIALIZED-HIT: Number of diagnostics: 1
-// SERIALIZED-MISS: Number of diagnostics: 0
-
-void foo(void) {
-  bar();
-}
+// SERIALIZED-MISS: warning: some warning
+// SERIALIZED-MISS: Number of diagnostics: 1
+
+// Make sure warnings are merged with driver ones.
+// Using normal compilation as baseline.
+// RUN: %clang -target x86_64-apple-macos11 -c %s -o %t/t.o -fmodules-cache-path=%t/mcp --serialize-diagnostics %t/t1.diag \
+// RUN:   2>&1 | FileCheck %s -check-prefix=WARN
+// RUN: env LLVM_CACHE_CAS_PATH=%t/cas %clang-cache \
+// RUN:   %clang -target x86_64-apple-macos11 -c %s -o %t/t.o -fmodules-cache-path=%t/mcp --serialize-diagnostics %t/t2.diag \
+// RUN:   2>&1 | FileCheck %s -check-prefix=WARN
+// RUN: diff %t/t1.diag %t/t2.diag
+
+// Try again with cache hit.
+// RUN: rm %t/t2.diag
+// RUN: env LLVM_CACHE_CAS_PATH=%t/cas %clang-cache \
+// RUN:   %clang -target x86_64-apple-macos11 -c %s -o %t/t.o -fmodules-cache-path=%t/mcp --serialize-diagnostics %t/t2.diag
+// RUN: env LLVM_CACHE_CAS_PATH=%t/cas %clang-cache \
+// RUN:   %clang -target x86_64-apple-macos11 -c %s -o %t/t.o -fmodules-cache-path=%t/mcp --serialize-diagnostics %t/t2.diag \
+// RUN:   2>&1 | FileCheck %s -check-prefix=WARN
+// RUN: diff %t/t1.diag %t/t2.diag
+
+// WARN: warning: argument unused during compilation
+// WARN: warning: some warning
+
+#warning some warning

clang/tools/driver/cc1_main.cpp

@@ -20,10 +20,12 @@
 #include "clang/Config/config.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
+#include "clang/Frontend/ChainedDiagnosticConsumer.h"
 #include "clang/Frontend/CompileJobCacheKey.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "clang/Frontend/SerializedDiagnosticPrinter.h"
 #include "clang/Frontend/TextDiagnosticBuffer.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Frontend/Utils.h"
@@ -255,7 +257,8 @@ class CompileJobCache {
   /// Replay a cache hit.
   ///
   /// Return status if should exit immediately, otherwise None.
-  Optional<int> replayCachedResult(llvm::cas::ObjectRef ResultID,
+  Optional<int> replayCachedResult(CompilerInstance &Clang,
+                                   llvm::cas::ObjectRef ResultID,
                                    bool JustComputedResult);
 
   bool CacheCompileJob = false;
@@ -266,6 +269,7 @@ class CompileJobCache {
   std::unique_ptr<llvm::raw_ostream> ResultDiagsOS;
   IntrusiveRefCntPtr<llvm::cas::CASOutputBackend> CASOutputs;
   std::string OutputFile;
+  Optional<llvm::vfs::OutputFile> SerialDiagsOutput;
 };
 } // end anonymous namespace
 
@@ -330,32 +334,48 @@ class raw_mirroring_ostream : public llvm::raw_ostream {
 };
 } // namespace
 
+static Expected<llvm::vfs::OutputFile>
+createBinaryOutputFile(CompilerInstance &Clang, StringRef OutputPath) {
+  using namespace llvm::vfs;
+  Expected<OutputFile> O = Clang.getOrCreateOutputBackend().createFile(
+      OutputPath, OutputConfig()
+                      .setTextWithCRLF(false)
+                      .setDiscardOnSignal(true)
+                      .setAtomicWrite(true)
+                      .setImplyCreateDirectories(false));
+  if (!O)
+    return O.takeError();
+
+  O->discardOnDestroy([](llvm::Error E) { consumeError(std::move(E)); });
+  return O;
+}
+
 Optional<int> CompileJobCache::tryReplayCachedResult(CompilerInstance &Clang) {
   if (!CacheCompileJob)
     return None;
 
+  DiagnosticsEngine &Diags = Clang.getDiagnostics();
+
   // Create the result cache key once Invocation has been canonicalized.
-  ResultCacheKey = createCompileJobCacheKey(*CAS, Clang.getDiagnostics(),
-                                            Clang.getInvocation());
+  ResultCacheKey = createCompileJobCacheKey(*CAS, Diags, Clang.getInvocation());
   if (!ResultCacheKey)
     return 1;
 
   Expected<llvm::cas::CASID> Result = CAS->getCachedResult(*ResultCacheKey);
   if (Result) {
     if (Optional<llvm::cas::ObjectRef> ResultRef = CAS->getReference(*Result)) {
-      Clang.getDiagnostics().Report(diag::remark_compile_job_cache_hit)
+      Diags.Report(diag::remark_compile_job_cache_hit)
           << ResultCacheKey->toString() << Result->toString();
       Optional<int> Status =
-          replayCachedResult(*ResultRef, /*JustComputedResult=*/false);
+          replayCachedResult(Clang, *ResultRef, /*JustComputedResult=*/false);
       assert(Status && "Expected a status for a cache hit");
       return *Status;
     }
-    Clang.getDiagnostics().Report(
-        diag::remark_compile_job_cache_miss_result_not_found)
+    Diags.Report(diag::remark_compile_job_cache_miss_result_not_found)
         << ResultCacheKey->toString() << Result->toString();
   } else {
     llvm::consumeError(Result.takeError());
-    Clang.getDiagnostics().Report(diag::remark_compile_job_cache_miss)
+    Diags.Report(diag::remark_compile_job_cache_miss)
         << ResultCacheKey->toString();
   }
 
@@ -373,22 +393,57 @@ Optional<int> CompileJobCache::tryReplayCachedResult(CompilerInstance &Clang) {
   ResultDiagsOS = std::make_unique<raw_mirroring_ostream>(
       llvm::errs(), std::make_unique<llvm::raw_svector_ostream>(ResultDiags));
 
-  // FIXME: This should be saving/replaying serialized diagnostics, thus
-  // using the current llvm::errs() colour capabilities. We still want to
-  // print errors live during this compilation, just also serialize them.
+  // FIXME: This should be saving/replaying structured diagnostics, not saving
+  // stderr and a separate diagnostics file, thus using the current llvm::errs()
+  // colour capabilities and making the choice of whether colors are used, or
+  // whether a serialized diagnostics file is emitted, not affect the
+  // compilation key. We still want to print errors live during this
+  // compilation, just also serialize them. Another benefit of saving structured
+  // diagnostics is that it will enable remapping canonicalized paths in
+  // diagnostics to their non-canical form for displaying purposes
+  // (rdar://85234207).
   //
-  // However, serialized diagnostics can only be written to a file, not to a
-  // raw_ostream. Need to fix that first. Also, maybe the format doesn't need
-  // to be bitcode... we just want to serialize them faithfully such that we
-  // can decide at output time whether to make the colours pretty.
+  // Note that the serialized diagnostics file format loses information, e.g.
+  // the include stack is written as additional 'note' diagnostics but when
+  // printed in terminal the include stack is printed in a different way than
+  // 'note' diagnostics. We should serialize/deserialize diagnostics in a way
+  // that we can accurately feed them to a DiagnosticConsumer (whatever that
+  // consumer implementation is doing). A potential way is to serialize data
+  // that can be deserialized as 'StoredDiagnostic's, which would be close to
+  // what the DiagnosticConsumers expect.
 
   // Notify the existing diagnostic client that all files were processed.
   Clang.getDiagnosticClient().finish();
 
+  DiagnosticOptions &DiagOpts = Clang.getInvocation().getDiagnosticOpts();
   Clang.getDiagnostics().setClient(
-      new TextDiagnosticPrinter(*ResultDiagsOS,
-                                &Clang.getInvocation().getDiagnosticOpts()),
+      new TextDiagnosticPrinter(*ResultDiagsOS, &DiagOpts),
       /*ShouldOwnClient=*/true);
+  if (!DiagOpts.DiagnosticSerializationFile.empty()) {
+    // Save the serialized diagnostics file as CAS output.
+    if (Error E =
+            createBinaryOutputFile(Clang, DiagOpts.DiagnosticSerializationFile)
+                .moveInto(SerialDiagsOutput)) {
+      Diags.Report(diag::err_fe_unable_to_open_output)
+          << DiagOpts.DiagnosticSerializationFile
+          << errorToErrorCode(std::move(E)).message();
+      return 1;
+    }
+
+    Expected<std::unique_ptr<raw_pwrite_stream>> OS =
+        SerialDiagsOutput->createProxy();
+    if (!OS) {
+      Diags.Report(diag::err_fe_unable_to_open_output)
+          << DiagOpts.DiagnosticSerializationFile
+          << errorToErrorCode(OS.takeError()).message();
+      return 1;
+    }
+    auto SerializedConsumer = clang::serialized_diags::create(
+        OutputFile, &DiagOpts, /*MergeChildRecords*/ false, std::move(*OS));
+    Diags.setClient(new ChainedDiagnosticConsumer(
+        Diags.takeClient(), std::move(SerializedConsumer)));
+  }
+
   return None;
 }
 
@@ -398,6 +453,20 @@ void CompileJobCache::finishComputedResult(CompilerInstance &Clang,
   if (!CacheCompileJob)
     return;
 
+  if (SerialDiagsOutput) {
+    llvm::handleAllErrors(
+        SerialDiagsOutput->keep(),
+        [&](const llvm::vfs::TempFileOutputError &E) {
+          Clang.getDiagnostics().Report(diag::err_unable_to_rename_temp)
+              << E.getTempPath() << E.getOutputPath()
+              << E.convertToErrorCode().message();
+        },
+        [&](const llvm::vfs::OutputError &E) {
+          Clang.getDiagnostics().Report(diag::err_fe_unable_to_open_output)
+              << E.getOutputPath() << E.convertToErrorCode().message();
+        });
+  }
+
   // Don't cache failed builds.
   //
   // TODO: Consider caching failed builds! Note: when output files are written
@@ -432,18 +501,31 @@ void CompileJobCache::finishComputedResult(CompilerInstance &Clang,
     llvm::report_fatal_error(std::move(E));
 
   // Replay / decanonicalize as necessary.
-  Optional<int> Status = replayCachedResult(CAS->getReference(*Result),
+  Optional<int> Status = replayCachedResult(Clang, CAS->getReference(*Result),
                                             /*JustComputedResult=*/true);
   (void)Status;
   assert(Status == None);
 }
 
 /// Replay a result after a cache hit.
-Optional<int> CompileJobCache::replayCachedResult(llvm::cas::ObjectRef ResultID,
+Optional<int> CompileJobCache::replayCachedResult(CompilerInstance &Clang,
+                                                  llvm::cas::ObjectRef ResultID,
                                                   bool JustComputedResult) {
   if (JustComputedResult)
     return None;
 
+  if (!JustComputedResult) {
+    // Disable the existing DiagnosticConsumer, we'll both print to stderr
+    // directly and also potentially output a serialized diagnostics file, in
+    // which case we don't want the outer DiagnosticConsumer to overwrite it and
+    // lose the compilation diagnostics.
+    // See FIXME in CompileJobCache::tryReplayCachedResult() about improving how
+    // we handle diagnostics for caching purposes.
+    Clang.getDiagnosticClient().finish();
+    Clang.getDiagnostics().setClient(new IgnoringDiagConsumer(),
+                                     /*ShouldOwnClient=*/true);
+  }
+
   // FIXME: Stop calling report_fatal_error().
   Optional<llvm::cas::TreeProxy> Result;
   llvm::cas::TreeSchema Schema(*CAS);