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

[jkorous]

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!

[vsavchenko]

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]

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.

[vsavchenko]

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]

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]

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.

[Szelethus]

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?

[martong]

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?

[aaron.ballman]

In D89638#2341855, @martong wrote:

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.

[vsavchenko]

In D89638#2341855, @martong wrote:

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.

In D89638#2341859, @aaron.ballman wrote:

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.

[martong]

In D89638#2341923, @vsavchenko wrote:

In D89638#2341855, @martong wrote:

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

clang/include/clang/Basic/Attr.td

 
 def Builtin : InheritableAttr {
   let Spellings = [];
   let Args = [UnsignedArgument<"ID">];
   let Subjects = SubjectList<[Function]>;
   let SemaHandler = 0;
   let Documentation = [Undocumented];
 }
+
+def NoRefCntblBaseVirtualDtor : InheritableAttr {
+  let Spellings = [Clang<"NoRefCntblBaseVirtualDtor">];
+  let Subjects = SubjectList<[CXXRecord]>;
+  let Documentation = [Undocumented];
+}
+
+def NoUncountedLambdaCaptures : InheritableAttr {
+  let Spellings = [Clang<"NoUncountedLambdaCaptures">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [Undocumented];
+}
+
+def NoNoUncountedMember : InheritableAttr {
+  let Spellings = [Clang<"NoNoUncountedMember">];
+  let Subjects = SubjectList<[Field]>;
+  let Documentation = [Undocumented];
+}
+
+def NoUncountedLocalVars : InheritableAttr {
+  let Spellings = [Clang<"NoUncountedLocalVars">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [Undocumented];
+}
+
+def NoUncountedCallArgs : InheritableAttr {
+  let Spellings = [Clang<"NoUncountedCallArgs">];
+  let Subjects = SubjectList<[ParmVar]>;
+  let Documentation = [Undocumented];
+}

clang/lib/Sema/SemaDeclAttr.cpp

static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
 
   case ParsedAttr::AT_ReleaseHandle:
     handleHandleAttr<ReleaseHandleAttr>(S, D, AL);
     break;
 
   case ParsedAttr::AT_UseHandle:
     handleHandleAttr<UseHandleAttr>(S, D, AL);
     break;
+
+  case ParsedAttr::AT_NoRefCntblBaseVirtualDtor:
+    handleSimpleAttribute<NoRefCntblBaseVirtualDtorAttr>(S, D, AL);
+    break;
+
+  case ParsedAttr::AT_NoUncountedLambdaCaptures:
+    handleSimpleAttribute<NoUncountedLambdaCapturesAttr>(S, D, AL);
+    break;
+
+  case ParsedAttr::AT_NoNoUncountedMember:
+    handleSimpleAttribute<NoNoUncountedMemberAttr>(S, D, AL);
+    break;
+
+  case ParsedAttr::AT_NoUncountedLocalVars:
+    handleSimpleAttribute<NoUncountedLocalVarsAttr>(S, D, AL);
+    break;
+
+  case ParsedAttr::AT_NoUncountedCallArgs:
+    handleSimpleAttribute<NoUncountedCallArgsAttr>(S, D, AL);
+    break;
   }
 }
 
 /// ProcessDeclAttributeList - Apply all the decl attributes in the specified
 /// attribute list to the specified decl, ignoring any type attributes.
 void Sema::ProcessDeclAttributeList(Scope *S, Decl *D,
                                     const ParsedAttributesView &AttrList,
                                     bool IncludeCXX11Attributes) {

clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp

void visitRecordDecl(const RecordDecl *RD) const {
     if (shouldSkipDecl(RD))
       return;
 
     for (auto Member : RD->fields()) {
       const Type *MemberType = Member->getType().getTypePtrOrNull();
       if (!MemberType)
         continue;
 
+      if (Member->hasAttr<NoNoUncountedMemberAttr>())
+        continue;
+
       if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
         // If we don't see the definition we just don't know.
         if (MemberCXXRD->hasDefinition()) {
           llvm::Optional<bool> isRCAble = isRefCountable(MemberCXXRD);
           if (isRCAble && *isRCAble)
             reportBug(Member, MemberType, MemberCXXRD, RD);
         }
       }

clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp

bool shouldSkipDecl(const CXXRecordDecl *RD) const {
     if (Kind != TTK_Struct && Kind != TTK_Class)
       return true;
 
     // Ignore CXXRecords that come from system headers.
     if (BR->getSourceManager().getFileCharacteristic(RDLocation) !=
         SrcMgr::C_User)
       return true;
 
+    if (RD->hasAttr<NoRefCntblBaseVirtualDtorAttr>())
+      return true;
+
     return false;
   }
 
   void reportBug(const CXXRecordDecl *DerivedClass,
                  const CXXBaseSpecifier *BaseSpec,
                  const CXXRecordDecl *ProblematicBaseClass) const {
     assert(DerivedClass);
     assert(BaseSpec);

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp

if (auto *F = CE->getDirectCallee()) {
           isa<CXXOperatorCallExpr>(CE) && dyn_cast_or_null<CXXMethodDecl>(F);
 
       for (auto P = F->param_begin();
            // FIXME: Also check variadic function parameters.
            // FIXME: Also check default function arguments. Probably a different
            // checker. In case there are default arguments the call can have
            // fewer arguments than the callee has parameters.
            P < F->param_end() && ArgIdx < CE->getNumArgs(); ++P, ++ArgIdx) {
         // TODO: attributes.
         // if ((*P)->hasAttr<SafeRefCntblRawPtrAttr>())
         //  continue;
 
+        if ((*P)->hasAttr<NoUncountedCallArgsAttr>())
+          continue;
+
         const auto *ArgType = (*P)->getType().getTypePtrOrNull();
         if (!ArgType)
           continue; // FIXME? Should we bail?
 
         // FIXME: more complex types (arrays, references to raw pointers, etc)
         Optional<bool> IsUncounted = isUncountedPtr(ArgType);
         if (!IsUncounted || !(*IsUncounted))
           continue;

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp

void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR,
     LocalVisitor visitor(this);
     visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
   }
 
   void visitLambdaExpr(LambdaExpr *L) const {
     for (const LambdaCapture &C : L->captures()) {
       if (C.capturesVariable()) {
         VarDecl *CapturedVar = C.getCapturedVar();
+        if (!CapturedVar)
+          continue;
+        if (CapturedVar->hasAttr<NoUncountedLambdaCapturesAttr>())
+          continue;
+
         if (auto *CapturedVarType = CapturedVar->getType().getTypePtrOrNull()) {
           Optional<bool> IsUncountedPtr = isUncountedPtr(CapturedVarType);
           if (IsUncountedPtr && *IsUncountedPtr) {
             reportBug(C, CapturedVar, CapturedVarType);
           }
         }
       }
     }

clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

public:
   bool shouldSkipVarDecl(const VarDecl *V) const {
     assert(V);
     if (!V->isLocalVarDecl())
       return true;
 
     if (isDeclaredInForOrIf(V))
       return true;
 
+    if (V->hasAttr<NoUncountedLocalVarsAttr>())
+      return true;
+
     return false;
   }
 
   void reportBug(const VarDecl *V) const {
     assert(V);
     SmallString<100> Buf;
     llvm::raw_svector_ostream Os(Buf);
 

clang/test/Analysis/Checkers/WebKit/call-args.cpp

void foo() {
     f + global;
     // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
     f - global;
     // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
     f(global);
     // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}}
   }
 }
+
+namespace suppression {
+  void consume_refcntbl_unsafe([[clang::NoUncountedCallArgs]] RefCountable*) {}
+  void foo() {
+    consume_refcntbl_unsafe(provide());
+  }
+}

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

 // RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
 
 struct RefCntblBase {
   void ref() {}
   void deref() {}
 };
 
 struct Derived : RefCntblBase { };
 // expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'Derived' but doesn't have virtual destructor}}
 
 struct DerivedWithVirtualDtor : RefCntblBase {
 // expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedWithVirtualDtor' but doesn't have virtual destructor}}
   virtual ~DerivedWithVirtualDtor() {}
 };
 
-
+struct [[clang::NoRefCntblBaseVirtualDtor]] Derived2 : RefCntblBase { };
 
 template<class T>
 struct DerivedClassTmpl : T { };
 typedef DerivedClassTmpl<RefCntblBase> Foo;
 
 
 
 struct RandomBase {};

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

// This code is not expected to trigger any warnings.
     RefCountable automatic;
     RefCountable &ref_countable_ref = automatic;
   }
 
   auto foo3 = [&]() {};
   auto foo4 = [=]() {};
   RefCountable *ref_countable = nullptr;
 }
+
+void suppressed() {
+  RefCountable patatino;
+  RefCountable& ref_countable_ref [[clang::NoUncountedLambdaCaptures]] = patatino;
+
+  auto foo1 = [ref_countable_ref](){};
+}
 No newline at end of file

clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp

 void foo() {
   // no warnings
   if (RefCountable *a = provide_ref_ctnbl()) { }
   for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) { }
   RefCountable *array[1];
   for (RefCountable *a : array) { }
 }
 } // namespace ignore_for_if
+
+namespace suppression {
+void foo_ref() {
+  RefCountable automatic;
+  RefCountable &bar [[clang::NoUncountedLocalVars]] = automatic;
+}
+}

clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp

namespace ignore_unions {
 
   template<class T>
   union RefPtr {
     T* a;
   };
 
   void forceTmplToInstantiate(RefPtr<RefCountable>) {}
 }
+
+namespace suppression {
+  struct Foo {
+    RefCountable* a [[clang::NoNoUncountedMember]] = nullptr;
+  };
+}