[PATCH] D131084: Add support for specifying the severity of a SARIF Result.

Christopher Di Bella via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 3 13:27:17 PDT 2022


cjdb added a comment.

It seems I got confused and conflated the two this morning.

In D131084#3697525 <https://reviews.llvm.org/D131084#3697525>, @vaibhav.y wrote:

>> Hmm, we can probably use "informational" for notes, warnings, and remarks, but I'm kinda partial to proposing the latter two upstream.
>
> Interesting,  I agree that remarks could fit under "informational".
>
> As for notes: As I understand it they are always child diagnostics of warnings/errors, so seems it would be okay to associate these with the "fail" kind.

I don't think classifying warnings as "fail" is right unless `-Werror` is being used. For notes, I think there may be two options, if we don't want to use informational:

- inherit from the parent diagnostic
- use notApplicable, since they don't have any influence on their own



In D131084#3697308 <https://reviews.llvm.org/D131084#3697308>, @denik wrote:

> In D131084#3697256 <https://reviews.llvm.org/D131084#3697256>, @cjdb wrote:
>
>> In D131084#3697211 <https://reviews.llvm.org/D131084#3697211>, @vaibhav.y wrote:
>>
>>> Submitting for review:
>>>
>>> Some notes:
>>>
>>> There are a couple of ways I think we can acheive this, per the spec:
>>>
>>> 1. The reportingDescriptor (rule) objects can be given a default configuration property <https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317850>, which can set the default warning level and other data such as rule parameters etc.
>>> 2. The reportingDescriptor objects can omit the default configuration (which then allows operating with warning as default), and the level is then set when the result is reported.
>>>
>>> The first approach would be "more correct", what are your thoughts on this? Would we benefit from having per-diagnostic configuration?
>>>
>>> There is also the question about the "kind" of results in clang. From my reading of the spec <https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317647>, it seems that "fail" is the only case that applies to us because:
>>>
>>> - "pass": Implies no issue was found.
>>> - "open": This value is used by proof-based tools. It could also mean additional assertions required
>>> - "informational": The specified rule was evaluated and produced a purely informational result that does not indicate the presence of a problem
>>> - "notApplicable": The rule specified by ruleId was not evaluated, because it does not apply to the analysis target.
>>>
>>> Of these "open" and "notApplicable" seem to be ones that *could* come to use but I'm not sure where / what kind of diagnostics would use these. Probably clang-tidy's `bugprone-*` suite?
>>>
>>> Let me know what you think is a good way to approach this wrt clang's diagnostics system.
>>
>> Hmm, we can probably use "informational" for notes, warnings, and remarks, but I'm kinda partial to proposing the latter two upstream.
>
> I think we can skip `kind` in the clang diagnostic. I found this:
>
>> If kind is absent, it SHALL default to "fail".
>> If level has any value other than "none" and kind is present, then kind SHALL have the value "fail".

If skipping the kind defaults to fail, then we shouldn't skip it, because getting a warning or remark doesn't necessarily mean that the build has failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131084



More information about the cfe-commits mailing list