Page MenuHomePhabricator

[Analyzer][WebKit] Add attributes to suppress specific checkers
Needs RevisionPublic

Authored by jkorous on Sat, Oct 17, 8:35 PM.

Details

Summary

One thing I am not quite sure is naming. I started with the convention of "No" prefix but it gets weird in some cases - any ideas are welcome!

Diff Detail

Event Timeline

jkorous created this revision.Sat, Oct 17, 8:35 PM
jkorous requested review of this revision.Sat, Oct 17, 8:35 PM

Great job, I think it's awesome to have more control over the analyzer with the language itself.

The naming does seem pretty tough, I agree. In the majority of cases, it seems like a mouthful ☹️ . It can also negatively impact discoverability of these attributes.

I noticed that they (for the most part) do not overlap in terms of their AST nodes. And it got me thinking, is it possible to rethink current solution with just one attribute? I think it can be easier for the end user.

clang/include/clang/Basic/Attr.td
3579

This got me thinking here, what about IndirectFieldDecl? Not only in terms of the attribute, of course, but for the checker as well.

clang/lib/Sema/SemaDeclAttr.cpp
7975

This looks confusing to me. I do get that uniformity is good, but it's good for the purposes of clarity and clarity here is arguable. Maybe we can change the name here?
It's not critical or smith, but I want to hear what other folks think about this matter.

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
80–82

Should it go then?

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
61–62

It looks like a new case! Do we have a test for that?

clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor.cpp
16

Can you please put a // no warning comment, so everyone gets that this is expected.
The same goes to other tests as well.

clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
52

Whoops 😅

aaron.ballman requested changes to this revision.Mon, Oct 19, 8:18 AM
aaron.ballman added inline comments.
clang/include/clang/Basic/Attr.td
3565

I'm worried that this will lead to an explosion of one-off attributes. Rather than introduce new attributes to suppress diagnostics in a one-off manner, I think a better approach is to extend the existing attribute related to diagnostic suppression. See the Suppress attribute, which is currently only exposed with the name gsl::suppress. I think this attribute could be given an additional spelling Clang<"suppress"> and be used to cover this same functionality but in a more extensible way.

This revision now requires changes to proceed.Mon, Oct 19, 8:18 AM
vsavchenko added inline comments.Mon, Oct 19, 8:38 AM
clang/include/clang/Basic/Attr.td
3565

This is exactly what we plan to do! And it's awesome to hear that you believe it's the right approach!
There is one problem with that approach though, we want to have an option for the use to suppress one particular diagnostic. However, there is no one good looking identifier type for checkers that the end user can see and use. Straight up checker names are too verbose and can change, diagnostic messages can be pretty long and also susceptible to changes. I guess we need to open some sort of discussion here.

I see a few options here:
The first option is to implement suppress that suppresses whatever warning is there, without warning identifiers for now. This is a bigger effort though and has larger scope than what Jan is doing here.
The second option is to support suppress for uncounted checkers. So, we start small and don't ask Jan to do everything in this patch. The problem here is with identifier (again). We probably need to choose a good identifier/set of identifiers for this group of checkers that will persist even when we proceed with other checkers.
And, finally, the third option is to introduce a separate attribute that suppresses all the checks from that group. In this case, it should be probably decoupled from the checker, meaning that it should tell something to the person reading this code. Sorta like volatile refcountable.

What do you think?

aaron.ballman added inline comments.Mon, Oct 19, 9:47 AM
clang/include/clang/Basic/Attr.td
3565

This is exactly what we plan to do! And it's awesome to hear that you believe it's the right approach!

Excellent!

There is one problem with that approach though, we want to have an option for the use to suppress one particular diagnostic. However, there is no one good looking identifier type for checkers that the end user can see and use. Straight up checker names are too verbose and can change, diagnostic messages can be pretty long and also susceptible to changes. I guess we need to open some sort of discussion here.

I agree, this should be done in a broader community discussion. The typical way this is handled by other tools is to come up with a stable identifier for each rule so that users can use the stable identifier for suppression even if the rule title or other aspects change.

The first option is to implement suppress that suppresses whatever warning is there, without warning identifiers for now. This is a bigger effort though and has larger scope than what Jan is doing here.

Agreed -- also, IIRC, the static analyzer already supports // NOLINT comments, so I don't think this gets us a whole lot that we don't already have today.

The second option is to support suppress for uncounted checkers. So, we start small and don't ask Jan to do everything in this patch. The problem here is with identifier (again). We probably need to choose a good identifier/set of identifiers for this group of checkers that will persist even when we proceed with other checkers.
And, finally, the third option is to introduce a separate attribute that suppresses all the checks from that group. In this case, it should be probably decoupled from the checker, meaning that it should tell something to the person reading this code. Sorta like volatile refcountable.

There may be a fourth option here -- come up with an RFC and start a discussion about how to suppress analyzer diagnostics by name. There are a few different approaches to consider there -- do you want to suppress just one aspect of a given check or do you want to suppress the check as a whole? Do you want to be able to suppress multiple different checks using a single attribute or should that require multiple attributes? Should there be a warning flag specific to this attribute so that users can opt in to warning when the suppression attribute is being used? Should there be a warning flag the user can opt in to/out of warning when the suppress attribute is used but there is no diagnostic to suppress?

vsavchenko added inline comments.Mon, Oct 19, 10:19 AM
clang/include/clang/Basic/Attr.td
3565

Agreed -- also, IIRC, the static analyzer already supports // NOLINT comments, so I don't think this gets us a whole lot that we don't already have today.

AFAIK only clang-tidy supports those, but maybe I missed it somehow.

There may be a fourth option here -- come up with an RFC and start a discussion about how to suppress analyzer diagnostics by name. There are a few different approaches to consider there -- do you want to suppress just one aspect of a given check or do you want to suppress the check as a whole? Do you want to be able to suppress multiple different checks using a single attribute or should that require multiple attributes? Should there be a warning flag specific to this attribute so that users can opt in to warning when the suppression attribute is being used? Should there be a warning flag the user can opt in to/out of warning when the suppress attribute is used but there is no diagnostic to suppress?

This is actually the reason why I suggest options 2 and 3. It's a lot of questions to answer! We also need to come up with a stable identifier. And I suggest to do something that is maybe not an awesome and all around generalized solution, but that is not a hacky solution "for the time being" either. We can start moving in the right direction here and also initiate the conversation at the same time.

I'll just add a generic comment because I don't know much (==anything) about webkit checkers. Do you expect to stumble across false positives regularly enough so that an attribute like this would be necessary? Do these checkers rely on some heuristic?

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

This is along the lines of what I was envisioning, except I'd name the attribute better rather than re-use annotate for this. e.g.,

[[clang::suppress("analyzer.somecheck.somefunctionality")]]
[[clang::suppress("compiler.warning.12345")]]
[[clang::suppress("tidy.check-name.whatever")]]
etc.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Honestly, I think that "CSA:supress:RefCntblBaseVirtualDtor" is way too verbose. It is incredibly easy to make mistakes, it seems hard to use and it is hell a lot to put in the actual code. It also doesn't resolve the issue with what unique identifier we should use. Imagine we decide that we should use Bug type as that unique identifier. It means that "CSA:supress:RefCntblBaseVirtualDtor" should get changed, but it is already in the user's code! It is quite important to settle on a solution that will still be available when we land a more general solution.

This is along the lines of what I was envisioning, except I'd name the attribute better rather than re-use annotate for this. e.g.,

[[clang::suppress("analyzer.somecheck.somefunctionality")]]
[[clang::suppress("compiler.warning.12345")]]
[[clang::suppress("tidy.check-name.whatever")]]
etc.

Yes, I agree, this syntax is superior IMO. It is cleaner and more clear in its intentions.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Honestly, I think that "CSA:supress:RefCntblBaseVirtualDtor" is way too verbose. It is incredibly easy to make mistakes, it seems hard to use and it is hell a lot to put in the actual code. It also doesn't resolve the issue with what unique identifier we should use. Imagine we decide that we should use Bug type as that unique identifier. It means that "CSA:supress:RefCntblBaseVirtualDtor" should get changed, but it is already in the user's code! It is quite important to settle on a solution that will still be available when we land a more general solution.

Yes, absolutely, I agree.
I just launched an RFC on cfe-dev about this. And about that this could be a possible approach for providing summaries and taint analysis notations as well. Please take a look: http://lists.llvm.org/pipermail/cfe-dev/2020-October/067074.html