[clang] Emit SARIF Diagnostics: Create `clang::SarifDocumentWriter` interface

[vaibhav.y]

Summary

Create an interface for writing SARIF documents from within clang:

The primary intent of this change is to introduce the interface clang::SarifDocumentWriter, which allows incrementally adding diagnostic data to a JSON backed document. The proposed interface is not yet connected to the compiler internals, which will be covered in future work. As such this change will not change the input/output interface of clang.

Previous discussions:

• RFC for this change: https://lists.llvm.org/pipermail/cfe-dev/2021-March/067907.html
• https://lists.llvm.org/pipermail/cfe-dev/2021-July/068480.html

See also:

• https://discourse.llvm.org/t/rfc-improving-clang-s-diagnostics/62584

SARIF Standard (2.1.0):

• https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html

[vaibhav.y]

Format code using patch from buildkite

[vaibhav.y]

Ping.

clang-format is the only check failing as of now (attempting to reformat the <a> links in doc comments.

[aaron.ballman]

Thanks for your patience with this review! I'm currently in WG14 meetings this week and out on vacation next week, so my review availability is a bit limited at the moment (sorry for that).

I think this is heading in the right direction, but there are some lifetime issues we need to figure out how to resolve. I pointed out a few such places, but I did not do an exhaustive search to catch them all.

clang/include/clang/Basic/Sarif.h

81–83

One thing that's worth calling out is that StringRef is non-owning which means that the argument passed to create the SarifArtifactLocation has to outlive the returned object to avoid dangling references. This makes the class a bit more dangerous to use because Twine or automatic std::string objects may cause lifetime concerns.

Should these classes be storing a std::string so that the memory is owned by SARIF?

117

Loc is already a SarifArtifactLocation, did you mean SarifArtifact by any chance?

(Note, this suggests to me we should mark the ctor's here as explicit.)

230–231

244–246

This comment looks incorrect to me.

368

It'd be good to explain why createRule() returns a value here and above.

379

clang/include/clang/Basic/SourceLocation.h

441–456

Looks like unrelated formatting changes; feel free to submit these as an NFC change if you'd like, but the changes should be reverted from this patch for clarity.

clang/lib/Basic/Sarif.cpp

200

This seems like it'll be a use-after-free because the local std::string will be destroyed before the lifetime of the SarifArtifactLocation ends.

208–210

No worries! Our style is not... all that typical... so it can be hard to remember.

[vaibhav.y]

Rebase on upstream/main:

• [clangBasic] Format code
• [clangBasic] Mark all constructors taking single values explicit
• [clangBasic] Convert StringRef to std::string fields that own the data
• [clangBasic] Fixup outdated comments

clang/include/clang/Basic/Sarif.h

81–83

Good point! Will change it to std::string to start with.

Some fields such as MimeType, RuleID would probably do better with SmallString, I haven't been able to find any good measurements on how the lengths of those two are distributed, can it be made into SmallString<N> in a future PR?

117

Agree, I've marked all constructors taking a single parameter as explicit.

244–246

Ack, fixed that as well. the right rationale for uint32_t is that it is the largest non-negative type that can be safely promoted to int64_t (which is what LLVM's json encoder supports)

clang/include/clang/Basic/SourceLocation.h

441–456

Ack, these are definitely a result of a bad rebase.

clang/lib/Basic/Sarif.cpp

200

Will run it through a pass of asan & msan, is the best way to add: -fsanitize=memory -fsanitize=address to the test CMakeLists.txt & run them?

I've changed all strings to std::string, so this one should no longer be a problem but I wonder if there's any others I have missed as well.

[vaibhav.y]

Rename enum members

[vaibhav.y]

Mark completed comments as done

[vaibhav.y]

ping: This is ready for review now.

Thanks for your patience with the review as well!

[vaibhav.y]

ping: Requesting review

[aaron.ballman]

Thank you for your patience! I finally had the chance to go through this a bit more. I identified a bunch of tiny nits (the review feedback may look scary, but most should be trivial changes), but a few larger things about the design as well that are worth thinking about.

One thing that I'm still a bit worried about is validating the output document. The unit tests are a good start and the sort of thing we need for this functionality, but I'm also worried we won't find substantial design issues until we finally get SARIF results out of Clang or the static analyzer so we can see whether tools can actually *import* the SARIF we produce. I don't think we need in-tree tests for that sort of thing, but it'd definitely make me feel a lot more comfortable if we had external evidence that we're producing valid SARIF given that this is an exchange format. That said, I also don't want this to become a cumbersome patch for you to work on or for us to review, so I'm not certain what the best answer is here yet.

clang/include/clang/Basic/Sarif.h

47

Having thought about this a bit more, I think this line should be removed because it's within a header file, so all includes of this header file will be impacted and likely without knowing it. I'm very sorry for the churn, but I think this should go and the llvm:: prefixes should be brought back within the header file. Within the source file, it's fine to use the using.

81–83

Some fields such as MimeType, RuleID would probably do better with SmallString, I haven't been able to find any good measurements on how the lengths of those two are distributed, can it be made into SmallString<N> in a future PR?

Yeah, I think that can be done in a follow-up if we find a performance benefit from it.

113

Should we delete the default ctor?

163

One question this raises for me is whether we should be enforcing invariants from the SARIF spec as part of this interface or not. Currently, you can create a thread flow that has no importance or a rule without a name/id, etc. That means it's pretty easy to create SARIF that won't validate properly. One possible way to alleviate this would be for the create() methods/ctors to require these be set when creating the objects. However, I can imagine there will be times when that is awkward due to following the builder pattern with the interface. Another option would be to have some validation methods on each of the interfaces and the whole tree gets validated after construction, but this could have performance impacts.

What are your thoughts?

249

340

341

Once you use the default ctor, there's no way to associate language options with the document writer. Is it wise to expose this constructor? (If we didn't, then we could store the LangOptions as a const reference instead of making a copy in the other constructor. Given that we never mutate the options, I think that's a win.)

343

345

347

401

407

408

clang/lib/Basic/Sarif.cpp

55–57

73–75

114–121

200

Will run it through a pass of asan & msan, is the best way to add: -fsanitize=memory -fsanitize=address to the test CMakeLists.txt & run them?

Yup! The critical part will be test coverage -- code paths that aren't executed won't get issues reported with them.

220

236–237

249

(At this point, I'll stop commenting on these -- can you go through the patch and make sure that all comments have appropriate terminating punctuation?)

[vaibhav.y]

Fixup comments
Mark clang::FullSourceRange as returning const & to its locs
rebase on upstream main

[vaibhav.y]

Hi,

Apologies for the long delay! I was on a break to focus to other projects. I have some changes in mind such as:

• Creating the SarifLog object to represent the top-level document. Currently we store this as a JSON object which ends up rather mucky. Having a separate structure can release internal state in SarifDocumentWriter. That way the writer will end up only dealing with the serialization of a SarifLog.

Regarding how to validate documents, I think having a SarifLog::validate() that checks everything underneath is the way to go. Ideally I'd prefer handling on the interface level, but I'm uncertain what would be a good approach. What do you think?

I'll comb through the previous comments again to make sure I'm not missing punctuations, will signal when this is ready for review again.

Thanks!

clang/include/clang/Basic/Sarif.h

113

Agreed, there's no reason for it to be callable. Will do the same for SarifArtifactLocation.

341

That's a good observation, I will delete this constructor and expose SarifDocumentWriter(const LangOptions &LangOpts) instead.

clang/lib/Basic/Sarif.cpp

249

Ack, sincere apologies again!

[aaron.ballman]

In D109701#3376127, @vaibhav.y wrote:

Hi,

Apologies for the long delay! I was on a break to focus to other projects.

Not a problem at all!

I have some changes in mind such as:

• Creating the SarifLog object to represent the top-level document. Currently we store this as a JSON object which ends up rather mucky. Having a separate structure can release internal state in SarifDocumentWriter. That way the writer will end up only dealing with the serialization of a SarifLog.

That seems like an interesting idea (though I think it could be done in a follow-up).

Regarding how to validate documents, I think having a SarifLog::validate() that checks everything underneath is the way to go. Ideally I'd prefer handling on the interface level, but I'm uncertain what would be a good approach. What do you think?

I think that's a reasonable idea for assert builds to let us know if there are issues with the internal consistency of the data; we do something similar for IR verification in LLVM. But the kind of validating that will tell us whether this is successful is validating against another tool (the whole point to SARIF is to be an exchange format between tools, so self-testing only gets you so much information about how well the implementation works). However, there's no good way to automate that kind of testing in our test suite, so this is more of a request to try the output in tools that can consume SARIF to see if they're successful, and if they're not, see if we can come up with unit tests for those cases.

I'll comb through the previous comments again to make sure I'm not missing punctuations, will signal when this is ready for review again.

Thanks!

clang/lib/Basic/Sarif.cpp

249

No worries! These sort of nits are really easy to miss, it happens to me too. :-)

[vaibhav.y]

Update tests to check serialization as well

• SARIF text generated is validated externally against [Microsoft's online validator][1]

[1]: https://sarifweb.azurewebsites.net/Validation

[vaibhav.y]

@aaron.ballman Would it be possible that I add ::validate through a follow-up PR?

I'm currently checking the JSON output from the writer using Microsoft's online validator, and it is passing. Though it tends to complain about things outside of the spec (e.g. the spec doesn't constrain the toolComponent version to be numeric but the tool requires it to be).

[vaibhav.y]

https://discourse.llvm.org/t/rfc-improving-clang-s-diagnostics/62584

There is an ongoing RFC similar to the work here, worth noting.

[vaibhav.y]

Rebase on main branch from upstream git

[vaibhav.y]

Fix bug detected from ASAN run

Passes -fsanitize=address

[aaron.ballman]

Aside from some minor nits and the open question about validation, I think this is getting pretty close. I'm curious about the next steps with this though, given that there are no in-tree uses for it currently. You had mentioned you planned to work on an adapter so that we could eventually start to remove the existing SARIF implementation work at: https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp. Are you still intending to work on that, or has the work from @cjdb changed your plans for the next steps? (I mostly am trying to figure out whether we should land this now because there's enough momentum that it will start being used "Real Soon Now" or whether we should wait for a bit until there's a patch to make use of this and then land the whole patch stack.)

clang/include/clang/Basic/Sarif.h

83

We don't typically use top-level const like this (same applies elsewhere) unless it's on a pointee/reference.

163

Are you still intending to add a validate() interface to take care of this, or are you still thinking about how to enforce invariants during construction (or some combination of the two)?

303–306

FWIW, this use of top-level const is fine (we do use it for data member variables).

[vaibhav.y]

Thanks, will push changes to address the comments soon.

As I understood from our discussion the work @cjdb has planned would create a new DiagnosticsConsumer, it can be started in parallel but would need the changes in D109701 to complete. I have other work planned for SARIF as well, some notes here: https://gist.github.com/envp/6abc3230dcc5043416c86aefb3d24419 (@cjdb plans to address the "BRIDGE" component)

For validation, I think having a hybrid approach is best here. As much as possible we should try to be correct by construction, but we certainly need to have validation before we serialize on the data.

My plan is to iterate on the interface as follows:

1. To decompose SarifDocumentWriter, into: SarifLog & other builders, along with a SarifLogSerializer whose sole responsibility is to construct the json::Value. This should improve the number of properties that are correct by construction.
2. Try to use this newer interface with https://github.com/llvm/llvm-project/blob/main/clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp, and incorporate any new features I think might be useful

Do you think it would it better to use the changes here in libStaticAnalyzer before they land & then iterate on it? I don't have a strong preference for either approach so I'd defer to your experience.

I'm also unfamiliar with the workflow for stacking patches in phabricator, is there documentation I can refer to for this? My guess is: I make a git branch whose base is that of D109701, and then arc diff D109701..OTHER_BRANCH to create the phabricator that would use these changes?

clang/include/clang/Basic/Sarif.h

163

Definitely! I plan to add that in a follow-up patch. The goal would be to have as much as possible be correct by construction (through small builders having a limited field set), but we'll still need a validate().

[vaibhav.y]

Discard top-level const specifier where target isn't pointee/ref

[cjdb]

I think this CL (which is quite large) might be worth just getting the functionality in, and deferring the in-tree usage to a second CL. That second CL probably should just lift the existing diagnostics so that they're in the message component if the SARIF flag is enabled: nothing fancy. I also expect this to be fairly non-intrusive with respect to the compiler, but I expect that the diagnostic engine will need some teaching here.

We're intending @abrahamcd to pilot proper true integration, by getting overload resolution failure diagnostics to be SARIF-operational. That can be done in parallel to the aforementioned CL 2, or at least in a staged pipeline after CL 2 is up for review.

[vaibhav.y]

Fix formatting in Sarif.h

[aaron.ballman]

Okay, thanks both for your perspective on this. This LGTM as-is and we can handle validation and integration in subsequent patches. Thank you for this, @vaibhav.y!

[vaibhav.y]

Thanks for your patience with the review as well!

Just noticed that I need to add a link to revision in the commit messages as well: (https://www.llvm.org/docs/Phabricator.html#committing-a-change)

I will update the commit messages, but I cannot commit to trunk.

Regarding further work on validation and integration, I couldn't find any documentation on how to work with stacked changes. Could you please direct me to some documentation for that?

[aaron.ballman]

In D109701#3606042, @vaibhav.y wrote:

Thanks for your patience with the review as well!

Likewise!

Just noticed that I need to add a link to revision in the commit messages as well: (https://www.llvm.org/docs/Phabricator.html#committing-a-change)

Yup, you'd link the commit message back to this review, like: Differential Revision: https://reviews.llvm.org/D109701

I will update the commit messages, but I cannot commit to the github repo.

Ah, thank you for letting me know. I can land the changes on your behalf. What name and email address would you like me to use for patch attribution? (I probably won't land it until tomorrow at this point though.)

Regarding further work on validation and integration, I couldn't find any documentation on how to work with stacked changes. Could you please direct me to some documentation for that?

You shouldn't need to use a patch stack once we land the changes in this review (because they'll then be on the main branch). As for documentation on stacked changes, I don't think we have any community specific documentation for it, so I'd recommend using your favorite search engine. If you still don't find much on it, we can discuss it more later.

[vaibhav.y]

Reword commit messages

• Prefix [tag] specifying which components are affected
• Append link to D109701 in commit message per commit message guidelines

[vaibhav.y]

I will update the commit messages, but I cannot commit to the github repo.

Ah, thank you for letting me know. I can land the changes on your behalf. What name and email address would you like me to use for patch attribution? (I probably won't land it until tomorrow at this point though.)

Please use:

• Name: Vaibhav Yenamandra
• email address: [e-mail]

[aaron.ballman]

Unfortunately, I had to roll this back in 7a3918b540c30cc630aaae9124c67e5e4db123c2 because there's a layering violation. I've commented on it in the review.

clang/lib/Basic/Sarif.cpp

162

I didn't catch this during the review -- but this is a layering violation that caused link errors on some of the build bots. Lexer can call into Basic, but Basic cannot call into Lexer. So we'll need to find a different way to handle this.

[aaron.ballman]

Marking as needing changes so it's clear this shouldn't be re-landed yet.

[vaibhav.y]

clang/lib/Basic/Sarif.cpp

162

Would moving the code to Support, having it depend on Basic & Lex work?

[aaron.ballman]

clang/lib/Basic/Sarif.cpp

162

I don't think so -- Support is supposed to be a pretty low-level interface; it currently only relies on LLVM's Support library. I think the layering is supposed to be: Support -> Basic -> Lex.

As I see it, there are a couple of options as to where this could live. It could live in the Frontend library, as that's where all the diagnostic consumer code in Clang lives. But that library might be a bit "heavy" to pull into other tools (maybe? I don't know). It could also live in AST -- that already links in Basic and Lex. But that feels like a somewhat random place for this to live as this has very little to do with the AST itself.

Another approach, which might be better, is to require the user of this interface to pass in the token length calculation themselves in the places where it's necessary. e.g., json::Object whatever(SourceLocation Start, SourceLocation End, unsigned EndLen) and then you can remove the reliance on the lexer entirely while keeping the interface in Basic. I'm not certain how obnoxious this suggestion is, but I think it's my preferred approach for the moment (but not a strongly held position yet). WDYT of this approach?

[vaibhav.y]

Factor dependency on Lexer::MeasureTokenLength into externally provided functor

Introduces a type: TokenLengthMetric which measures the length of a token
starting at the given SLoc

[vaibhav.y]

clang/lib/Basic/Sarif.cpp

162

I think the approach to injecting the function is better here. I've tried to make the smallest change possiblew with passing in a function whose interface is almost identical to Lexer::MeasureTokenLength. The intent was to hint at this being the "canonical metric" for token lengths (with an example in the tests for the same).

I tried passing start, end locs but couldn't find a strong use case yet since end would likely always be: Lexer::getLocForEndOfToken(start, 0)

[vaibhav.y]

Discard unused includes

[aaron.ballman]

clang/include/clang/Basic/Sarif.h

297–298

I worry about the performance aspects of using a callback for this. Calls across DSO boundaries can be slower (due to having to call through a thunk), and it necessitates calling back to the user to calculate information that the user could simply pass in directly. That's on top of std::function already being pretty heavy-weight.

clang/lib/Basic/Sarif.cpp

162

I'm not convinced that the less obtrusive change is a good design in this case. But I also agree that we should not use start/end *locations* either. SourceLocation traditionally points to the *start* of a token, so it would be super easy to get the end location wrong by forgetting to pass the location for the end of the token.

My suggestion was to continue to pass the start of the starting token, the start of the ending token, and the length of the ending token. With the callback approach, you have to call through the callback to eventually call Lexer::MeasureTokenLength(); with the direct approach, you skip needing to call through a callback (which means at least one less function call on every source location operation).

[vaibhav.y]

clang/lib/Basic/Sarif.cpp

162

Ah, I think I misunderstood your initial suggestion (json::Object whatever(SourceLocation Start, SourceLocation End, unsigned EndLen)) seemed like a function call to me, when it seems the suggested change was to pass in an object? Apologies, will fix that up.

[vaibhav.y]

Comment isn't done.

[aaron.ballman]

clang/lib/Basic/Sarif.cpp

162

Sorry for the confusion! Just to make sure we're on the same page -- my suggestion was to change the function interfaces like SarficDocumentWriter::createPhysicalLocation() so that they would take an additional unsigned EndLen parameter.

However, now that I dig around a bit, it seems like CharSourceRange is what you'd want to use there -- then you can assert that what you're given is a char range and not a token range. So you won't need the unsigned EndLen parameter after all!

[vaibhav.y]

clang/lib/Basic/Sarif.cpp

162

Interesting!

Asking for my understanding: If a CharSourceRange is a valid character range, then the End SLoc points to the last character in the range (even if it is mid token)? (Unlike slocs where it the first character of the last token).

[vaibhav.y]

Use CharSourceRange instead of FullSourceRange so we no longer need to compute
the length of the last token. This should already be encoded in the end location
of the CharSourceRange by the caller

TODO(@vaibhav.y): Once accepted, drop the commit that introduces: clang::FullSourceRange

[vaibhav.y]

Discard include: clang/Lex/Lexer.h

[aaron.ballman]

Thanks, I think this is heading in the right direction!

clang/include/clang/Basic/Sarif.h

168

Should we assert this source range is not a token range?

281

In an asserts build, should we additionally have a loop to assert that each location is a char range rather than a token range?

clang/lib/Basic/Sarif.cpp

162

Your understanding is correct.

[vaibhav.y]

clang/include/clang/Basic/Sarif.h

168

I don't have a strong opinion here (since these are validated downstream in createPhysicalLocation) but it makes sense to be defensive & assert early.

I'll preserve the downstream one as well in case a new type gets added that feeds data into createPhysicalLocation as well.

281

Will do, I had the asserts in createPhysicalLocation initially. Adding them at the site of creation makes sense as well.

[vaibhav.y]

Assert that CharSourceRanges passed to Threadflow, SarifResult are
character Ranges.

[aaron.ballman]

LGTM! Thank you for the fixes to this! I'll land this again on your behalf with the same information as before.

[aaron.ballman]

I had to roll it back because of failures with test bots:

https://lab.llvm.org/buildbot/#/builders/91/builds/11328

So this was reverted in b46ad1b5be694feefabd4c6cd112cbbd04a7b3a7, can you take a look when you get the chance?

[vaibhav.y]

In D109701#3622983, @aaron.ballman wrote:

I had to roll it back because of failures with test bots:

https://lab.llvm.org/buildbot/#/builders/91/builds/11328

So this was reverted in b46ad1b5be694feefabd4c6cd112cbbd04a7b3a7, can you take a look when you get the chance?

Odd! I'll try to reproduce that at the earliest. check-clang-unit was passing on my local machine last I checked. I'll try to build this on a RHEL system to be closer to what the buildbot reports.

Apologies for having to revert again!

[vaibhav.y]

Discard most likely culprit in test code causing unexpected crash.

@aaron.ballman I was unable to reproduce the issue on my end using the buildbot instructions and ASAN, UBSAN.

Is it possible for you to trigger a pre-merge check on the LLVM cluster?

I have suspicions that it was the SarifResult && type in the test so I changed it to a const SarifResult &.

I've tried running it on a RHEL 7, Darwin on my end.

[aaron.ballman]

In D109701#3642561, @vaibhav.y wrote:

Discard most likely culprit in test code causing unexpected crash.

@aaron.ballman I was unable to reproduce the issue on my end using the buildbot instructions and ASAN, UBSAN.

Is it possible for you to trigger a pre-merge check on the LLVM cluster?

Unfortunately, there isn't.

I have suspicions that it was the SarifResult && type in the test so I changed it to a const SarifResult &.

I've tried running it on a RHEL 7, Darwin on my end.

If you think you've got it fixed, I think the best we can do is to re-commit and watch the bots to see how they react. I'll commit again for you and watch them.

[vaibhav.y]

I have suspicions that it was the SarifResult && type in the test so I changed it to a const SarifResult &.

I've tried running it on a RHEL 7, Darwin on my end.

If you think you've got it fixed, I think the best we can do is to re-commit and watch the bots to see how they react. I'll commit again for you and watch them.

Thank you! Let us try that.

[aaron.ballman]

Unfortunately, it still seems to be causing failures (I had to revert again):

https://lab.llvm.org/buildbot/#/builders/91/builds/11840

It looks to be the same failure as before (https://lab.llvm.org/buildbot/#/builders/91/builds/11328). :-(

[vaibhav.y]

In D109701#3642748, @aaron.ballman wrote:

Unfortunately, it still seems to be causing failures (I had to revert again):

https://lab.llvm.org/buildbot/#/builders/91/builds/11840

It looks to be the same failure as before (https://lab.llvm.org/buildbot/#/builders/91/builds/11328). :-(

Thanks, this seems to be reproducible. Perhaps there's something special about my environment making it work. I'll try to pare down more (possibly try to build inside a container).

The buildbot properties say its Linux aurora is there an official container / build spec I can use to mimic the environment?

[aaron.ballman]

In D109701#3642786, @vaibhav.y wrote:

In D109701#3642748, @aaron.ballman wrote:

Unfortunately, it still seems to be causing failures (I had to revert again):

https://lab.llvm.org/buildbot/#/builders/91/builds/11840

It looks to be the same failure as before (https://lab.llvm.org/buildbot/#/builders/91/builds/11328). :-(

Thanks, this seems to be reproducible. Perhaps there's something special about my environment making it work. I'll try to pare down more (possibly try to build inside a container).

Hopefully!

The buildbot properties say its Linux aurora is there an official container / build spec I can use to mimic the environment?

I don't know that there is such a thing, unfortunately. (Many of the bots are bots hosted by various different companies with different views on access to servers, I'd imagine.)

[abrahamcd]

Hi! I'm interning with @cjdb and @denik this summer and I was working on adding a -fdiagnostics-format=sarif option to start off my project, but I just found that a previous abandoned version of this change (D109697) was intending to add it. Seeing as the flag is no longer included in this version of the change, is it okay for me to go on and add it myself, or are you still planning on adding it here? Thanks!

[vaibhav.y]

In D109701#3646856, @abrahamcd wrote:

Hi! I'm interning with @cjdb and @denik this summer and I was working on adding a -fdiagnostics-format=sarif option to start off my project, but I just found that a previous abandoned version of this change (D109697) was intending to add it. Seeing as the flag is no longer included in this version of the change, is it okay for me to go on and add it myself, or are you still planning on adding it here? Thanks!

Sure, feel free to use D109697 as you see fit!

[abrahamcd]

In D109701#3648168, @vaibhav.y wrote:

In D109701#3646856, @abrahamcd wrote:

Hi! I'm interning with @cjdb and @denik this summer and I was working on adding a -fdiagnostics-format=sarif option to start off my project, but I just found that a previous abandoned version of this change (D109697) was intending to add it. Seeing as the flag is no longer included in this version of the change, is it okay for me to go on and add it myself, or are you still planning on adding it here? Thanks!

Sure, feel free to use D109697 as you see fit!

Great, thank you!

[vaibhav.y]

@aaron.ballman

The culprit turned out to be the difference in release flags on the build server vs my environment. I had unfortunately run the configuration command once in Debug mode, and hadn't re-configured. Not a bright moment :)

The ASSERT_DEATH tests that I had written weren't gated by NDEBUG, GTEST_HAS_DEATH_TEST (other unit tests use some combination two). This was causing them to pass on my machine but fail pre-merge (which is RelWithDebInfo)

I will gate the tests similar to what https://github.com/llvm/llvm-project/blob/main/clang/unittests/Serialization/InMemoryModuleCacheTest.cpp#L48-L52 does, but force a skip instead.

Running another clean build on my end to ensure I have the right culprit. Thank you for your patience!

[vaibhav.y]

Gate death tests on NDEBUG and available of GTEST_HAS_DEATH_TEST

This should fix recent post-merge failures

[vaibhav.y]

Undo test case renames

[aaron.ballman]

LGTM assuming precommit CI comes back happy with it, thank you! I'll land it once I see things are green.

clang/include/clang/Basic/Sarif.h

+//== clang/Basic/Sarif.h - SARIF Diagnostics Object Model -------*- C++ -*--==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  \file
+//  This file defines SarifDocument, a class for pretty-printing clang
+//  diagnostics in the SARIF standard. The document created can be accessed
+//  as a JSON Object. This class only implements a valid subset of SARIF, which
+//  is limited to conforming to all 'SHALL' / 'MUST' definitions for properties
+//  of interest to Clang.
+//
+//  A SARIF (Static Analysis Results Interchange Format) document is JSON
+//  document that describes in detail the results of running static analysis
+//  tools on a project. Each (non-trivial) document consists of at least one
+//  "run", which are themselves composed of details such as:
+//  * Tool: The tool that was run
+//  * Rules: The rules applied during the tool run, represented by
+//    \c reportingDescriptor objects in SARIF
+//  * Results: The matches for the rules applied against the project(s) being
+//    evaluated, represented by \c result objects in SARIF
+//
+//  Reference:
+//  1. <a href="http://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html">The SARIF standard</a>
+//  2. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317836">SARIF<pre>reportingDescriptor</pre></a>
+//  3. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317638">SARIF<pre>result</pre></a>
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CLANG_BASIC_SARIF_H
+#define CLANG_BASIC_SARIF_H
+
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include <initializer_list>
+
+namespace clang {
+
+using namespace llvm;
+
+class SarifDocumentWriter;
+class LangOptions;
+class SourceManager;
+class FullSourceRange;
+
+namespace detail {
+
+/// An artifact location is SARIF's way of describing the complete Location
+/// of an artifact encountered during analysis. The \c artifactLocation object
+/// typically consists of a URI, and/or an index to reference the artifact it
+/// locates.
+///
+/// This builder makes an additional assumption: that every artifact encountered
+/// by \c clang will be a physical, top-level artifact. Which is why the static
+/// creation method \ref SarifArtifactLocation::create takes a mandatory URI
+/// parameter. The official standard states that either a \c URI or \c Index
+/// must be available in the object, \c clang picks the \c URI as a reasonable,
+/// arbitrary default.
+///
+/// Reference:
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317427">artifactLocation object</a>
+/// 2. \ref SarifArtifact
+class SarifArtifactLocation {
+private:
+  friend class clang::SarifDocumentWriter;
+
+  llvm::Optional<uint32_t> Index;
+  StringRef URI;
+
+  SarifArtifactLocation(const StringRef &URI) : Index(), URI(URI) {}
+
+public:
+  static SarifArtifactLocation create(const StringRef &URI) {
+    return SarifArtifactLocation{URI};
+  }
+
+  SarifArtifactLocation &setIndex(uint32_t Idx) {
+    this->Index = Idx;
+    return *this;
+  }
+};
+
+/// An artifact in SARIF is any object (a sequence of bytes) addressable by
+/// a URI (RFC 3986). The most common type of artifact for clang's use-case
+/// would be source files. SARIF's artifact object is described in detail in
+/// section 3.24.
+//
+/// Since every in clang artifact MUST have a location (there being no nested
+/// artifacts), the creation method \ref SarifArtifact::create requires a
+/// \ref SarifArtifactLocation object
+///
+/// Reference:
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317611">artifact object</a>
+class SarifArtifact {
+private:
+  friend class clang::SarifDocumentWriter;
+
+  llvm::Optional<uint32_t> Offset;
+  llvm::Optional<size_t> Length;
+  StringRef MimeType;
+  SarifArtifactLocation Location;
+  SmallVector<StringRef> Roles;
+
+  SarifArtifact(const SarifArtifactLocation &Loc)
+      : Offset(), Length(), MimeType(), Location(Loc), Roles() {}
+
+public:
+  static SarifArtifact create(const SarifArtifactLocation &Loc) {
+    return SarifArtifactLocation{Loc};
+  }
+
+  SarifArtifact &setOffset(uint32_t Offset) {
+    this->Offset = Offset;
+    return *this;
+  }
+
+  SarifArtifact &setLength(size_t NumBytes) {
+    this->Length = NumBytes;
+    return *this;
+  }
+
+  SarifArtifact &setRoles(const std::initializer_list<StringRef> &Roles) {
+    this->Roles.assign(Roles);
+    return *this;
+  }
+
+  SarifArtifact &setMimeType(const StringRef &MimeType) {
+    this->MimeType = MimeType;
+    return *this;
+  }
+};
+
+} // namespace detail
+
+/// A thread flow is a sequence of code locations that specify a possible path
+/// through a single thread of execution.
+/// A thread flow in SARIF is related to a code flow which describes
+/// the progress of one or more programs through one or more thread flows.
+///
+/// Reference:
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317744">threadFlow object</a>
+/// 2. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317740">codeFlow object</a>
+class ThreadFlow {
+  friend class SarifDocumentWriter;
+
+  FullSourceRange Range;
+  StringRef Importance;
+  StringRef Message;
+
+  ThreadFlow() = default;
+
+public:
+  static ThreadFlow create() { return {}; }
+
+  ThreadFlow &setRange(const FullSourceRange &Range) {
+    this->Range = Range;
+    return *this;
+  }
+
+  ThreadFlow &setImportance(const StringRef &Importance) {
+    this->Importance = Importance;
+    return *this;
+  }
+
+  ThreadFlow &setMessage(const StringRef &Message) {
+    this->Message = Message;
+    return *this;
+  }
+};
+
+/// A SARIF rule (\c reportingDescriptor object) contains information that
+/// describes a reporting item generated by a tool. A reporting item is
+/// either a result of analysis or notification of a condition encountered by
+/// the tool. Rules are arbitrary but are identifiable by a hierarchical
+/// rule-id.
+///
+/// This builder provides an interface to create SARIF \c reportingDescriptor
+/// objects via the \ref SarifRule::create static method.
+///
+/// Reference:
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317836">reportingDescriptor object</a>
+class SarifRule {
+  friend class clang::SarifDocumentWriter;
+
+  StringRef Name;
+  StringRef RuleId;
+  StringRef Description;
+  StringRef HelpURI;
+
+  SarifRule() = default;
+
+public:
+  static SarifRule create() { return {}; }
+
+  SarifRule &setName(const StringRef &Name) {
+    this->Name = Name;
+    return *this;
+  }
+
+  SarifRule &setRuleId(const StringRef &RuleId) {
+    this->RuleId = RuleId;
+    return *this;
+  }
+
+  SarifRule &setDescription(const StringRef &Description) {
+    this->Description = Description;
+    return *this;
+  }
+
+  SarifRule &setHelpURI(const StringRef &HelpURI) {
+    this->HelpURI = HelpURI;
+    return *this;
+  }
+};
+
+/// A SARIF result (also called a "reporting item") is a unit of output
+/// produced when one of the tool's \c reportingDescriptor encounters a match
+/// on the file being analysed by the tool.
+///
+/// This builder provides a \ref SarifResult::create static method that can be
+/// used to create an empty shell onto which attributes can be added using the
+/// \c setX(...) methods.
+///
+/// For example:
+/// \code{.cpp}
+/// SarifResult result = SarifResult::create()
+///                         .setIndex(...)
+///                         .setRuleId(...)
+///                         .setDiagnosticMessage(...);
+/// \endcode
+///
+/// Reference:
+/// 1. <a href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317638">SARIF<pre>result</pre></a>
+class SarifResult {
+  friend class clang::SarifDocumentWriter;
+
+  uint32_t RuleIdx;
+  StringRef RuleID;
+  StringRef DiagnosticMessage;
+  ArrayRef<FullSourceRange> Locations;
+  ArrayRef<ThreadFlow> ThreadFlows;
+
+  SarifResult() = default;
+
+public:
+  static SarifResult create() { return {}; }
+
+  SarifResult &setIndex(uint32_t idx) {
+    this->RuleIdx = idx;
+    return *this;
+  }
+
+  SarifResult &setRuleId(const StringRef &RuleID) {
+    this->RuleID = RuleID;
+    return *this;
+  }
+
+  SarifResult &setDiagnosticMessage(const StringRef &Message) {
+    this->DiagnosticMessage = Message;
+    return *this;
+  }
+
+  SarifResult &setLocations(const ArrayRef<FullSourceRange> &DiagLocs) {
+    this->Locations = DiagLocs;
+    return *this;
+  }
+  SarifResult &setThreadFlows(const ArrayRef<ThreadFlow> &ThreadFlows) {
+    this->ThreadFlows = ThreadFlows;
+    return *this;
+  }
+};
+
+/// This class handles creating a valid SARIF document given various input
+/// attributes. However, it requires an ordering among certain method calls:
+///
+/// 1. Because every SARIF document must contain at least 1 \c run, callers
+///    must ensure that \ref SarifDocumentWriter::createRun is is called before
+///    anyother methods.
+/// 2. If SarifDocumentWriter::endRun is called, callers MUST call
+///    SarifDocumentWriter::createRun, before invoking any of the result
+///    aggregation methods such as SarifDocumentWriter::appendResult etc.
+class SarifDocumentWriter {
+private:
+  const StringRef SchemaURI{
+      "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/"
+      "sarif-schema-2.1.0.json"};
+  const StringRef SchemaVersion{"2.1.0"};
+
+  /// \internal
+  /// Return a pointer to the current tool. If no run exists, this will
+  /// crash.
+  json::Object *getCurrentTool();
+
+  /// \internal
+  /// Checks if there is a run associated with this document
+  ///
+  /// \return true on success
+  bool hasRun() const;
+
+  /// \internal
+  /// Reset portions of the internal state so that the document is ready to
+  /// recieve data for a new run
+  void reset();
+
+  /// \internal
+  /// \brief Return a mutable pointer to the current run, if it exists.
+  ///
+  /// \note If a run does not exist in the SARIF document, calling this will
+  /// trigger undefined behaviour
+  json::Object *currentRun();
+
+  /// Create a code flow object for the given threadflows.
+  /// See \link ThreadFlow \endlink
+  ///
+  /// \note If a run does not exist in the SARIF document, calling this will
+  /// trigger undefined behaviour
+  json::Object createCodeFlow(const ArrayRef<ThreadFlow> &ThreadFlows);
+
+  /// Add the given threadflows to the ones this SARIF document knows about
+  json::Array createThreadFlows(const ArrayRef<ThreadFlow> &ThreadFlows);
+
+  /// Add the given \ref FullSourceRange to the SARIF document as a physical
+  /// location, with it's corresponding artifact
+  json::Object createPhysicalLocation(const FullSourceRange &R);
+
+public:
+  /// Create a new empty SARIF document
+  SarifDocumentWriter() = default;
+
+  /// Create a new empty SARIF document with the given language options
+  SarifDocumentWriter(const LangOptions &LangOpts) : LangOpts(&LangOpts) {}
+
+  /// Release resources held by this SARIF document
+  ~SarifDocumentWriter() = default;
+
+  /// Create a new run with which any upcoming analysis will be associated.
+  /// Each run requires specifying the tool that is generating reporting items
+  void createRun(const StringRef &ShortToolName, const StringRef &LongToolName);
+
+  /// If there is a current run, end it. This method collects various
+  /// book-keeping required to clear and close resources associated
+  /// with the current run, but may also allocate some for the next run.
+  ///
+  /// If no run exists, this amounts to a no-op.
+  void endRun();
+
+  /// Create a new rule, and associate it with the current run
+  /// Returns integer rule index for the created rule that is unique within
+  /// the current run
+  ///
+  /// \pre
+  /// There must be a run associated with the document, failing to do so will
+  /// cause undefined behaviour
+  size_t createRule(const StringRef &Name, const StringRef &RuleId,
+                    const StringRef &Description,
+                    const StringRef &HelpURI = "");
+
+  /// Associate the given rule with the current run
+  ///
+  /// \pre
+  /// There must be a run associated with the document, failing to do so will
+  /// cause undefined behaviour
+  size_t createRule(const SarifRule &Rule);
+
+  /// Append a new result to the currently in-flight run.
+  ///
+  /// \pre
+  /// There must be a run associated with the document, failing to do so will
+  /// cause undefined behaviour
+  /// \pre
+  /// \c RuleIdx must correspond to a rule known by the SARIF document. i.e.
+  /// it must be the value returned by a previous call to \ref createRule
+  void appendResult(size_t RuleIdx, const SarifResult &SarifResult);
+
+  /// Return the SARIF document in its current state.
+  /// Calling this will trigger a copy of the internal state including all
+  /// reported diagnostics, resulting in an expensive call.
+  json::Object createDocument();
+
+private:
+  /// Langauge options to use for the current SARIF document
+  const LangOptions *LangOpts;
+
+  /// A sequence of SARIF runs
+  /// A run object describes a single run of an analysis tool and contains the
+  /// output of that run
+  ///
+  /// Reference: <a
+  /// href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317484">run
+  /// object</a>
+  json::Array Runs;
+
+  /// The list of rules associated with the most recent active run. These are
+  /// defined using the diagnostics passed to the SarifDocument. Each rule
+  /// need not be unique through the result set. E.g. there may be several
+  /// 'syntax' errors throughout code under analysis, each of which has its
+  /// own specific diagnostic message (and consequently, RuleId). Rules are
+  /// also known as "reportingDescriptor" objects in SARIF
+  ///
+  /// Reference: <a
+  /// href="https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317556">rules
+  /// property</a>
+  SmallVector<SarifRule, 32> CurrentRules;
+
+  /// The list of artifacts that have been encountered on the most recent active
+  /// run. An artifact is defined in SARIF as a sequence of bytes addressable
+  /// by a URI. A common example for clang's case would be files named by
+  /// filesystem paths.
+  StringMap<detail::SarifArtifact> CurrentArtifacts;
+};
+} // namespace clang
+
+#endif // CLANG_BASIC_SARIF_H

clang/include/clang/Basic/SourceLocation.h

public:
   bool isBeforeInTranslationUnitThan(FullSourceLoc Loc) const {
     assert(Loc.isValid());
     assert(SrcMgr == Loc.SrcMgr && "Loc comes from another SourceManager!");
     return isBeforeInTranslationUnitThan((SourceLocation)Loc);
   }
 
   /// Comparison function class, useful for sorting FullSourceLocs.
   struct BeforeThanCompare {
-    bool operator()(const FullSourceLoc& lhs, const FullSourceLoc& rhs) const {
+    bool operator()(const FullSourceLoc &lhs, const FullSourceLoc &rhs) const {
       return lhs.isBeforeInTranslationUnitThan(rhs);
     }
   };
 
   /// Prints information about this FullSourceLoc to stderr.
   ///
   /// This is useful for debugging.
   void dump() const;
 
-  friend bool
-  operator==(const FullSourceLoc &LHS, const FullSourceLoc &RHS) {
+  friend bool operator==(const FullSourceLoc &LHS, const FullSourceLoc &RHS) {
     return LHS.getRawEncoding() == RHS.getRawEncoding() &&
            LHS.SrcMgr == RHS.SrcMgr;
   }
 
-  friend bool
-  operator!=(const FullSourceLoc &LHS, const FullSourceLoc &RHS) {
+  friend bool operator!=(const FullSourceLoc &LHS, const FullSourceLoc &RHS) {
     return !(LHS == RHS);
   }
 };
 
+/// A pair of FullSourceLoc objects
+///
+/// Useful for passing to methods that expect SourceRanges and SourceManagers
+/// together.
+class FullSourceRange {
+  FullSourceLoc B;
+  FullSourceLoc E;
+
+public:
+  FullSourceRange() = default;
+  FullSourceRange(FullSourceLoc Begin, FullSourceLoc End) : B(Begin), E(End) {}
+
+  const FullSourceLoc &getBegin() const { return B; }
+  const FullSourceLoc &getEnd() const { return E; }
+
+  bool isValid() const { return B.isValid() && E.isValid(); }
+  bool isInvalid() const { return !isValid(); }
+
+  bool operator==(const FullSourceRange &X) const {
+    return B == X.B && E == X.E;
+  }
+
+  bool operator!=(const FullSourceRange &X) const {
+    return B != X.B || E != X.E;
+  }
+
+  // Returns true iff other is wholly contained within this range.
+  bool fullyContains(const FullSourceRange &other) const {
+    return B <= other.B && E >= other.E;
+  }
+
+  void print(raw_ostream &OS) const;
+  std::string printToString() const;
+  void dump() const;
+};
+
 } // namespace clang
 
 namespace llvm {
 
   /// Define DenseMapInfo so that FileID's can be used as keys in DenseMap and
   /// DenseSets.
   template <>
   struct DenseMapInfo<clang::FileID> {

clang/lib/Basic/CMakeLists.txt

add_clang_library(clangBasic
   ObjCRuntime.cpp
   OpenCLOptions.cpp
   OpenMPKinds.cpp
   OperatorPrecedence.cpp
   ProfileList.cpp
   NoSanitizeList.cpp
   SanitizerSpecialCaseList.cpp
   Sanitizers.cpp
+  Sarif.cpp
   SourceLocation.cpp
   SourceManager.cpp
   Stack.cpp
   TargetID.cpp
   TargetInfo.cpp
   Targets.cpp
   Targets/AArch64.cpp
   Targets/AMDGPU.cpp

clang/lib/Basic/Sarif.cpp

+#include "clang/Basic/Sarif.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/Version.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/Path.h"
+
+#include <limits>
+#include <string>
+#include <utility>
+
+using namespace clang;
+using namespace llvm;
+
+using clang::detail::SarifArtifact;
+using clang::detail::SarifArtifactLocation;
+
+namespace {
+
+StringRef getFileName(const FileEntry &FE) {
+  StringRef Filename = FE.tryGetRealPathName();
+  if (Filename.empty())
+    Filename = FE.getName();
+  return Filename;
+}
+/// \name URI
+/// @{
+
+/// \internal
+/// \brief
+/// Return the RFC3986 encoding of the input character
+///
+/// \param C Character to encode to RFC3986
+///
+/// \return The RFC3986 representation of \c C
+std::string percentEncodeURICharacter(char C) {
+  // RFC 3986 claims alpha, numeric, and this handful of
+  // characters are not reserved for the path component and
+  // should be written out directly. Otherwise, percent
+  // encode the character and write that out instead of the
+  // reserved character.
+  if (llvm::isAlnum(C) ||
+      StringRef::npos != StringRef("-._~:@!$&'()*+,;=").find(C))
+    return std::string(&C, 1);
+  return "%" + llvm::toHex(StringRef(&C, 1));
+}
+
+/// \internal
+/// \brief Return a URI representing the given file name
+///
+/// \param Filename
+///
+/// \return RFC3986 URI representing the input file name
+std::string fileNameToURI(StringRef Filename) {
+  llvm::SmallString<32> Ret = StringRef("file://");
+
+  // Get the root name to see if it has a URI authority.
+  StringRef Root = sys::path::root_name(Filename);
+  if (Root.startswith("//")) {
+    // There is an authority, so add it to the URI.
+    Ret += Root.drop_front(2).str();
+  } else if (!Root.empty()) {
+    // There is no authority, so end the component and add the root to the URI.
+    Ret += Twine("/" + Root).str();
+  }
+
+  auto Iter = sys::path::begin(Filename), End = sys::path::end(Filename);
+  assert(Iter != End && "Expected there to be a non-root path component.");
+  // Add the rest of the path components, encoding any reserved characters;
+  // we skip past the first path component, as it was handled it above.
+  std::for_each(++Iter, End, [&Ret](StringRef Component) {
+    // For reasons unknown to me, we may get a backslash with Windows native
+    // paths for the initial backslash following the drive component, which
+    // we need to ignore as a URI path part.
+    if (Component == "\\")
+      return;
+
+    // Add the separator between the previous path part and the one being
+    // currently processed.
+    Ret += "/";
+
+    // URI encode the part.
+    for (char C : Component) {
+      Ret += percentEncodeURICharacter(C);
+    }
+  });
+
+  return std::string(Ret);
+}
+///  @}
+
+/// \brief Calculate the column position expressed in the number of UTF-8 code
+/// points from column start to the source location
+///
+/// \param Loc The source location whose column needs to be calculated
+/// \param TokenLen Optional hint for when the token is multiple bytes long
+///
+/// \return The column number as a UTF-8 aware byte offset from column start to
+/// the effective source location
+unsigned int adjustColumnPos(FullSourceLoc Loc, unsigned int TokenLen = 0) {
+  assert(!Loc.isInvalid() && "invalid Loc when adjusting column position");
+
+  std::pair<FileID, unsigned> LocInfo = Loc.getDecomposedLoc();
+  assert(LocInfo.second > Loc.getExpansionColumnNumber() &&
+         "position in file is before column number?");
+
+  Optional<MemoryBufferRef> Buf =
+      Loc.getManager().getBufferOrNone(LocInfo.first);
+  assert(Buf && "got an invalid buffer for the location's file");
+  assert(Buf->getBufferSize() >= (LocInfo.second + TokenLen) &&
+         "token extends past end of buffer?");
+
+  // Adjust the offset to be the start of the line, since we'll be counting
+  // Unicode characters from there until our column offset.
+  unsigned int Off = LocInfo.second - (Loc.getExpansionColumnNumber() - 1);
+  unsigned int Ret = 1;
+  while (Off < (LocInfo.second + TokenLen)) {
+    Off += getNumBytesForUTF8(Buf->getBuffer()[Off]);
+    Ret++;
+  }
+
+  return Ret;
+}
+
+/// \name SARIF Utilities
+/// @{
+
+/// \internal
+json::Object createMessage(StringRef Text) {
+  return json::Object{{"text", Text.str()}};
+}
+
+/// \internal
+json::Object createTextRegion(const LangOptions &LO, const FullSourceRange &R) {
+  json::Object Region{{"startLine", R.getBegin().getExpansionLineNumber()},
+                      {"startColumn", adjustColumnPos(R.getBegin())}};
+  if (R.getBegin() == R.getEnd()) {
+    Region["endColumn"] = adjustColumnPos(R.getBegin());
+  } else {
+    Region["endLine"] = R.getEnd().getExpansionLineNumber();
+    Region["endColumn"] = adjustColumnPos(
+        R.getEnd(), Lexer::MeasureTokenLength(R.getEnd().getLocWithOffset(0),
+                                              R.getEnd().getManager(), LO));
+  }
+  return Region;
+}
+
+json::Object createLocation(json::Object &&PhysicalLocation,
+                            StringRef Message = "") {
+  json::Object Ret{{"physicalLocation", std::move(PhysicalLocation)}};
+  if (!Message.empty())
+    Ret.insert({"message", createMessage(Message)});
+  return Ret;
+}
+
+json::Object createThreadFlowLocation(json::Object &&Location,
+                                      const StringRef &Importance) {
+  return json::Object{{"locations", std::move(Location)},
+                      {"importance", Importance}};
+}
+///  @}
+
+} // namespace
+
+json::Object
+SarifDocumentWriter::createPhysicalLocation(const FullSourceRange &R) {
+  assert(R.isValid() &&
+         "Cannot create a physicalLocation from invalid SourceRange!");
+  const FileEntry *FE = R.getBegin().getExpansionLoc().getFileEntry();
+  assert(FE != nullptr && "Diagnostic does not exist within a valid file!");
+
+  const std::string &FileURI = fileNameToURI(getFileName(*FE));
+  auto I = CurrentArtifacts.find(FileURI);
+
+  if (I == CurrentArtifacts.end()) {
+    uint32_t Idx = static_cast<uint32_t>(CurrentArtifacts.size());
+    const SarifArtifactLocation &location =
+        SarifArtifactLocation::create(FileURI).setIndex(Idx);
+    const SarifArtifact &artifact = SarifArtifact::create(location)
+                                        .setRoles({"resultFile"})
+                                        .setLength(FE->getSize())
+                                        .setMimeType("text/plain");
+    auto statusIter = CurrentArtifacts.insert({FileURI, artifact});
+    // If inserted, ensure the original iterator points to the newly inserted
+    // element, so it can be used downstream
+    if (statusIter.second) {
+      I = statusIter.first;
+    }
+  }
+  assert(I != CurrentArtifacts.end() && "Failed to insert new artifact");
+  const SarifArtifactLocation &location = I->second.Location;
+  auto Idx = location.Index.getValue();
+  return json::Object{
+      {{"artifacts", json::Object{{{"uri", FileURI}, {"index", Idx}}}},
+       {"region", createTextRegion(*LangOpts, R)}}};
+}
+
+json::Object *SarifDocumentWriter::getCurrentTool() {
+  assert(hasRun() && "Need to call createRun() before using getcurrentTool!");
+  return Runs.back().getAsObject()->get("tool")->getAsObject();
+}
+
+void SarifDocumentWriter::reset() {
+  CurrentRules.clear();
+  CurrentArtifacts.clear();
+}
+
+void SarifDocumentWriter::endRun() {
+  if (!hasRun()) {
+    return;
+  }
+
+  // Flush all the rules
+  json::Object &Tool = *getCurrentTool();
+  json::Array Rules{};
+  for (const SarifRule &R : CurrentRules) {
+    json::Object theRule{{"name", R.Name},
+                         {"ruleId", R.RuleId},
+                         {"fullDescription", R.Description}};
+    if (!R.HelpURI.empty()) {
+      theRule["helpUri"] = R.HelpURI;
+    }
+    Rules.emplace_back(std::move(theRule));
+  }
+  Tool["rules"] = std::move(Rules);
+
+  // Flush all the artifacts
+  json::Array *Artifacts = currentRun()->getArray("artifacts");
+  for (const auto &kv : CurrentArtifacts) {
+    const SarifArtifact &A = kv.getValue();
+    json::Object Loc{{"uri", A.Location.URI}};
+    if (A.Location.Index.hasValue()) {
+      Loc["index"] = static_cast<int64_t>(A.Location.Index.getValue());
+    }
+    json::Object theArtifact;
+    theArtifact["location"] = std::move(Loc);
+    if (A.Length.hasValue()) {
+      theArtifact["length"] = static_cast<int64_t>(A.Length.getValue());
+    }
+    if (!A.Roles.empty()) {
+      theArtifact["roles"] = json::Array(A.Roles);
+    }
+    if (!A.MimeType.empty()) {
+      theArtifact["mimeType"] = A.MimeType;
+    }
+    if (A.Offset.hasValue()) {
+      theArtifact["offset"] = A.Offset;
+    }
+    Artifacts->push_back(json::Value(std::move(theArtifact)));
+  }
+
+  // Clear, reset temporaries before new run
+  reset();
+}
+
+json::Array SarifDocumentWriter::createThreadFlows(
+    const ArrayRef<ThreadFlow> &ThreadFlows) {
+  json::Object Ret{{"locations", json::Array{}}};
+  json::Array Locs{};
+  for (const auto &ThreadFlow : ThreadFlows) {
+    json::Object PLoc = createPhysicalLocation(ThreadFlow.Range);
+    json::Object Loc = createLocation(std::move(PLoc), ThreadFlow.Message);
+    Locs.emplace_back(
+        createThreadFlowLocation(std::move(Loc), ThreadFlow.Importance));
+  }
+  Ret["locations"] = std::move(Locs);
+  return json::Array{std::move(Ret)};
+}
+
+json::Object
+SarifDocumentWriter::createCodeFlow(const ArrayRef<ThreadFlow> &ThreadFlows) {
+  return json::Object{{"threadFlows", createThreadFlows(ThreadFlows)}};
+}
+
+void SarifDocumentWriter::createRun(const StringRef &ShortToolName,
+                                    const StringRef &LongToolName) {
+  // Clear resources associated with a previous run
+  endRun();
+
+  json::Object Tool{
+      {"driver",
+       json::Object{{"name", ShortToolName},
+                    {"fullName", LongToolName},
+                    {"language", "en-US"},
+                    {"version", getClangToolFullVersion(ShortToolName)}}}};
+  json::Object currentRun{{"tool", std::move(Tool)},
+                          {"results", {}},
+                          {"artifacts", {}},
+                          {"columnKind", "unicodeCodePoints"}};
+  Runs.emplace_back(std::move(currentRun));
+}
+
+bool SarifDocumentWriter::hasRun() const { return Runs.size() != 0; }
+
+json::Object *SarifDocumentWriter::currentRun() {
+  assert(hasRun() && "SARIF Document has no runs, create a run first!");
+  return Runs.back().getAsObject();
+}
+
+size_t SarifDocumentWriter::createRule(const StringRef &Name,
+                                       const StringRef &RuleId,
+                                       const StringRef &Description,
+                                       const StringRef &HelpURI) {
+  size_t Ret = CurrentRules.size();
+  SarifRule Rule = SarifRule::create()
+                       .setName(Name)
+                       .setRuleId(RuleId)
+                       .setDescription(Description)
+                       .setHelpURI(HelpURI);
+  CurrentRules.emplace_back(Rule);
+  return Ret;
+}
+
+size_t SarifDocumentWriter::createRule(const SarifRule &Rule) {
+  size_t Ret = CurrentRules.size();
+  CurrentRules.emplace_back(Rule);
+  return Ret;
+}
+
+void SarifDocumentWriter::appendResult(size_t RuleIdx,
+                                       const SarifResult &Result) {
+  assert(RuleIdx < CurrentRules.size() &&
+         "Trying to reference a rule that doesn't exist");
+  json::Object Ret{{"message", createMessage(Result.DiagnosticMessage)},
+                   {"ruleIndex", static_cast<int64_t>(RuleIdx)},
+                   {"ruleId", CurrentRules[RuleIdx].RuleId}};
+  if (Result.Locations.size() != 0) {
+    json::Array Locs{};
+    for (auto &Range : Result.Locations) {
+      Locs.emplace_back(createLocation(createPhysicalLocation(Range)));
+    }
+    Ret["locations"] = std::move(Locs);
+  }
+  if (Result.ThreadFlows.size() != 0) {
+    Ret["codeFlows"] = json::Array{createCodeFlow(Result.ThreadFlows)};
+  }
+  json::Object *Run = currentRun();
+  json::Array *Results = Run->getArray("results");
+  Results->emplace_back(std::move(Ret));
+}
+
+json::Object SarifDocumentWriter::createDocument() {
+  // Flush all temporaries to their destinations if needed
+  endRun();
+
+  json::Object doc{
+      {"$schema", SchemaURI},
+      {"version", SchemaVersion},
+  };
+  if (Runs.size() > 0) {
+    doc["runs"] = json::Array(Runs);
+  }
+  return doc;
+}

clang/lib/Basic/SourceLocation.cpp

 StringRef FullSourceLoc::getBufferData(bool *Invalid) const {
   assert(isValid());
   return SrcMgr->getBufferData(SrcMgr->getFileID(*this), Invalid);
 }
 
 std::pair<FileID, unsigned> FullSourceLoc::getDecomposedLoc() const {
   return SrcMgr->getDecomposedLoc(*this);
 }
+
+//===----------------------------------------------------------------------===//
+// FullSourceRange
+//===----------------------------------------------------------------------===//
+
+void FullSourceRange::print(raw_ostream &OS) const {
+
+  OS << '<';
+  auto PrintedLoc = PrintDifference(OS, B.getManager(), B, {});
+  if (B != E) {
+    OS << ", ";
+    PrintDifference(OS, E.getManager(), E, PrintedLoc);
+  }
+  OS << '>';
+}
+
+LLVM_DUMP_METHOD std::string FullSourceRange::printToString() const {
+  std::string S;
+  llvm::raw_string_ostream OS(S);
+  print(OS);
+  return OS.str();
+}
+
+LLVM_DUMP_METHOD void FullSourceRange::dump() const {
+  this->print(llvm::errs());
+  llvm::errs() << '\n';
+}

clang/unittests/Basic/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(BasicTests
   CharInfoTest.cpp
   DarwinSDKInfoTest.cpp
   DiagnosticTest.cpp
   FileEntryTest.cpp
   FileManagerTest.cpp
   LineOffsetMappingTest.cpp
   SanitizersTest.cpp
+  SarifTest.cpp
   SourceManagerTest.cpp
   )
 
 clang_target_link_libraries(BasicTests
   PRIVATE
   clangAST
   clangBasic
   clangLex
   )

clang/unittests/Basic/SarifTest.cpp

+//===- unittests/Basic/SarifTest.cpp - Test writing SARIF documents -------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Basic/Sarif.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/JSON.h"
+#include <algorithm>
+
+#include "gmock/gmock.h"
+#include "gtest/gtest-death-test.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm;
+
+namespace {
+
+TEST(SarifDocumentWriterTest, createEmptyDocument) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+
+  // WHEN:
+  const json::Object &emptyDocument = writer.createDocument();
+  std::vector<StringRef> keys(emptyDocument.size());
+  std::transform(emptyDocument.begin(), emptyDocument.end(), keys.begin(),
+                 [](auto item) { return item.getFirst(); });
+
+  // THEN:
+  ASSERT_THAT(keys, testing::UnorderedElementsAre("$schema", "version"));
+}
+
+// Test that a newly inserted run will associate correct tool names
+TEST(SarifDocumentWriterTest, documentWithARun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const char *shortName = "sariftest";
+  const char *longName = "sarif writer test";
+
+  // WHEN:
+  writer.createRun(shortName, longName);
+  writer.endRun();
+  const json::Object &document = writer.createDocument();
+  const json::Array *runs = document.getArray("runs");
+
+  // THEN:
+  // A run was created
+  ASSERT_THAT(runs, testing::NotNull());
+
+  // It is the only run
+  ASSERT_EQ(runs->size(), 1UL);
+
+  // The tool associated with the run was the tool
+  const json::Object *driver =
+      runs->begin()->getAsObject()->getObject("tool")->getObject("driver");
+  ASSERT_THAT(driver, testing::NotNull());
+
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+  ASSERT_TRUE(driver->getString("language").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), shortName);
+  EXPECT_EQ(driver->getString("fullName").getValue(), longName);
+  EXPECT_EQ(driver->getString("language").getValue(), "en-US");
+}
+
+// Test adding result without a run causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWillCrashIfThereIsNoRun) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  //  A SarifDocumentWriter::createRun(...) was not called prior to
+  //  SarifDocumentWriter::appendResult(...)
+  // But a rule exists
+  auto ruleIdx = writer.createRule(SarifRule::create());
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(ruleIdx, emptyResult); },
+               ".*create a run first.*");
+}
+
+// Test adding result for invalid ruleIdx causes a crash
+TEST(SarifDocumentWriterTest, addingResultsWithoutRuleWillCrash) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  SarifResult &&emptyResult = SarifResult::create();
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  // But caller forgot to create a rule for this run:
+
+  // THEN:
+  ASSERT_DEATH({ writer.appendResult(0, emptyResult); },
+               "Trying to reference a rule that doesn't exist");
+}
+
+// Test adding rule and result shows up in the final document
+TEST(SarifDocumentWriterTest, addResultWIthValidRuleIsOk) {
+  // GIVEN:
+  SarifDocumentWriter writer;
+  const SarifResult &result = SarifResult::create();
+  const SarifRule &rule =
+      SarifRule::create()
+          .setRuleId("clang.unittest")
+          .setDescription("Example rule created during unit tests")
+          .setName("clang unit test");
+
+  // WHEN:
+  writer.createRun("sarif test", "sarif test runner");
+  unsigned ruleIdx = writer.createRule(rule);
+  writer.appendResult(ruleIdx, result);
+  const json::Object &document = writer.createDocument();
+
+  // THEN:
+  // A document with a valid schema and version exists
+  ASSERT_THAT(document.get("$schema"), ::testing::NotNull());
+  ASSERT_THAT(document.get("version"), ::testing::NotNull());
+  const json::Array *runs = document.getArray("runs");
+
+  // A run exists on this document
+  ASSERT_THAT(runs, ::testing::NotNull());
+  ASSERT_EQ(runs->size(), 1UL);
+  const json::Object *theRun = runs->back().getAsObject();
+
+  // The run has slots for tools, results, rules and artifacts
+  ASSERT_THAT(theRun->get("tool"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("results"), ::testing::NotNull());
+  ASSERT_THAT(theRun->get("artifacts"), ::testing::NotNull());
+  const json::Object *driver = theRun->getObject("tool")->getObject("driver");
+  const json::Array *results = theRun->getArray("results");
+  const json::Array *artifacts = theRun->getArray("artifacts");
+
+  // The tool is as expected
+  ASSERT_TRUE(driver->getString("name").hasValue());
+  ASSERT_TRUE(driver->getString("fullName").hasValue());
+
+  EXPECT_EQ(driver->getString("name").getValue(), "sarif test");
+  EXPECT_EQ(driver->getString("fullName").getValue(), "sarif test runner");
+
+  // The results are as expected
+  EXPECT_EQ(results->size(), 1UL);
+
+  // The artifacts are as expected
+  EXPECT_TRUE(artifacts->empty());
+}
+
+} // namespace