[clang] [clang] Apply internal buffering to clang diagnostics printing (PR #113440)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

<details>
<summary>Changes</summary>

To stabilize output of clang when clang is run in multiple build threads, the whole diagnostic message is written first to internal buffer and then the whole message is put to the output stream which usually points to stderr which is unbuffered by default. This helps to avoid printing to stderr with small chunks and interleaving of multiple diagnostic messages.

This is also fixing a slight regression that happened somewhere in clang-17. I checked clang-14 and its output is slightly more stable.
In general the tool that is run in several threads can't control output interleaving so this change might be questionable. It is up to build's system/script/tool that is running clang to handle output interleaving. However, it seems for example Make doesn't prevent compiler's output interleaving in case of many threads and gcc's output is much more stable. I was using a makefile to build 6 files within 6 thread, each file contained a warning, clang's output was like (just an illustration, it is pretty random):

<img width="1265" alt="clang-multi-before" src="https://github.com/user-attachments/assets/7664b43b-70fb-4668-a6c6-083ec2a7f081">

gcc's output using the same makefile:

<img width="307" alt="gcc-output" src="https://github.com/user-attachments/assets/374be96b-884e-4ad1-a911-a2c55302f407">

So I decided to give it a try since it could greatly improve user experience for some cases. It turned out simple and gave a relatively stable result.

clang's output after the change:

<img width="301" alt="clang-multi-after" src="https://github.com/user-attachments/assets/a2abfd17-f10d-4cef-a2e1-48fb1dda47fc">

I'm not sure how to test this properly, so the PR doesn't contain any test. But please let me know what you think, I'm open to any suggestions.

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


3 Files Affected:

- (modified) clang/include/clang/Frontend/TextDiagnostic.h (+11-1) 
- (modified) clang/lib/Frontend/TextDiagnostic.cpp (+13-3) 
- (modified) clang/lib/Frontend/TextDiagnosticPrinter.cpp (+7-3) 


``````````diff
diff --git a/clang/include/clang/Frontend/TextDiagnostic.h b/clang/include/clang/Frontend/TextDiagnostic.h
index a2fe8ae995423b..e47a111c1dcdc2 100644
--- a/clang/include/clang/Frontend/TextDiagnostic.h
+++ b/clang/include/clang/Frontend/TextDiagnostic.h
@@ -16,6 +16,7 @@
 #define LLVM_CLANG_FRONTEND_TEXTDIAGNOSTIC_H
 
 #include "clang/Frontend/DiagnosticRenderer.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace clang {
@@ -33,8 +34,15 @@ namespace clang {
 /// DiagnosticClient is implemented through this class as is diagnostic
 /// printing coming out of libclang.
 class TextDiagnostic : public DiagnosticRenderer {
-  raw_ostream &OS;
+  raw_ostream &Out;
   const Preprocessor *PP;
+  // To stabilize output of clang when clang is run in multiple build threads
+  // the whole diagnostic message is written first to internal buffer and then
+  // the whole message is put to the output stream Out which usually points to
+  // stderr to avoid printing to stderr with small chunks and interleaving of
+  // multiple diagnostic messages.
+  SmallString<1024> InternalBuffer;
+  llvm::raw_svector_ostream OS;
 
 public:
   TextDiagnostic(raw_ostream &OS, const LangOptions &LangOpts,
@@ -104,6 +112,8 @@ class TextDiagnostic : public DiagnosticRenderer {
 
   void emitBuildingModuleLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                   StringRef ModuleName) override;
+  void endDiagnostic(DiagOrStoredDiag D,
+                     DiagnosticsEngine::Level Level) override;
 
 private:
   void emitFilename(StringRef Filename, const SourceManager &SM);
diff --git a/clang/lib/Frontend/TextDiagnostic.cpp b/clang/lib/Frontend/TextDiagnostic.cpp
index 4119ce6048d45d..fddb846745aa83 100644
--- a/clang/lib/Frontend/TextDiagnostic.cpp
+++ b/clang/lib/Frontend/TextDiagnostic.cpp
@@ -656,7 +656,11 @@ static bool printWordWrapped(raw_ostream &OS, StringRef Str, unsigned Columns,
 TextDiagnostic::TextDiagnostic(raw_ostream &OS, const LangOptions &LangOpts,
                                DiagnosticOptions *DiagOpts,
                                const Preprocessor *PP)
-    : DiagnosticRenderer(LangOpts, DiagOpts), OS(OS), PP(PP) {}
+    : DiagnosticRenderer(LangOpts, DiagOpts), Out(OS), PP(PP),
+      OS(InternalBuffer) {
+  this->OS.buffer().clear();
+  this->OS.enable_colors(true);
+}
 
 TextDiagnostic::~TextDiagnostic() {}
 
@@ -664,7 +668,7 @@ void TextDiagnostic::emitDiagnosticMessage(
     FullSourceLoc Loc, PresumedLoc PLoc, DiagnosticsEngine::Level Level,
     StringRef Message, ArrayRef<clang::CharSourceRange> Ranges,
     DiagOrStoredDiag D) {
-  uint64_t StartOfLocationInfo = OS.tell();
+  uint64_t StartOfLocationInfo = Out.tell();
 
   // Emit the location of this particular diagnostic.
   if (Loc.isValid())
@@ -677,7 +681,7 @@ void TextDiagnostic::emitDiagnosticMessage(
     printDiagnosticLevel(OS, Level, DiagOpts->ShowColors);
   printDiagnosticMessage(OS,
                          /*IsSupplemental*/ Level == DiagnosticsEngine::Note,
-                         Message, OS.tell() - StartOfLocationInfo,
+                         Message, Out.tell() - StartOfLocationInfo,
                          DiagOpts->MessageLength, DiagOpts->ShowColors);
 }
 
@@ -1545,3 +1549,9 @@ void TextDiagnostic::emitParseableFixits(ArrayRef<FixItHint> Hints,
     OS << "\"\n";
   }
 }
+
+void TextDiagnostic::endDiagnostic(DiagOrStoredDiag D,
+                                   DiagnosticsEngine::Level Level) {
+  Out << OS.str();
+  OS.buffer().clear();
+}
diff --git a/clang/lib/Frontend/TextDiagnosticPrinter.cpp b/clang/lib/Frontend/TextDiagnosticPrinter.cpp
index dac5c44fe92566..6e24a19a1ad501 100644
--- a/clang/lib/Frontend/TextDiagnosticPrinter.cpp
+++ b/clang/lib/Frontend/TextDiagnosticPrinter.cpp
@@ -133,12 +133,16 @@ void TextDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
   // diagnostics in a context that lacks language options, a source manager, or
   // other infrastructure necessary when emitting more rich diagnostics.
   if (!Info.getLocation().isValid()) {
-    TextDiagnostic::printDiagnosticLevel(OS, Level, DiagOpts->ShowColors);
+    SmallString<1000> OutText;
+    llvm::raw_svector_ostream OutputBuffer(OutText);
+    OutputBuffer.enable_colors(true);
+    TextDiagnostic::printDiagnosticLevel(OutputBuffer, Level,
+                                         DiagOpts->ShowColors);
     TextDiagnostic::printDiagnosticMessage(
-        OS, /*IsSupplemental=*/Level == DiagnosticsEngine::Note,
+        OutputBuffer, /*IsSupplemental=*/Level == DiagnosticsEngine::Note,
         DiagMessageStream.str(), OS.tell() - StartOfLocationInfo,
         DiagOpts->MessageLength, DiagOpts->ShowColors);
-    OS.flush();
+    OS << OutputBuffer.str();
     return;
   }
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/113440