[clang] fixes header processing for `-fdiagnostics-format=sarif`

[cjdb]

Summary

Including headers used to fire an assertion; now they report a
diagnostic similarly to unstructured diagnostics.

Depends on D146654.

[cjdb]

actually commits the files

[cjdb]

fixes something that wasn't supposed to be changed in the rebase

[aaron.ballman]

It looks like there's a valid precommit CI failure that needs to be addressed:

********************
Failed Tests (1):
  Clang :: Frontend/sarif-diagnostics.cpp

clang/lib/Frontend/SARIFDiagnostic.cpp

214–229

Why do we want -1 as the rule ID and... can we use "-1" instead of doing a string conversion?

[cjdb]

sorts artifacts so that they're output in index order

[cjdb]

This comment has been deleted.

clang/lib/Basic/Sarif.cpp

314–317

I'm wondering if I should instead copy CurrentArtifacts to a vector and sort prior to insertion, rather than in post.

clang/lib/Frontend/SARIFDiagnostic.cpp

214–229

lol at obvious C++ goof.

Re -1, there doesn't seem to be a diagnostic associated with this note, so I picked a value that I know isn't in use.

[cjdb]

fixes string goof

[aaron.ballman]

clang/lib/Basic/Sarif.cpp

314–317

I think this is okay, but might be interesting to see what has better perf. Given that this is 1) related to issuing diagnostics, and 2) dumping data onto disk, I am not super concerned about perf for this until we see something show up in a profiler.

clang/lib/Frontend/SARIFDiagnostic.cpp

214–229

Rather than have these functions devise their own diagnostic IDs, should we make some SARIF-specific notes in the diagnostics system that we can use more directly? (Might be overkill for the first such note here, but the other emitFooLocation() functions make me think we're going to want this wrapped in a helper sooner rather than later.)

[cjdb]

clang/lib/Frontend/SARIFDiagnostic.cpp

214–229

Since it's entirely string-based, we can probably evolve a naming scheme for these over time. The numbers are currently useless, so I'm okay with having "-1" for now though. We should come up with a strategy on whether or not we want Microsoft-like codes, but I'm pretty against that (and I think my opinions are on the record, but happy to reiterate).

[aaron.ballman]

(Note, precommit CI on Windows still shows a valid failure.)

clang/lib/Frontend/SARIFDiagnostic.cpp

214–229

We discussed this offline a bit and yeah, we're going to have to come up with some sort of naming scheme for these eventually. For the moment, -1 is fine for this one use case, but you should probably add a FIXME comment about needing to replace the -1 with something better in the future.

[cjdb]

fixes breakage and adds FIXME

[cjdb]

swaps commits to see if that fixes CI (part 1)

[cjdb]

And on this CL's side too

[aaron.ballman]

In D145201#4214395, @cjdb wrote:

swaps commits to see if that fixes CI (part 1)

Oh, if this is just stacked patches confusing our precommit CI, then it's okay. I hadn't realized this was a stacked patch.

[aaron.ballman]

LGTM aside from a tiny variable naming nit I missed before.

clang/lib/Basic/Sarif.cpp

314–317

[cjdb]

In D145201#4216567, @aaron.ballman wrote:

In D145201#4214395, @cjdb wrote:

swaps commits to see if that fixes CI (part 1)

Oh, if this is just stacked patches confusing our precommit CI, then it's okay. I hadn't realized this was a stacked patch.

No, I introduced a new patch and it was weirdly failing everything, so I decided to see if it could be merged before this one.

clang/include/clang/Frontend/SARIFDiagnostic.h

void emitBuildingModuleLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                   StringRef ModuleName) override;
 
 private:
   // Shared between SARIFDiagnosticPrinter and this renderer.
   SarifDocumentWriter *Writer;
 
   SarifResult addLocationToResult(SarifResult Result, FullSourceLoc Loc,
                                   PresumedLoc PLoc,
-                                  ArrayRef<CharSourceRange> Ranges,
-                                  const Diagnostic &Diag);
+                                  ArrayRef<CharSourceRange> Ranges);
 
   SarifRule addDiagnosticLevelToRule(SarifRule Rule,
                                      DiagnosticsEngine::Level Level);
 
   llvm::StringRef emitFilename(StringRef Filename, const SourceManager &SM);
 };
 
 } // end namespace clang
 
 #endif

clang/lib/Basic/Sarif.cpp

for (const auto &Pair : CurrentArtifacts) {
     if (!A.Roles.empty())
       Artifact["roles"] = json::Array(A.Roles);
     if (!A.MimeType.empty())
       Artifact["mimeType"] = A.MimeType;
     if (A.Offset.has_value())
       Artifact["offset"] = *A.Offset;
     Artifacts->push_back(json::Value(std::move(Artifact)));
   }
+  llvm::sort(*Artifacts, [](const json::Value &x, const json::Value &y) {
+    return x.getAsObject()->getNumber("index") <
+           y.getAsObject()->getNumber("index");
+  });
 
   // Clear, reset temporaries before next run.
   reset();
 
   // Mark the document as closed.
   Closed = true;
 }
 

clang/lib/Frontend/SARIFDiagnostic.cpp

 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Locale.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
+#include <sstream>
 #include <string>
 
 namespace clang {
 
 SARIFDiagnostic::SARIFDiagnostic(raw_ostream &OS, const LangOptions &LangOpts,
                                  DiagnosticOptions *DiagOpts,
                                  SarifDocumentWriter *Writer)
     : DiagnosticRenderer(LangOpts, DiagOpts), Writer(Writer) {}
void SARIFDiagnostic::emitDiagnosticMessage(
   Rule = addDiagnosticLevelToRule(Rule, Level);
 
   unsigned RuleIdx = Writer->createRule(Rule);
 
   SarifResult Result =
       SarifResult::create(RuleIdx).setDiagnosticMessage(Message);
 
   if (Loc.isValid())
-    Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag);
+    Result = addLocationToResult(Result, Loc, PLoc, Ranges);
 
   Writer->appendResult(Result);
 }
 
-SarifResult SARIFDiagnostic::addLocationToResult(
-    SarifResult Result, FullSourceLoc Loc, PresumedLoc PLoc,
-    ArrayRef<CharSourceRange> Ranges, const Diagnostic &Diag) {
+SarifResult
+SARIFDiagnostic::addLocationToResult(SarifResult Result, FullSourceLoc Loc,
+                                     PresumedLoc PLoc,
+                                     ArrayRef<CharSourceRange> Ranges) {
   SmallVector<CharSourceRange> Locations = {};
 
   if (PLoc.isInvalid()) {
     // At least add the file name if available:
     FileID FID = Loc.getFileID();
     if (FID.isValid()) {
       if (const FileEntry *FE = Loc.getFileEntry()) {
         emitFilename(FE->getName(), Loc.getManager());

 /// ranges necessary.
 void SARIFDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
                                         DiagnosticsEngine::Level Level,
                                         ArrayRef<CharSourceRange> Ranges) {
   assert(false && "Not implemented in SARIF mode");
 }
 
 void SARIFDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  assert(false && "Not implemented in SARIF mode");
+  SarifRule Rule = SarifRule::create().setRuleId("-1");
+  Rule = addDiagnosticLevelToRule(Rule, DiagnosticsEngine::Level::Note);
+
+  unsigned RuleIdx = Writer->createRule(Rule);
+
+  SmallString<200> MessageStorage;
+  llvm::raw_svector_ostream Message(MessageStorage);
+  Message << "in file included from " << PLoc.getFilename() << ':'
+          << PLoc.getLine() << ":\n";
+  SarifResult Result =
+      SarifResult::create(RuleIdx).setDiagnosticMessage(Message.str());
+
+  if (Loc.isValid())
+    Result = addLocationToResult(Result, Loc, PLoc, {});
+
+  Writer->appendResult(Result);
 }
 
 void SARIFDiagnostic::emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
                                          StringRef ModuleName) {
   assert(false && "Not implemented in SARIF mode");
 }
 
 void SARIFDiagnostic::emitBuildingModuleLocation(FullSourceLoc Loc,
                                                  PresumedLoc PLoc,
                                                  StringRef ModuleName) {
   assert(false && "Not implemented in SARIF mode");
 }
 } // namespace clang

clang/test/Frontend/sarif-diagnostics.hpp

+Test test;

clang/test/Frontend/sarif-diagnostics.cpp

 }
 }
 
 struct t1 { };
 void f1(t1 x, t1 y) {
     x + y;
 }
 
+#include "sarif-diagnostics.hpp"
+
 // RUN: %clang -fsyntax-only -Wall -Wextra -fdiagnostics-format=sarif %s > %t.txt 2>&1 || true
 // RUN: FileCheck -dump-input=always %s --input-file=%t.txt --check-prefixes=STDERR,SARIF
 
 // STDERR: warning: diagnostic formatting in SARIF mode is currently unstable [-Wsarif-format-unstable]
 // SARIF: {
 // SARIF:   "$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json",
 // SARIF:   "runs":[
 // SARIF:     {
 // SARIF:       "artifacts":[
 // SARIF:         {
 // SARIF:           "length":{{[0-9]+}},
 // SARIF:           "location":{
 // SARIF:             "index":0,
 // SARIF:             "uri":{{"file://[^"]+/clang/test/Frontend/sarif-diagnostics.cpp"}}
 // SARIF:           },
 // SARIF:           "mimeType":"text/plain",
 // SARIF:           "roles":[
 // SARIF:             "resultFile"
 // SARIF:           ]
+// SARIF:         },
+// SARIF:         {
+// SARIF:           "length":{{[0-9]+}},
+// SARIF:           "location":{
+// SARIF:             "index":1,
+// SARIF:             "uri":"file://{{[^"]+test/Frontend/sarif-diagnostics.hpp}}"
+// SARIF:           },
+// SARIF:           "mimeType":"text/plain",
+// SARIF:           "roles":[
+// SARIF:             "resultFile"
+// SARIF:           ]
 // SARIF:         }
 // SARIF:       ],
 // SARIF:       "columnKind":"unicodeCodePoints",
 // SARIF:       "results":[
 // SARIF:         {
 // SARIF:           "level":"error",
 // SARIF:           "locations":[
 // SARIF:             {

 // SARIF:         },
 // SARIF:         {
 // SARIF:           "level":"warning",
 // SARIF:           "locations":[
 // SARIF:             {
 // SARIF:               "physicalLocation":{
 // SARIF:                 "artifactLocation":{
 // SARIF:                   "index":0,
-// SARIF:                   "uri":{{"file://[^"]+/clang/test/Frontend/sarif-diagnostics.cpp"}}
+// SARIF:                   "uri":{{"file://[^"]+clang/test/Frontend/sarif-diagnostics.cpp"}}
 // SARIF:                 },
 // SARIF:                 "region":{
 // SARIF:                   "endColumn":10,
 // SARIF:                   "startColumn":10,
 // SARIF:                   "startLine":18
 // SARIF:                 }
 // SARIF:               }
 // SARIF:             }
 // SARIF:           ],
 // SARIF:           "message":{
 // SARIF:             "text":"unused variable 'Yes'"
 // SARIF:           },
 // SARIF:           "ruleId":"6593",
 // SARIF:           "ruleIndex":5
 // SARIF:         },
 // SARIF:         {
 // SARIF:           "level":"error",
 // SARIF:           "locations":[
 // SARIF:             {
 // SARIF:               "physicalLocation":{
 // SARIF:                 "artifactLocation":{
 // SARIF:                   "index":0,
-// SARIF:                   "uri":{{"file://[^"]+/clang/test/Frontend/sarif-diagnostics.cpp"}}
+// SARIF:                   "uri":{{"file://[^"]+clang/test/Frontend/sarif-diagnostics.cpp"}}
 // SARIF:                 },
 // SARIF:                 "region":{
 // SARIF:                   "endColumn":12,
 // SARIF:                   "startColumn":12,
 // SARIF:                   "startLine":21
 // SARIF:                 }
 // SARIF:               }
 // SARIF:             }

 // SARIF:               }
 // SARIF:             }
 // SARIF:           ],
 // SARIF:           "message":{
 // SARIF:             "text":"invalid operands to binary expression ('t1' and 't1')"
 // SARIF:           },
 // SARIF:           "ruleId":"4567",
 // SARIF:           "ruleIndex":8
+// SARIF:         },
+// SARIF:         {
+// SARIF:           "level":"note",
+// SARIF:           "locations":[
+// SARIF:             {
+// SARIF:               "physicalLocation":{
+// SARIF:                 "artifactLocation":{
+// SARIF:                   "index":0,
+// SARIF:                   "uri":{{"file://[^"]+/clang/test/Frontend/sarif-diagnostics.cpp"}}
+// SARIF:                 },
+// SARIF:                 "region":{
+// SARIF:                   "endColumn":10,
+// SARIF:                   "startColumn":10,
+// SARIF:                   "startLine":30
+// SARIF:                 }
+// SARIF:               }
+// SARIF:             }
+// SARIF:           ],
+// SARIF:           "message":{
+// SARIF:             "text":"in file included from {{[^"]+test/Frontend/sarif-diagnostics.cpp:30:}}\n"
+// SARIF:           },
+// SARIF:           "ruleId":"-1",
+// SARIF:           "ruleIndex":9
+// SARIF:         },
+// SARIF:         {
+// SARIF:           "level":"error",
+// SARIF:           "locations":[
+// SARIF:             {
+// SARIF:               "physicalLocation":{
+// SARIF:                 "artifactLocation":{
+// SARIF:                   "index":1,
+// SARIF:                   "uri":"file:///{{[^"]+/test/Frontend/sarif-diagnostics.hpp}}"
+// SARIF:                 },
+// SARIF:                 "region":{
+// SARIF:                   "endColumn":1,
+// SARIF:                   "startColumn":1,
+// SARIF:                   "startLine":1
+// SARIF:                 }
+// SARIF:               }
+// SARIF:             }
+// SARIF:           ],
+// SARIF:           "message":{
+// SARIF:             "text":"unknown type name 'Test'"
+// SARIF:           },
+// SARIF:           "ruleId":"4657",
+// SARIF:           "ruleIndex":10
 // SARIF:         }
 // SARIF:       ],
 // SARIF:       "tool":{
 // SARIF:         "driver":{
 // SARIF:           "fullName":"",
 // SARIF:           "informationUri":"https://clang.llvm.org/docs/UsersManual.html",
 // SARIF:           "language":"en-US",
 // SARIF:           "name":"clang",

 // SARIF:                 "level":"error",
 // SARIF:                 "rank":50
 // SARIF:               },
 // SARIF:               "fullDescription":{
 // SARIF:                 "text":""
 // SARIF:               },
 // SARIF:               "id":"4567",
 // SARIF:               "name":""
+// SARIF:             },
+// SARIF:             {
+// SARIF:               "defaultConfiguration":{
+// SARIF:                 "enabled":true,
+// SARIF:                 "level":"note",
+// SARIF:                 "rank":-1
+// SARIF:               },
+// SARIF:               "fullDescription":{
+// SARIF:                 "text":""
+// SARIF:               },
+// SARIF:               "id":"-1",
+// SARIF:               "name":""
+// SARIF:             },
+// SARIF:             {
+// SARIF:               "defaultConfiguration":{
+// SARIF:                 "enabled":true,
+// SARIF:                 "level":"error",
+// SARIF:                 "rank":50
+// SARIF:               },
+// SARIF:               "fullDescription":{
+// SARIF:                 "text":""
+// SARIF:               },
+// SARIF:               "id":"4657",
+// SARIF:               "name":""
 // SARIF:             }
 // SARIF:           ],
 // SARIF:           "version":"17.0.0"
 // SARIF:         }
 // SARIF:       }
 // SARIF:     }
 // SARIF:   ],
 // SARIF:   "version":"2.1.0"
 // SARIF: }
-// STDERR: 2 warnings and 6 errors generated.
+// STDERR: 2 warnings and 7 errors generated.