[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added a comment.

In https://reviews.llvm.org/D51901#1239759, @delesley wrote:

> This looks okay to me, but I have not tested it on a large code base to see 
> if it breaks anything.


On our code base (not as large as Google's) there was one true positive and no 
false positives. At least for templates we should be fine now, since we assume 
that all type-dependent base classes could have any attribute.


Repository:
  rC Clang

https://reviews.llvm.org/D51901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342605: Thread Safety Analysis: warnings for attributes 
without arguments (authored by aaronpuchert, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D51901?vs=165004=166205#toc

Repository:
  rC Clang

https://reviews.llvm.org/D51901

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-parsing.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -476,6 +476,29 @@
   return nullptr;
 }
 
+template 
+static bool checkRecordDeclForAttr(const RecordDecl *RD) {
+  // Check if the record itself has the attribute.
+  if (RD->hasAttr())
+return true;
+
+  // Else check if any base classes have the attribute.
+  if (const auto *CRD = dyn_cast(RD)) {
+CXXBasePaths BPaths(false, false);
+if (CRD->lookupInBases(
+[](const CXXBaseSpecifier *BS, CXXBasePath &) {
+  const auto  = *BS->getType();
+  // If it's type-dependent, we assume it could have the attribute.
+  if (Ty.isDependentType())
+return true;
+  return Ty.getAs()->getDecl()->hasAttr();
+},
+BPaths, true))
+  return true;
+  }
+  return false;
+}
+
 static bool checkRecordTypeForCapability(Sema , QualType Ty) {
   const RecordType *RT = getRecordType(Ty);
 
@@ -491,21 +514,7 @@
   if (threadSafetyCheckIsSmartPointer(S, RT))
 return true;
 
-  // Check if the record itself has a capability.
-  RecordDecl *RD = RT->getDecl();
-  if (RD->hasAttr())
-return true;
-
-  // Else check if any base classes have a capability.
-  if (const auto *CRD = dyn_cast(RD)) {
-CXXBasePaths BPaths(false, false);
-if (CRD->lookupInBases([](const CXXBaseSpecifier *BS, CXXBasePath &) {
-  const auto *Type = BS->getType()->getAs();
-  return Type->getDecl()->hasAttr();
-}, BPaths))
-  return true;
-  }
-  return false;
+  return checkRecordDeclForAttr(RT->getDecl());
 }
 
 static bool checkTypedefTypeForCapability(QualType Ty) {
@@ -563,8 +572,27 @@
 static void checkAttrArgsAreCapabilityObjs(Sema , Decl *D,
const ParsedAttr ,
SmallVectorImpl ,
-   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(D);
+if (MD && !MD->isStatic()) {
+  const CXXRecordDecl *RD = MD->getParent();
+  // FIXME -- need to check this again on template instantiation
+  if (!checkRecordDeclForAttr(RD) &&
+  !checkRecordDeclForAttr(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);
 
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3008,6 +3008,14 @@
 def warn_thread_attribute_ignored : Warning<
   "ignoring %0 attribute because its argument is invalid">,
   InGroup, 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, 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, DefaultIgnore;
 def warn_thread_attribute_argument_not_lockable : Warning<
   "%0 attribute requires arguments whose type is annotated "
   "with 'capability' attribute; type here is %1">,
Index: test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- test/SemaCXX/warn-thread-safety-parsing.cpp
+++ test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -572,11 +572,11 @@
 
 // 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' 

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Delesley Hutchins via Phabricator via cfe-commits]

delesley accepted this revision.
delesley added a comment.
This revision is now accepted and ready to land.

This looks okay to me, but I have not tested it on a large code base to see if 
it breaks anything.




Comment at: lib/Sema/SemaDeclAttr.cpp:589
+<< AL << MD->getParent();
+} else
+  S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_non_static_member)

Curly braces around the else section.


Repository:
  rC Clang

https://reviews.llvm.org/D51901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert marked an inline comment as done.
aaronpuchert added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments can only be applied in a class "
+  "annotated with 'capability' or 'scoped_lockable' attribute, but %1 isn't">,
+  InGroup, DefaultIgnore;

jmgao wrote:
> aaronpuchert wrote:
> > Alternative wording: "%0 attribute without capability arguments refers to 
> > 'this', but %1 isn't annotated with 'capability' or 'scoped_lockable' 
> > attribute".
> Maybe something like "implicit 'this' argument for %0 attribute isn't 
> annotated with 'capability' or 'scoped_lockable" attribute"?
I like that it's short, but technically we want the argument's type—not the 
argument itself—to be annotated and I worry that this might not be clear. In 
the following warning message we talk about the "argument whose type is 
annotated" for example.



Comment at: test/SemaCXX/warn-thread-safety-parsing.cpp:1286-1287
+
+// FIXME: warn on template instantiation.
+template struct SLTemplateDerived;
+

The explicit instantiation appears in the AST in its full glory, but I'm not 
sure how make the analysis run over it. I saw that 
`Sema::ProcessDeclAttributeList` is called from 
`Sema::ActOnExplicitInstantiation`, but that seems to check the attributes on 
the class/function itself, not on members.


Repository:
  rC Clang

https://reviews.llvm.org/D51901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 165004.
aaronpuchert added a comment.

Improved handling of type-dependent base classes and slightly reworded warning 
message.


Repository:
  rC Clang

https://reviews.llvm.org/D51901

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-parsing.cpp

Index: test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- test/SemaCXX/warn-thread-safety-parsing.cpp
+++ test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -572,11 +572,11 @@
 
 // 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 @@
  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 @@
 
 // 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 @@
  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 @@
 // 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 @@
  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_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 

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Josh Gao via Phabricator via cfe-commits]

jmgao added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments can only be applied in a class "
+  "annotated with 'capability' or 'scoped_lockable' attribute, but %1 isn't">,
+  InGroup, DefaultIgnore;

aaronpuchert wrote:
> Alternative wording: "%0 attribute without capability arguments refers to 
> 'this', but %1 isn't annotated with 'capability' or 'scoped_lockable' 
> attribute".
Maybe something like "implicit 'this' argument for %0 attribute isn't annotated 
with 'capability' or 'scoped_lockable" attribute"?


Repository:
  rC Clang

https://reviews.llvm.org/D51901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

The second warning message is a bit long, so if any of you have a better idea 
I'd welcome it.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3016-3017
+def warn_thread_attribute_not_on_capability_member : Warning<
+  "%0 attribute without capability arguments can only be applied in a class "
+  "annotated with 'capability' or 'scoped_lockable' attribute, but %1 isn't">,
+  InGroup, DefaultIgnore;

Alternative wording: "%0 attribute without capability arguments refers to 
'this', but %1 isn't annotated with 'capability' or 'scoped_lockable' 
attribute".


Repository:
  rC Clang

https://reviews.llvm.org/D51901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51901: Thread Safety Analysis: warnings for attributes without arguments

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley, jmgao.
Herald added a subscriber: cfe-commits.

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 https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D51901

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-parsing.cpp

Index: test/SemaCXX/warn-thread-safety-parsing.cpp
===
--- test/SemaCXX/warn-thread-safety-parsing.cpp
+++ test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -572,11 +572,11 @@
 
 // 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 @@
  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 can only be applied in a class annotated with 'capability' or 'scoped_lockable' attribute, but 'ElfFoo' isn't}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -644,11 +645,11 @@
 
 // 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 @@
  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 can only be applied in a class annotated with 'capability' or 'scoped_lockable' attribute, but 'SlfFoo' isn't}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -717,14 +719,16 @@
 // 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