[Clang][Diagnose] Minimal support on emit-include-location in sarif mode

[anonymouspc]

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.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: anonymous (anonymouspc)

Changes

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.

---

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

4 Files Affected:

• (modified) clang/include/clang/Basic/Sarif.h (+13)
• (modified) clang/include/clang/Frontend/SARIFDiagnostic.h (+9-1)
• (modified) clang/lib/Basic/Sarif.cpp (+8)
• (modified) clang/lib/Frontend/SARIFDiagnostic.cpp (+41-14)

diff --git a/clang/include/clang/Basic/Sarif.h b/clang/include/clang/Basic/Sarif.h
index a88d1ee2965a9..154b72d7aa520 100644
--- a/clang/include/clang/Basic/Sarif.h
+++ b/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;
 
@@ -364,6 +365,18 @@ class SarifResult {
     Locations.assign(DiagLocs.begin(), DiagLocs.end());
     return *this;
   }
+
+  SarifResult setRelatedLocations(llvm::ArrayRef<CharSourceRange> DiagLocs) {
+#ifndef NDEBUG
+    for (const auto &Loc : DiagLocs) {
+      assert(Loc.isCharRange() &&
+             "SARIF Results require character granular source ranges!");
+    }
+#endif
+    RelatedLocations.assign(DiagLocs.begin(), DiagLocs.end());
+    return *this;
+  }
+
   SarifResult setThreadFlows(llvm::ArrayRef<ThreadFlow> ThreadFlowResults) {
     ThreadFlows.assign(ThreadFlowResults.begin(), ThreadFlowResults.end());
     return *this;
diff --git a/clang/include/clang/Frontend/SARIFDiagnostic.h b/clang/include/clang/Frontend/SARIFDiagnostic.h
index 780f36c874109..5efe665b05ba9 100644
--- a/clang/include/clang/Frontend/SARIFDiagnostic.h
+++ b/clang/include/clang/Frontend/SARIFDiagnostic.h
@@ -58,15 +58,23 @@ class SARIFDiagnostic : public DiagnosticRenderer {
   // Shared between SARIFDiagnosticPrinter and this renderer.
   SarifDocumentWriter *Writer;
 
-  SarifResult addLocationToResult(SarifResult Result, FullSourceLoc Loc,
+  SarifResult addLocationToResult(SarifResult Result, FullSourceLoc Loc, 
                                   PresumedLoc PLoc,
                                   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
diff --git a/clang/lib/Basic/Sarif.cpp b/clang/lib/Basic/Sarif.cpp
index b3fb9a21249e9..448de96d474af 100644
--- a/clang/lib/Basic/Sarif.cpp
+++ b/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) {
diff --git a/clang/lib/Frontend/SARIFDiagnostic.cpp b/clang/lib/Frontend/SARIFDiagnostic.cpp
index ac27d7480de3e..33c3b83496a1b 100644
--- a/clang/lib/Frontend/SARIFDiagnostic.cpp
+++ b/clang/lib/Frontend/SARIFDiagnostic.cpp
@@ -58,12 +58,47 @@ 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) {
+    ArrayRef<CharSourceRange> Ranges,
+    const Diagnostic& Diag) {
+  auto Locations = getSarifLocation(Loc, PLoc, Ranges);
+  return Result.setLocations(Locations);
+}
+
+SarifResult SARIFDiagnostic::addRelatedLocationToResult(
+    SarifResult Result, FullSourceLoc Loc, PresumedLoc PLoc) {
+  auto Locations = getSarifLocation(Loc, PLoc, {});
+  return Result.setRelatedLocations(Locations);
+}
+
+llvm::SmallVector<CharSourceRange> SARIFDiagnostic::getSarifLocation(
+    FullSourceLoc Loc, PresumedLoc PLoc, ArrayRef<CharSourceRange> Ranges) {
   SmallVector<CharSourceRange> Locations = {};
 
   if (PLoc.isInvalid()) {
@@ -75,7 +110,7 @@ SarifResult SARIFDiagnostic::addLocationToResult(
         // FIXME(llvm-project/issues/57366): File-only locations
       }
     }
-    return Result;
+    return {};
   }
 
   FileID CaretFileID = Loc.getExpansionLoc().getFileID();
@@ -127,10 +162,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 +243,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) {

[anonymouspc]

@haoNoQ @Xazax-hun @steakhal
I'm a new contributor and I hope for a code review. Thank you sincerely!

[AaronBallman]

Thank you for the fix! I think you should add a release note to clang/docs/ReleaseNotes.rst so users know about the fixed crash. I think it probably also makes sense to add a test case to clang/unittests/Basic/SarifTest.cpp showing the behavior of include locations and demonstrating the fix.

[AaronBallman]

FYI, lldb precommit CI failure appears to be unrelated to the changes in this PR.

[anonymouspc]

Thank you for the fix! I think you should add a release note to clang/docs/ReleaseNotes.rst so users know about the fixed crash. I think it probably also makes sense to add a test case to clang/unittests/Basic/SarifTest.cpp showing the behavior of include locations and demonstrating the fix.

Sure!

• I added a note to the release notes.
• I modified the SarifDocumentWriterTest.checkSerializingArtifacts test in clang/unittests/Basic/SarifTest.cpp. The SarifDocumentWriterTest now checks the serialization of both Location and the new relatedLocation.
  • It seems that the existing tests of SarifTest.cpp focuses on "whether the SarifDocumentWriter can correctly serialize a given message", instead of "how we produce a new diagnostic", So I simply added the same DiagLoc (as the Location one above) to the relatedLocation here as well.

Thanks for your code review @AaronBallman ! Happy post-thanksgiving. :)