This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by cjdb on Mar 2 2023, 3:16 PM.

Details

Summary

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

Depends on D146654.

Diff Detail

Event Timeline

cjdb created this revision.Mar 2 2023, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 3:16 PM
cjdb requested review of this revision.Mar 2 2023, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2023, 3:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cjdb added a subscriber: denik.Mar 2 2023, 3:25 PM
cjdb updated this revision to Diff 502020.Mar 2 2023, 4:12 PM

actually commits the files

cjdb updated this revision to Diff 502021.Mar 2 2023, 4:15 PM

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

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–231

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

cjdb updated this revision to Diff 502186.Mar 3 2023, 10:55 AM

sorts artifacts so that they're output in index order

cjdb added a comment.EditedMar 3 2023, 10:56 AM
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–231

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 updated this revision to Diff 502190.Mar 3 2023, 10:58 AM

fixes string goof

aaron.ballman added inline comments.Mar 6 2023, 11:55 AM
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–231

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 added inline comments.Mar 21 2023, 10:12 AM
clang/lib/Frontend/SARIFDiagnostic.cpp
214–231

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).

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

clang/lib/Frontend/SARIFDiagnostic.cpp
214–231

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 updated this revision to Diff 507137.Mar 21 2023, 3:01 PM

fixes breakage and adds FIXME

cjdb updated this revision to Diff 507497.Mar 22 2023, 2:10 PM
cjdb edited the summary of this revision. (Show Details)

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

cjdb updated this revision to Diff 507511.Mar 22 2023, 2:30 PM
cjdb edited the summary of this revision. (Show Details)

And on this CL's side too

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 accepted this revision.Mar 23 2023, 7:21 AM

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

clang/lib/Basic/Sarif.cpp
314–317
This revision is now accepted and ready to land.Mar 23 2023, 7:21 AM
cjdb added a comment.Mar 23 2023, 8:58 AM

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.