Thread Safety Analysis: warnings for attributes without arguments

Summary:
When thread safety annotations are used without capability arguments,
they are assumed to apply to `this` instead. So we warn when either
`this` doesn't exist, or the class is not a capability type.

This is based on earlier work by Josh Gao that was committed in r310403,
but reverted in r310698 because it didn't properly work in template
classes. See also D36237.

The solution is not to go via the QualType of `this`, which is then a
template type, hence the attributes are not known because it could be
specialized. Instead we look directly at the class in which we are
contained.

Additionally I grouped two of the warnings together. There are two
issues here: the existence of `this`, which requires us to be a
non-static member function, and the appropriate annotation on the class
we are contained in. So we don't distinguish between not being in a
class and being static, because in both cases we don't have `this`.

Fixes PR38399.

Reviewers: aaron.ballman, delesley, jmgao, rtrieu

Reviewed By: delesley

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D51901

llvm-svn: 342605

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3008,6 +3008,14 @@ def warn_invalid_capability_name : Warning<
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
   InGroup<ThreadSafetyAttributes>, DefaultIgnore;
+def warn_thread_attribute_not_on_non_static_member : Warning<
+  "%0 attribute without capability arguments can only be applied to non-static "
+  "methods of a class">,
+  InGroup<ThreadSafetyAttributes>, DefaultIgnore;
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments refers to 'this', but %1 isn't "
+  "annotated with 'capability' or 'scoped_lockable' attribute">,
+  InGroup<ThreadSafetyAttributes>, DefaultIgnore;
 def warn_thread_attribute_argument_not_lockable : Warning<
   "%0 attribute requires arguments whose type is annotated "
   "with 'capability' attribute; type here is %1">,

clang/lib/Sema/SemaDeclAttr.cpp

@@ -476,6 +476,29 @@ static const RecordType *getRecordType(QualType QT) {
   return nullptr;
 }
 
+template <typename AttrType>
+static bool checkRecordDeclForAttr(const RecordDecl *RD) {
+  // Check if the record itself has the attribute.
+  if (RD->hasAttr<AttrType>())
+    return true;
+
+  // Else check if any base classes have the attribute.
+  if (const auto *CRD = dyn_cast<CXXRecordDecl>(RD)) {
+    CXXBasePaths BPaths(false, false);
+    if (CRD->lookupInBases(
+            [](const CXXBaseSpecifier *BS, CXXBasePath &) {
+              const auto &Ty = *BS->getType();
+              // If it's type-dependent, we assume it could have the attribute.
+              if (Ty.isDependentType())
+                return true;
+              return Ty.getAs<RecordType>()->getDecl()->hasAttr<AttrType>();
+            },
+            BPaths, true))
+      return true;
+  }
+  return false;
+}
+
 static bool checkRecordTypeForCapability(Sema &S, QualType Ty) {
   const RecordType *RT = getRecordType(Ty);
 
@@ -491,21 +514,7 @@ static bool checkRecordTypeForCapability(Sema &S, QualType Ty) {
   if (threadSafetyCheckIsSmartPointer(S, RT))
     return true;
 
-  // Check if the record itself has a capability.
-  RecordDecl *RD = RT->getDecl();
-  if (RD->hasAttr<CapabilityAttr>())
-    return true;
-
-  // Else check if any base classes have a capability.
-  if (const auto *CRD = dyn_cast<CXXRecordDecl>(RD)) {
-    CXXBasePaths BPaths(false, false);
-    if (CRD->lookupInBases([](const CXXBaseSpecifier *BS, CXXBasePath &) {
-          const auto *Type = BS->getType()->getAs<RecordType>();
-          return Type->getDecl()->hasAttr<CapabilityAttr>();
-        }, BPaths))
-      return true;
-  }
-  return false;
+  return checkRecordDeclForAttr<CapabilityAttr>(RT->getDecl());
 }
 
 static bool checkTypedefTypeForCapability(QualType Ty) {
@@ -563,8 +572,27 @@ static bool isCapabilityExpr(Sema &S, const Expr *Ex) {
 static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
                                            const ParsedAttr &AL,
                                            SmallVectorImpl<Expr *> &Args,
-                                           int Sidx = 0,
+                                           unsigned Sidx = 0,
                                            bool ParamIdxOk = false) {
+  if (Sidx == AL.getNumArgs()) {
+    // If we don't have any capability arguments, the attribute implicitly
+    // refers to 'this'. So we need to make sure that 'this' exists, i.e. we're
+    // a non-static method, and that the class is a (scoped) capability.
+    const auto *MD = dyn_cast<const CXXMethodDecl>(D);
+    if (MD && !MD->isStatic()) {
+      const CXXRecordDecl *RD = MD->getParent();
+      // FIXME -- need to check this again on template instantiation
+      if (!checkRecordDeclForAttr<CapabilityAttr>(RD) &&
+          !checkRecordDeclForAttr<ScopedLockableAttr>(RD))
+        S.Diag(AL.getLoc(),
+               diag::warn_thread_attribute_not_on_capability_member)
+            << AL << MD->getParent();
+    } else {
+      S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)
+          << AL;
+    }
+  }
+
   for (unsigned Idx = Sidx; Idx < AL.getNumArgs(); ++Idx) {
     Expr *ArgExp = AL.getArgAsExpr(Idx);
 

clang/test/Sema/attr-capabilities.c

@@ -37,15 +37,25 @@ void Func6(void) __attribute__((requires_shared_capability(BadCapability))) {}
 void Func7(void) __attribute__((assert_capability(GUI))) {}
 void Func8(void) __attribute__((assert_shared_capability(GUI))) {}
 
+void Func9(void) __attribute__((assert_capability())) {} // expected-warning {{'assert_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+void Func10(void) __attribute__((assert_shared_capability())) {} // expected-warning {{'assert_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+
 void Func11(void) __attribute__((acquire_capability(GUI))) {}
 void Func12(void) __attribute__((acquire_shared_capability(GUI))) {}
 
+void Func13(void) __attribute__((acquire_capability())) {} // expected-warning {{'acquire_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+void Func14(void) __attribute__((acquire_shared_capability())) {} // expected-warning {{'acquire_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+
 void Func15(void) __attribute__((release_capability(GUI))) {}
 void Func16(void) __attribute__((release_shared_capability(GUI))) {}
 void Func17(void) __attribute__((release_generic_capability(GUI))) {}
 
-void Func21(void) __attribute__((try_acquire_capability(1))) {}
-void Func22(void) __attribute__((try_acquire_shared_capability(1))) {}
+void Func18(void) __attribute__((release_capability())) {} // expected-warning {{'release_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+void Func19(void) __attribute__((release_shared_capability())) {} // expected-warning {{'release_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+void Func20(void) __attribute__((release_generic_capability())) {} // expected-warning {{'release_generic_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+
+void Func21(void) __attribute__((try_acquire_capability(1))) {} // expected-warning {{'try_acquire_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
+void Func22(void) __attribute__((try_acquire_shared_capability(1))) {} // expected-warning {{'try_acquire_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void Func23(void) __attribute__((try_acquire_capability(1, GUI))) {}
 void Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {}

clang/test/SemaCXX/warn-thread-safety-parsing.cpp

@@ -572,11 +572,11 @@ UnlockableMu ab_var_arg_bad_5 ACQUIRED_BEFORE(mu_ab); // \
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void elf_function() EXCLUSIVE_LOCK_FUNCTION();
+void elf_function() EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void elf_function_args() EXCLUSIVE_LOCK_FUNCTION(mu1, mu2);
 
-int elf_testfn(int y) EXCLUSIVE_LOCK_FUNCTION();
+int elf_testfn(int y) EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int elf_testfn(int y) {
   int x EXCLUSIVE_LOCK_FUNCTION() = y; // \
@@ -591,7 +591,8 @@ class ElfFoo {
  private:
   int test_field EXCLUSIVE_LOCK_FUNCTION(); // \
     // expected-warning {{'exclusive_lock_function' attribute only applies to functions}}
-  void test_method() EXCLUSIVE_LOCK_FUNCTION();
+  void test_method() EXCLUSIVE_LOCK_FUNCTION(); // \
+    // expected-warning {{'exclusive_lock_function' attribute without capability arguments refers to 'this', but 'ElfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -644,11 +645,11 @@ int elf_function_bad_7() EXCLUSIVE_LOCK_FUNCTION(0); // \
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void slf_function() SHARED_LOCK_FUNCTION();
+void slf_function() SHARED_LOCK_FUNCTION(); // expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 void slf_function_args() SHARED_LOCK_FUNCTION(mu1, mu2);
 
-int slf_testfn(int y) SHARED_LOCK_FUNCTION();
+int slf_testfn(int y) SHARED_LOCK_FUNCTION(); // expected-warning {{'shared_lock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int slf_testfn(int y) {
   int x SHARED_LOCK_FUNCTION() = y; // \
@@ -666,7 +667,8 @@ class SlfFoo {
  private:
   int test_field SHARED_LOCK_FUNCTION(); // \
     // expected-warning {{'shared_lock_function' attribute only applies to functions}}
-  void test_method() SHARED_LOCK_FUNCTION();
+  void test_method() SHARED_LOCK_FUNCTION(); // \
+    // expected-warning {{'shared_lock_function' attribute without capability arguments refers to 'this', but 'SlfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -717,14 +719,16 @@ int slf_function_bad_7() SHARED_LOCK_FUNCTION(0); // \
 // takes a mandatory boolean or integer argument specifying the retval
 // plus an optional list of locks (vars/fields)
 
-void etf_function() __attribute__((exclusive_trylock_function));  // \
+void etf_function() __attribute__((exclusive_trylock_function)); // \
   // expected-error {{'exclusive_trylock_function' attribute takes at least 1 argument}}
 
 void etf_function_args() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2);
 
-void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1);
+void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
-int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1);
+int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int etf_testfn(int y) {
   int x EXCLUSIVE_TRYLOCK_FUNCTION(1) = y; // \
@@ -739,7 +743,8 @@ class EtfFoo {
  private:
   int test_field EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
     // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
-  void test_method() EXCLUSIVE_TRYLOCK_FUNCTION(1);
+  void test_method() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+    // expected-warning {{'exclusive_trylock_function' attribute without capability arguments refers to 'this', but 'EtfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
@@ -760,7 +765,8 @@ int etf_function_5() EXCLUSIVE_TRYLOCK_FUNCTION(1, &mu1);
 int etf_function_6() EXCLUSIVE_TRYLOCK_FUNCTION(1, muRef);
 int etf_function_7() EXCLUSIVE_TRYLOCK_FUNCTION(1, muDoubleWrapper.getWrapper()->getMu());
 int etf_functetfn_8() EXCLUSIVE_TRYLOCK_FUNCTION(1, muPointer);
-int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true);
+int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true); // \
+  // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 
 // illegal attribute arguments
@@ -795,9 +801,11 @@ void stf_function() __attribute__((shared_trylock_function));  // \
 
 void stf_function_args() SHARED_TRYLOCK_FUNCTION(1, mu2);
 
-void stf_function_arg() SHARED_TRYLOCK_FUNCTION(1);
+void stf_function_arg() SHARED_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'shared_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
-int stf_testfn(int y) SHARED_TRYLOCK_FUNCTION(1);
+int stf_testfn(int y) SHARED_TRYLOCK_FUNCTION(1); // \
+  // expected-warning {{'shared_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int stf_testfn(int y) {
   int x SHARED_TRYLOCK_FUNCTION(1) = y; // \
@@ -816,7 +824,8 @@ class StfFoo {
  private:
   int test_field SHARED_TRYLOCK_FUNCTION(1); // \
     // expected-warning {{'shared_trylock_function' attribute only applies to functions}}
-  void test_method() SHARED_TRYLOCK_FUNCTION(1);
+  void test_method() SHARED_TRYLOCK_FUNCTION(1); // \
+    // expected-warning {{'shared_trylock_function' attribute without capability arguments refers to 'this', but 'StfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class SHARED_TRYLOCK_FUNCTION(1) StfTestClass { // \
@@ -834,7 +843,8 @@ int stf_function_5() SHARED_TRYLOCK_FUNCTION(1, &mu1);
 int stf_function_6() SHARED_TRYLOCK_FUNCTION(1, muRef);
 int stf_function_7() SHARED_TRYLOCK_FUNCTION(1, muDoubleWrapper.getWrapper()->getMu());
 int stf_function_8() SHARED_TRYLOCK_FUNCTION(1, muPointer);
-int stf_function_9() SHARED_TRYLOCK_FUNCTION(true);
+int stf_function_9() SHARED_TRYLOCK_FUNCTION(true); // \
+  // expected-warning {{'shared_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 
 // illegal attribute arguments
@@ -863,11 +873,14 @@ int stf_function_bad_6() SHARED_TRYLOCK_FUNCTION(1, umu); // \
 
 // takes zero or more arguments, all locks (vars/fields)
 
-void uf_function() UNLOCK_FUNCTION();
+void uf_function() UNLOCK_FUNCTION(); // \
+  // expected-warning {{'unlock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
+
 
 void uf_function_args() UNLOCK_FUNCTION(mu1, mu2);
 
-int uf_testfn(int y) UNLOCK_FUNCTION();
+int uf_testfn(int y) UNLOCK_FUNCTION(); //\
+  // expected-warning {{'unlock_function' attribute without capability arguments can only be applied to non-static methods of a class}}
 
 int uf_testfn(int y) {
   int x UNLOCK_FUNCTION() = y; // \
@@ -882,7 +895,8 @@ class UfFoo {
  private:
   int test_field UNLOCK_FUNCTION(); // \
     // expected-warning {{'unlock_function' attribute only applies to functions}}
-  void test_method() UNLOCK_FUNCTION();
+  void test_method() UNLOCK_FUNCTION(); // \
+    // expected-warning {{'unlock_function' attribute without capability arguments refers to 'this', but 'UfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
 };
 
 class NO_THREAD_SAFETY_ANALYSIS UfTestClass { // \
@@ -1250,6 +1264,36 @@ struct Foomgoper {
   }
 };
 
+template <typename Mutex>
+struct SCOPED_LOCKABLE SLTemplateClass {
+  ~SLTemplateClass() UNLOCK_FUNCTION();
+};
+
+template <typename Mutex>
+struct NonSLTemplateClass {
+  ~NonSLTemplateClass() UNLOCK_FUNCTION(); // \
+    // expected-warning{{'unlock_function' attribute without capability arguments refers to 'this', but 'NonSLTemplateClass' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
+};
+
+template <>
+struct SLTemplateClass<int> {};
+
+template <typename Mutex>
+struct SLTemplateDerived : public SLTemplateClass<Mutex> {
+  ~SLTemplateDerived() UNLOCK_FUNCTION();
+};
+
+// FIXME: warn on template instantiation.
+template struct SLTemplateDerived<int>;
+
+struct SLDerived1 : public SLTemplateClass<double> {
+  ~SLDerived1() UNLOCK_FUNCTION();
+};
+
+struct SLDerived2 : public SLTemplateClass<int> {
+  ~SLDerived2() UNLOCK_FUNCTION(); // \
+    // expected-warning{{'unlock_function' attribute without capability arguments refers to 'this', but 'SLDerived2' isn't annotated with 'capability' or 'scoped_lockable' attribute}}
+};
 
 //-----------------------------------------------------
 // Parsing of member variables and function parameters
@@ -1525,4 +1569,4 @@ namespace CRASH_POST_R301735 {
      Mutex mu_;
    };
 }
-#endif
+#endif