[PATCH] D131632: [clang] Enable output of SARIF diagnostics

[Denis Nikitin via Phabricator via cfe-commits]

denik added inline comments.


================
Comment at: clang/lib/Frontend/SARIFDiagnosticPrinter.cpp:71
+  // other infrastructure necessary when emitting more rich diagnostics.
+  if (!Info.getLocation().isValid()) { // TODO: What is this case? 
+    // SARIFDiag->addDiagnosticWithoutLocation(
----------------
cjdb wrote:
> aaron.ballman wrote:
> > vaibhav.y wrote:
> > > vaibhav.y wrote:
> > > > The location maybe if the diagnostic's source is located in the scratch buffer. Likely for macro expansions where token pasting is involved. Another case would be errors on the command line.
> > > > 
> > > > I'm not entirely sure how the SARIF spec would handle this case, it might require an extension.
> > > > 
> > > > A few ways that might work could be:
> > > > 
> > > > Using the [[ https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127692 | logicalLocations ]] property to specify ([[ https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127910 | logicalLocation object ]]), this might need an extension for kind: "macro", another case that might need extension is diagnostics about invalid command line flags which are also diagnostics without a valid
> > > > 
> > > > The parentIndex for these logical locations could be set to the physical location that produced them.
> > > > 
> > > > I think this definitely warrants some discussion since the spec doesn't provide a clear way to express these cases.
> > > > 
> > > > WDYT @aaron.ballman @cjdb @denik 
> > > The spec does say for "kind":
> > > 
> > > > If none of those strings accurately describes the construct, kind MAY contain any value specified by the analysis tool.
> > > 
> > > So an extension might not be necessary, but might be worth discussing.
> > From looking through the spec, I think `logicalLocations` is probably the right choice and we'd want to make up our own kind for things like the scratch buffer or the command line. I think an extension would be worth discussing.
> We should defer this to a future CL, so that Abraham isn't blocked by our decision-making (and so we can make the right decision). I can start a GitHub issue to get the discussion in a good spot?
I agree that `logicalLocations` is a good use for the mentioned cases.

In addition, the spec mentions that `location object` can miss both physical and logical location properties in which cases it has to include the `message` property (see [[ https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127861 | 3.28.1 ]]). I think this also could be a good use case for the command line flag errors.

To unblock this change we can leave `result.locations` empty. It's allowed by the spec (see [[ https://docs.oasis-open.org/sarif/sarif/v2.0/csprd02/sarif-v2.0-csprd02.html#_Toc10127841 | 3.27.12 ]]). From what I see, `Sarif.cpp` doesn't use the source manager if `Locations` is empty.
But `locations` still has to be present with the empty list. This might need a fix in https://clang.llvm.org/doxygen/Sarif_8cpp_source.html#l00387. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131632/new/

https://reviews.llvm.org/D131632