[Clang][Diagnose] Minimal support on emit-include-location in sarif mode (#170415)

Currently, invoking `clang++` with `-fdiagnostics-format=sarif` causes a
crash, with stack traces indicating that
`SARIFDiagnostic::emitIncludeLocation` is unimplemented.

This PR adds minimal support for converting `In file included from ...`
and `In module ...` into `SARIF.result.relatedLocations`. With this
change, `clang++ -fdiagnostics-format=sarif` no longer crashes and now
provides a minimal amount of useful information.

Thank you.

Author: anonymouspc

clang/docs/ReleaseNotes.rst

@@ -465,6 +465,10 @@ Improvements to Clang's diagnostics
   Objective-C method and block declarations when calling format functions. It is part
   of the format-nonliteral diagnostic (#GH60718)
 
+- Fixed a crash when enabling ``-fdiagnostics-format=sarif`` and the output 
+  carries messages like 'In file included from ...' or 'In module ...'.
+  Now the include/import locations are written into `sarif.run.result.relatedLocations`.
+
 Improvements to Clang's time-trace
 ----------------------------------
 

clang/include/clang/Basic/Sarif.h

@@ -325,6 +325,7 @@ class SarifResult {
   std::string HostedViewerURI;
   llvm::SmallDenseMap<StringRef, std::string, 4> PartialFingerprints;
   llvm::SmallVector<CharSourceRange, 8> Locations;
+  llvm::SmallVector<CharSourceRange, 8> RelatedLocations;
   llvm::SmallVector<ThreadFlow, 8> ThreadFlows;
   std::optional<SarifResultLevel> LevelOverride;
 
@@ -354,16 +355,29 @@ class SarifResult {
     return *this;
   }
 
-  SarifResult setLocations(llvm::ArrayRef<CharSourceRange> DiagLocs) {
+  SarifResult addLocations(llvm::ArrayRef<CharSourceRange> DiagLocs) {
 #ifndef NDEBUG
     for (const auto &Loc : DiagLocs) {
       assert(Loc.isCharRange() &&
              "SARIF Results require character granular source ranges!");
     }
 #endif
-    Locations.assign(DiagLocs.begin(), DiagLocs.end());
+    Locations.append(DiagLocs.begin(), DiagLocs.end());
     return *this;
   }
+
+  SarifResult addRelatedLocations(llvm::ArrayRef<CharSourceRange> DiagLocs) {
+#ifndef NDEBUG
+    for (const auto &Loc : DiagLocs) {
+      assert(
+          Loc.isCharRange() &&
+          "SARIF RelatedLocations require character granular source ranges!");
+    }
+#endif
+    RelatedLocations.append(DiagLocs.begin(), DiagLocs.end());
+    return *this;
+  }
+
   SarifResult setThreadFlows(llvm::ArrayRef<ThreadFlow> ThreadFlowResults) {
     ThreadFlows.assign(ThreadFlowResults.begin(), ThreadFlowResults.end());
     return *this;

clang/include/clang/Frontend/SARIFDiagnostic.h

@@ -63,10 +63,20 @@ class SARIFDiagnostic : public DiagnosticRenderer {
                                   ArrayRef<CharSourceRange> Ranges,
                                   const Diagnostic &Diag);
 
+  SarifResult addRelatedLocationToResult(SarifResult Result, FullSourceLoc Loc,
+                                         PresumedLoc PLoc);
+
+  llvm::SmallVector<CharSourceRange>
+  getSarifLocation(FullSourceLoc Loc, PresumedLoc PLoc,
+                   ArrayRef<CharSourceRange> Ranges);
+
   SarifRule addDiagnosticLevelToRule(SarifRule Rule,
                                      DiagnosticsEngine::Level Level);
 
   llvm::StringRef emitFilename(StringRef Filename, const SourceManager &SM);
+
+  llvm::SmallVector<std::pair<FullSourceLoc, PresumedLoc>>
+      RelatedLocationsCache;
 };
 
 } // end namespace clang

clang/lib/Basic/Sarif.cpp

@@ -404,6 +404,14 @@ void SarifDocumentWriter::appendResult(const SarifResult &Result) {
     Ret["locations"] = std::move(Locs);
   }
 
+  if (!Result.RelatedLocations.empty()) {
+    json::Array ReLocs;
+    for (auto &Range : Result.RelatedLocations) {
+      ReLocs.emplace_back(createLocation(createPhysicalLocation(Range)));
+    }
+    Ret["relatedLocations"] = std::move(ReLocs);
+  }
+
   if (!Result.PartialFingerprints.empty()) {
     json::Object fingerprints = {};
     for (auto &pair : Result.PartialFingerprints) {

clang/lib/Frontend/SARIFDiagnostic.cpp

@@ -58,12 +58,48 @@ void SARIFDiagnostic::emitDiagnosticMessage(
   if (Loc.isValid())
     Result = addLocationToResult(Result, Loc, PLoc, Ranges, *Diag);
 
+  for (auto &[RelLoc, RelPLoc] : RelatedLocationsCache)
+    Result = addRelatedLocationToResult(Result, RelLoc, RelPLoc);
+  RelatedLocationsCache.clear();
+
   Writer->appendResult(Result);
 }
 
+void SARIFDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
+  // We always emit include location before results, for example:
+  //
+  // In file included from ...
+  // In file included from ...
+  // error: ...
+  //
+  // At this time We cannot peek the SarifRule. But what we
+  // do is to push it into a cache and wait for next time
+  // \ref SARIFDiagnostic::emitDiagnosticMessage to pick it up.
+  RelatedLocationsCache.push_back({Loc, PLoc});
+}
+
+void SARIFDiagnostic::emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
+                                         StringRef ModuleName) {
+  RelatedLocationsCache.push_back({Loc, PLoc});
+}
+
 SarifResult SARIFDiagnostic::addLocationToResult(
     SarifResult Result, FullSourceLoc Loc, PresumedLoc PLoc,
     ArrayRef<CharSourceRange> Ranges, const Diagnostic &Diag) {
+  auto Locations = getSarifLocation(Loc, PLoc, Ranges);
+  return Result.addLocations(Locations);
+}
+
+SarifResult SARIFDiagnostic::addRelatedLocationToResult(SarifResult Result,
+                                                        FullSourceLoc Loc,
+                                                        PresumedLoc PLoc) {
+  auto Locations = getSarifLocation(Loc, PLoc, {});
+  return Result.addRelatedLocations(Locations);
+}
+
+llvm::SmallVector<CharSourceRange>
+SARIFDiagnostic::getSarifLocation(FullSourceLoc Loc, PresumedLoc PLoc,
+                                  ArrayRef<CharSourceRange> Ranges) {
   SmallVector<CharSourceRange> Locations = {};
 
   if (PLoc.isInvalid()) {
@@ -75,7 +111,7 @@ SarifResult SARIFDiagnostic::addLocationToResult(
         // FIXME(llvm-project/issues/57366): File-only locations
       }
     }
-    return Result;
+    return {};
   }
 
   FileID CaretFileID = Loc.getExpansionLoc().getFileID();
@@ -127,10 +163,11 @@ SarifResult SARIFDiagnostic::addLocationToResult(
   SourceLocation DiagLoc = SM.translateLineCol(FID, PLoc.getLine(), ColNo);
 
   // FIXME(llvm-project/issues/57366): Properly process #line directives.
-  Locations.push_back(
-      CharSourceRange{SourceRange{DiagLoc, DiagLoc}, /* ITR = */ false});
+  CharSourceRange Range = {SourceRange{DiagLoc, DiagLoc}, /* ITR = */ false};
+  if (Range.isValid())
+    Locations.push_back(std::move(Range));
 
-  return Result.setLocations(Locations);
+  return Locations;
 }
 
 SarifRule
@@ -207,15 +244,6 @@ void SARIFDiagnostic::emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
   assert(false && "Not implemented in SARIF mode");
 }
 
-void SARIFDiagnostic::emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) {
-  assert(false && "Not implemented in SARIF mode");
-}
-
-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) {

clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp

@@ -218,7 +218,7 @@ SarifDiagnostics::createResult(const PathDiagnostic *Diag,
                     .setRuleId(CheckName)
                     .setDiagnosticMessage(Diag->getVerboseDescription())
                     .setDiagnosticLevel(SarifResultLevel::Warning)
-                    .setLocations({Range})
+                    .addLocations({Range})
                     .addPartialFingerprint(IssueHashKey, IssueHash)
                     .setHostedViewerURI(HtmlReportURL)
                     .setThreadFlows(Flows);

clang/unittests/Basic/SarifTest.cpp

@@ -290,7 +290,7 @@ TEST_F(SarifDocumentWriterTest, checkSerializingResultsWithCustomRuleConfig) {
 TEST_F(SarifDocumentWriterTest, checkSerializingArtifacts) {
   // GIVEN:
   const std::string ExpectedOutput =
-      R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0,"uri":"file:///main.cpp"},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";
+      R"({"$schema":"https://docs.oasis-open.org/sarif/sarif/v2.1.0/cos02/schemas/sarif-schema-2.1.0.json","runs":[{"artifacts":[{"length":40,"location":{"index":0,"uri":"file:///main.cpp"},"mimeType":"text/plain","roles":["resultFile"]}],"columnKind":"unicodeCodePoints","results":[{"level":"error","locations":[{"physicalLocation":{"artifactLocation":{"index":0,"uri":"file:///main.cpp"},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"message":{"text":"expected ';' after top level declarator"},"relatedLocations":[{"physicalLocation":{"artifactLocation":{"index":0,"uri":"file:///main.cpp"},"region":{"endColumn":14,"startColumn":14,"startLine":3}}}],"ruleId":"clang.unittest","ruleIndex":0}],"tool":{"driver":{"fullName":"sarif test runner","informationUri":"https://clang.llvm.org/docs/UsersManual.html","language":"en-US","name":"sarif test","rules":[{"defaultConfiguration":{"enabled":true,"level":"warning","rank":-1},"fullDescription":{"text":"Example rule created during unit tests"},"id":"clang.unittest","name":"clang unit test"}],"version":"1.0.0"}}}],"version":"2.1.0"})";    
 
   SarifDocumentWriter Writer{SourceMgr};
   const SarifRule &Rule =
@@ -312,12 +312,12 @@ TEST_F(SarifDocumentWriterTest, checkSerializingArtifacts) {
       registerSource("/main.cpp", SourceText, /* IsMainFile = */ true);
   CharSourceRange SourceCSR =
       getFakeCharSourceRange(MainFileID, {3, 14}, {3, 14});
-
   DiagLocs.push_back(SourceCSR);
 
   const SarifResult &Result =
       SarifResult::create(RuleIdx)
-          .setLocations(DiagLocs)
+          .addLocations(DiagLocs)
+          .addRelatedLocations(DiagLocs)
           .setDiagnosticMessage("expected ';' after top level declarator")
           .setDiagnosticLevel(SarifResultLevel::Error);
   Writer.appendResult(Result);
@@ -377,7 +377,7 @@ TEST_F(SarifDocumentWriterTest, checkSerializingCodeflows) {
   unsigned RuleIdx = Writer.createRule(Rule);
   const SarifResult &Result =
       SarifResult::create(RuleIdx)
-          .setLocations({DiagLoc})
+          .addLocations({DiagLoc})
           .setDiagnosticMessage("Redefinition of 'foo'")
           .setThreadFlows(Threadflows)
           .setDiagnosticLevel(SarifResultLevel::Warning);