[Diagnostics] Accept newline and format diag opts on first line

[vangthao]

Summary

Accept newlines when the diagnostic string is being given by an outside source
instead of ignoring newlines. I believe the other processing done in
FormatDiagnostic() does not ignore newlines so we should also not ignore it
here.

Also format diagnostic options on the first line when there are multiple lines.

The motivation behind this patch is to allow optimization remarks from the
backend to be able to format its output with newlines. Prior to this review if
we use newlines with optimization remarks, they would be ignored and this makes
it dificult to do any sort of information display formatting if we want to
output various type of information at once as was attempted in
https://reviews.llvm.org/D123878

[martong]

@NoQ @steakhal FYI

[steakhal]

Please, consider updating the summary to clearly specify the motivation for making this change.
It describes what this patch intends to do, but I'm about the why.

[vangthao]

Update commit message to include the motivation behind the patch.

[scott.linder]

Thank you for the patch! I will leave it to others with more knowledge of the history of diagnostics to comment on whether the premise is acceptable, but I have a few technical comments

clang/lib/Frontend/TextDiagnosticPrinter.cpp

119–137

Nit: I'd remove the ifs and just use the behavior of slice(npos, ...) returning the empty string

I also would reword "split the first line and feed it into printDiagnosticOptions", as it could be interpreted as meaning the first line is an input to printDiagnosticOptions

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

99–106

It's frustrating that std::string::insert doesn't follow the pattern of npos==end-of-string, but I'd still suggest only doing the actual insert/append once

100

I believe they are guaranteed to be equivalent, but this should be StringRef::npos or you should allocate the std::string sooner

101

I think you incur an extra copy assignment here, as the insert returns an lvalue-reference

[scott.linder]

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

99–106

Or to be more precise, I would try to only have one piece of code doing the insert/append

[steakhal]

Thanks for updating the summary.

I'm not too confident around diagnostics though.
I'm going to piggyback on @scott.linder's comments. I think they are on the point.

nit: I'm not sure if size_t is a thing in c++, std::size_t is though.

clang/lib/Basic/Diagnostic.cpp

815

Should we account for '\r'-s as well? I'm not sure how it works though

clang/lib/Frontend/TextDiagnosticPrinter.cpp

119–137

Use braces for both branches or no braces at all. LLVM CodingStandards

[aaron.ballman]

I'm a bit uncomfortable with this as we (at least currently) want to discourage non-terse diagnostics, and allowing newlines encourages longer diagnostics. There's an RFC kicking around about making more expressive diagnostics and so this discomfort may pass with time. (The only three uses of \n in our current Clang diagnostics are all driver diagnostics and all three of them are new for the HLSL stuff, wrong, and need to be reworded. CC @beanz to look into fixing up https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DiagnosticDriverKinds.td#L674).

Is there a reason the remarks can't use individual diagnostic emissions to simulate newlines? Or is this perhaps a demonstration that the remarks should not be using the diagnostic engine at all and should be emitting their output to a user-controllable stream (or file)?

[vangthao]

In D127923#3599451, @aaron.ballman wrote:

Is there a reason the remarks can't use individual diagnostic emissions to simulate newlines? Or is this perhaps a demonstration that the remarks should not be using the diagnostic engine at all and should be emitting their output to a user-controllable stream (or file)?

An issue with using multiple diagnostic emissions to simulate newlines is that there are repeated information such as diagnostic location and diagopts reprinted for each line. This creates a lot of noise and makes the output harder to read. Accepting newlines in diagnostics would allow for a lot more flexibility but I do see your point on how it would also encourage longer diagnostics.

[aaron.ballman]

In D127923#3600422, @vangthao wrote:

In D127923#3599451, @aaron.ballman wrote:

Is there a reason the remarks can't use individual diagnostic emissions to simulate newlines? Or is this perhaps a demonstration that the remarks should not be using the diagnostic engine at all and should be emitting their output to a user-controllable stream (or file)?

An issue with using multiple diagnostic emissions to simulate newlines is that there are repeated information such as diagnostic location and diagopts reprinted for each line. This creates a lot of noise and makes the output harder to read.

I can see how that's the case if the prefix information is different with each diagnostic line, but if they're all repeating the same prefix, that noise should be lessened by quite a bit (and for those truly distracted by it, using the same prefix each time makes it easier to strip the prefix in an editor). e.g.,

remark: foo.cl:27:0: 6 instructions in function
remark: foo.cl:27:0: Kernel Name: test_kernel
remark: foo.cl:27:0: SGPRs: 24
remark: foo.cl:27:0: VGPRs: 9
remark: foo.cl:27:0: AGPRs: 43
remark: foo.cl:27:0: ScratchSize [bytes/thread]: 0
remark: foo.cl:27:0: Occupancy [waves/SIMD]: 5
remark: foo.cl:27:0: SGPRs Spill: 0
remark: foo.cl:27:0: VGPRs Spill: 0
remark: foo.cl:27:0: LDS Size [bytes/block]: 512
remark: foo.cl:27:0: ------------------------------

doesn't look that terrible to me because all the salient information is visually aligned. (Line wrapping would be an issue, but the same is true for line wrapping mixed with manual newlines.) Do you have the ability to control the location of the diagnostic so you can keep the prefix the same throughout the report (or use the same prefix within a notional "section" of the report so that blocks of related remarks are visually grouped with the same prefix)?

[arsenm]

My objection to the original patch was including things like the "----" lines as remarks to mark a section. I don't think these should be emitted as remarks and are a display function the frontend should take care of if we want any kind of grouping

clang/lib/Basic/Diagnostic.cpp

FormatDiagnostic(const char *DiagStr, const char *DiagEnd,
   // When the diagnostic string is only "%0", the entire string is being given
   // by an outside source.  Remove unprintable characters from this string
   // and skip all the other string processing.
   if (DiagEnd - DiagStr == 2 &&
       StringRef(DiagStr, DiagEnd - DiagStr).equals("%0") &&
       getArgKind(0) == DiagnosticsEngine::ak_std_string) {
     const std::string &S = getArgStdStr(0);
     for (char c : S) {
-      if (llvm::sys::locale::isPrint(c) || c == '\t') {
+      if (llvm::sys::locale::isPrint(c) || c == '\t' || c == '\n') {
         OutStr.push_back(c);
       }
     }
     return;
   }
 
   /// FormattedArgs - Keep track of all of the arguments formatted by
   /// ConvertArgToString and pass them into subsequent calls to

clang/lib/Frontend/TextDiagnosticPrinter.cpp

void TextDiagnosticPrinter::HandleDiagnostic(DiagnosticsEngine::Level Level,
   // Default implementation (Warnings/errors count).
   DiagnosticConsumer::HandleDiagnostic(Level, Info);
 
   // Render the diagnostic message into a temporary buffer eagerly. We'll use
   // this later as we print out the diagnostic to the terminal.
   SmallString<100> OutStr;
   Info.FormatDiagnostic(OutStr);
 
-  llvm::raw_svector_ostream DiagMessageStream(OutStr);
+  // If the diagnostic message is formatted into multiple lines, split the
+  // first line and feed it into printDiagnosticOptions so that we print the
+  // options only on the first line instead of the last line.
+  SmallString<100> InitStr;
+  size_t NewlinePos = OutStr.find_first_of('\n');
+  if (NewlinePos != StringRef::npos) {
+    for (size_t I = 0; I != NewlinePos; ++I)
+      InitStr.push_back(OutStr[I]);
+  } else
+    InitStr = OutStr;
+
+  llvm::raw_svector_ostream DiagMessageStream(InitStr);
   printDiagnosticOptions(DiagMessageStream, Level, Info, *DiagOpts);
 
+  // Append the remaining diagnostic message if there were multiple lines.
+  if (NewlinePos != StringRef::npos)
+    DiagMessageStream << OutStr.slice(NewlinePos, OutStr.size());
+
   // Keeps track of the starting position of the location
   // information (e.g., "foo.c:10:4:") that precedes the error
   // message. We use this information to determine how long the
   // file+line+column number prefix is.
   uint64_t StartOfLocationInfo = OS.tell();
 
   if (!Prefix.empty())
     OS << Prefix << ": ";
 

clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

auto reportPiece = [&](unsigned ID, FullSourceLoc Loc, StringRef String,
         }
       }
     };
 
     for (std::vector<const PathDiagnostic *>::iterator I = Diags.begin(),
          E = Diags.end();
          I != E; ++I) {
       const PathDiagnostic *PD = *I;
+      std::string OutStr;
       std::string WarningMsg = (DiagOpts.ShouldDisplayDiagnosticName
                                     ? " [" + PD->getCheckerName() + "]"
                                     : "")
                                    .str();
 
-      reportPiece(WarnID, PD->getLocation().asLocation(),
-                  (PD->getShortDescription() + WarningMsg).str(),
+      size_t NewlinePos = PD->getShortDescription().find_first_of('\n');
+      if (NewlinePos != std::string::npos)
+        OutStr = PD->getShortDescription().str().insert(NewlinePos, WarningMsg);
+      else
+        OutStr = (PD->getShortDescription() + WarningMsg).str();
+
+      reportPiece(WarnID, PD->getLocation().asLocation(), OutStr,
                   PD->path.back()->getRanges(), PD->path.back()->getFixits());
 
       // First, add extra notes, even if paths should not be included.
       for (const auto &Piece : PD->path) {
         if (!isa<PathDiagnosticNotePiece>(Piece.get()))
           continue;
 
         reportPiece(NoteID, Piece->getLocation().asLocation(),
                     Piece->getString(), Piece->getRanges(),

clang/test/Analysis/padding_message.cpp

 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -std=c++14 -analyzer-checker=optin.performance -analyzer-config optin.performance.Padding:AllowedPad=2 -verify %s
 
 // expected-warning@+7{{\
-Excessive padding in 'struct IntSandwich' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'struct IntSandwich' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
 struct IntSandwich {
   char c1;
   int i;
   char c2;
 };
 
 // expected-warning@+7{{\
-Excessive padding in 'struct TurDuckHen' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'struct TurDuckHen' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
 struct TurDuckHen {
   char c1;
   struct IntSandwich i;
   char c2;
 };
 
 #pragma pack(push)
 #pragma pack(2)
 // expected-warning@+11{{\
-Excessive padding in 'struct SmallIntSandwich' (4 padding bytes, where 0 is optimal). \
-Optimal fields order: \
-i1, \
-i2, \
-i3, \
-c1, \
-c2, \
-c3, \
-c4, \
+Excessive padding in 'struct SmallIntSandwich' (4 padding bytes, where 0 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i1, \n\
+i2, \n\
+i3, \n\
+c1, \n\
+c2, \n\
+c3, \n\
+c4, \n\
 }}
 struct SmallIntSandwich {
   char c1;
   int i1;
   char c2;
   int i2;
   char c3;
   int i3;
   char c4;
 };
 #pragma pack(pop)
 
 union SomeUnion { // no-warning
   char c;
   short s;
   int i;
 };
 
 // expected-warning@+7{{\
-Excessive padding in 'struct HoldsAUnion' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-u, \
-c1, \
-c2, \
+Excessive padding in 'struct HoldsAUnion' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+u, \n\
+c1, \n\
+c2, \n\
 }}
 struct HoldsAUnion {
   char c1;
   union SomeUnion u;
   char c2;
 };
 
 struct SmallCharArray { // no-warning
   char c[5];
 };
 
 struct MediumIntArray { // no-warning
   int i[5];
 };
 
 // expected-warning@+7{{\
-Excessive padding in 'struct StructSandwich' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-m, \
-s, \
-s2, \
+Excessive padding in 'struct StructSandwich' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+m, \n\
+s, \n\
+s2, \n\
 }}
 struct StructSandwich {
   struct SmallCharArray s;
   struct MediumIntArray m;
   struct SmallCharArray s2;
 };
 
 // expected-warning@+7{{\
-Excessive padding in 'TypedefSandwich' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'TypedefSandwich' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
 typedef struct {
   char c1;
   int i;
   char c2;
 } TypedefSandwich;
 
 // expected-warning@+7{{\
-Excessive padding in 'struct StructAttrAlign' (10 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'struct StructAttrAlign' (10 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
 struct StructAttrAlign {
   char c1;
   int i;
   char c2;
 } __attribute__((aligned(8)));
 
 // expected-warning@+8{{\
-Excessive padding in 'struct OverlyAlignedChar' (8185 padding bytes, where 4089 is optimal). \
-Optimal fields order: \
-c, \
-c1, \
-c2, \
-x, \
+Excessive padding in 'struct OverlyAlignedChar' (8185 padding bytes, where 4089 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+c, \n\
+c1, \n\
+c2, \n\
+x, \n\
 }}
 struct OverlyAlignedChar {
   char c1;
   int x;
   char c2;
   char c __attribute__((aligned(4096)));
 };
 
 // expected-warning@+7{{\
-Excessive padding in 'struct HoldsOverlyAlignedChar' (8190 padding bytes, where 4094 is optimal). \
-Optimal fields order: \
-o, \
-c1, \
-c2, \
+Excessive padding in 'struct HoldsOverlyAlignedChar' (8190 padding bytes, where 4094 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+o, \n\
+c1, \n\
+c2, \n\
 }}
 struct HoldsOverlyAlignedChar {
   char c1;
   struct OverlyAlignedChar o;
   char c2;
 };
 
 void internalStructFunc() {
   // expected-warning@+7{{\
-Excessive padding in 'struct X' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-t, \
-c1, \
-c2, \
+Excessive padding in 'struct X' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+t, \n\
+c1, \n\
+c2, \n\
 }}
   struct X {
     char c1;
     int t;
     char c2;
   };
   struct X obj;
 }
 
 void typedefStructFunc() {
   // expected-warning@+7{{\
-Excessive padding in 'S' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-t, \
-c1, \
-c2, \
+Excessive padding in 'S' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+t, \n\
+c1, \n\
+c2, \n\
 }}
   typedef struct {
     char c1;
     int t;
     char c2;
   } S;
   S obj;
 }
 
 // expected-warning@+7{{\
-Excessive padding in 'struct DefaultAttrAlign' (22 padding bytes, where 6 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'struct DefaultAttrAlign' (22 padding bytes, where 6 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
 struct DefaultAttrAlign {
   char c1;
   long long i;
   char c2;
 } __attribute__((aligned));
 
 // expected-warning@+7{{\
-Excessive padding in 'struct SmallArrayShortSandwich' (2 padding bytes, where 0 is optimal). \
-Optimal fields order: \
-s, \
-c1, \
-c2, \
+Excessive padding in 'struct SmallArrayShortSandwich' (2 padding bytes, where 0 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+s, \n\
+c1, \n\
+c2, \n\
 }}
 struct SmallArrayShortSandwich {
   char c1;
   short s;
   char c2;
 } ShortArray[20];
 
 // expected-warning@+7{{\
-Excessive padding in 'struct SmallArrayInFunc' (2 padding bytes, where 0 is optimal). \
-Optimal fields order: \
-s, \
-c1, \
-c2, \
+Excessive padding in 'struct SmallArrayInFunc' (2 padding bytes, where 0 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+s, \n\
+c1, \n\
+c2, \n\
 }}
 struct SmallArrayInFunc {
   char c1;
   short s;
   char c2;
 };
 
 void arrayHolder() {
   struct SmallArrayInFunc Arr[15];
 }
 
 // expected-warning@+7{{\
-Excessive padding in 'class VirtualIntSandwich' (10 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'class VirtualIntSandwich' (10 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
 class VirtualIntSandwich {
   virtual void foo() {}
   char c1;
   int i;
   char c2;
 };
 
 // constructed so as not to have tail padding
 // expected-warning@+8{{\
-Excessive padding in 'class InnerPaddedB' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i1, \
-i2, \
-c1, \
-c2, \
+Excessive padding in 'class InnerPaddedB' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i1, \n\
+i2, \n\
+c1, \n\
+c2, \n\
 }}
 class InnerPaddedB {
   char c1;
   int i1;
   char c2;
   int i2;
 };
 
 class Empty {}; // no-warning
 
 // expected-warning@+7{{\
-Excessive padding in 'class LotsOfSpace' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-e1, \
-e2, \
+Excessive padding in 'class LotsOfSpace' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+e1, \n\
+e2, \n\
 }}
 class LotsOfSpace {
   Empty e1;
   int i;
   Empty e2;
 };
 
 // expected-warning@+7{{\
-Excessive padding in 'struct TypedefSandwich2' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-t, \
-c1, \
-c2, \
+Excessive padding in 'struct TypedefSandwich2' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+t, \n\
+c1, \n\
+c2, \n\
 }}
 typedef struct TypedefSandwich2 {
   char c1;
   // expected-warning@+7{{\
-Excessive padding in 'TypedefSandwich2::NestedTypedef' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-i, \
-c1, \
-c2, \
+Excessive padding in 'TypedefSandwich2::NestedTypedef' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+i, \n\
+c1, \n\
+c2, \n\
 }}
   typedef struct {
     char c1;
     int i;
     char c2;
   } NestedTypedef;
   NestedTypedef t;
   char c2;
 } TypedefSandwich2;
 
 template <typename T>
 struct Foo {
   // expected-warning@+7{{\
-Excessive padding in 'struct Foo<int>::Nested' (6 padding bytes, where 2 is optimal). \
-Optimal fields order: \
-t, \
-c1, \
-c2, \
+Excessive padding in 'struct Foo<int>::Nested' (6 padding bytes, where 2 is optimal).  [optin.performance.Padding]\n\
+Optimal fields order: \n\
+t, \n\
+c1, \n\
+c2, \n\
 }}
   struct Nested {
     char c1;
     T t;
     char c2;
   };
 };
 
 struct Holder { // no-warning
   Foo<int>::Nested t1;
   Foo<char>::Nested t2;
 };