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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Frontend/SARIFDiagnostic.cpp:78
+            emitFilename(FE->getName(), Loc.getManager());
+        // FIXME: No current way to add file-only location to SARIF object
+      }
----------------
cjdb wrote:
> I think it would be good to file an issue on GitHub and change to `FIXME(llvm-project/<issue>)`. @aaron.ballman WDYT?
I'd be fine with that.


================
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?
SGTM (I don't consider this to be blocking for this patch, FWIW).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131632