[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] D36122: Thread Safety Analysis: fix assert_capability.

[Josh Gao via Phabricator via cfe-commits]

jmgao created this revision.

Previously, the assert_capability attribute was completely ignored by
thread safety analysis.


https://reviews.llvm.org/D36122

Files:
  include/clang/Basic/Attr.td
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/SemaCXX/warn-thread-safety-analysis.cpp


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety 
-Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety 
-Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions 
-DUSE_ASSERT_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety 
-Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions 
-DUSE_ASSERT_CAPABILITY=1 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety 
-std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
@@ -13,8 +14,15 @@
 #define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
 #define EXCLUSIVE_LOCK_FUNCTION(...)
__attribute__((exclusive_lock_function(__VA_ARGS__)))
 #define SHARED_LOCK_FUNCTION(...)   
__attribute__((shared_lock_function(__VA_ARGS__)))
+
+#if USE_ASSERT_CAPABILITY
+#define ASSERT_EXCLUSIVE_LOCK(...)  
__attribute__((assert_capability(__VA_ARGS__)))
+#define ASSERT_SHARED_LOCK(...) 
__attribute__((assert_shared_capability(__VA_ARGS__)))
+#else
 #define ASSERT_EXCLUSIVE_LOCK(...)  
__attribute__((assert_exclusive_lock(__VA_ARGS__)))
 #define ASSERT_SHARED_LOCK(...) 
__attribute__((assert_shared_lock(__VA_ARGS__)))
+#endif
+
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) 
__attribute__((exclusive_trylock_function(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)
__attribute__((shared_trylock_function(__VA_ARGS__)))
 #define UNLOCK_FUNCTION(...)
__attribute__((unlock_function(__VA_ARGS__)))
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -5686,8 +5686,12 @@
 
 static void handleAssertCapabilityAttr(Sema &S, Decl *D,
const AttributeList &Attr) {
+  SmallVector Args;
+  if (!checkLockFunAttrCommon(S, D, Attr, Args))
+return;
+
   D->addAttr(::new (S.Context) AssertCapabilityAttr(Attr.getRange(), S.Context,
-Attr.getArgAsExpr(0),
+Args.data(), Args.size(),
 Attr.getAttributeSpellingListIndex()));
 }
 
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1735,8 +1735,23 @@
 CapExprSet AssertLocks;
 Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
 for (const auto &AssertLock : AssertLocks)
-  Analyzer->addLock(FSet, llvm::make_unique(
-  AssertLock, LK_Shared, Loc, false, true),
+  Analyzer->addLock(FSet,
+llvm::make_unique(
+AssertLock, LK_Shared, Loc, false, true),
+ClassifyDiagnostic(A));
+break;
+  }
+
+  case attr::AssertCapability: {
+AssertCapabilityAttr *A = cast(At);
+CapExprSet AssertLocks;
+Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+for (const auto &AssertLock : AssertLocks)
+  Analyzer->addLock(FSet,
+llvm::make_unique(
+AssertLock,
+A->isShared() ? LK_Shared : LK_Exclusive, Loc,
+false, true),
 ClassifyDiagnostic(A));
 break;
   }
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -2138,7 +2138,7 @@
   let TemplateDependent = 1;
   let ParseArgumentsAsUnevaluated = 1;
   let DuplicatesAllowedWhileMerging = 1;
-  let Args = [ExprArgument<"Expr">];
+  let Args = [VariadicExprArgument<"Args">];
   let Accessors = [Accessor<"isShared",
 [GNU<"assert_shared_capability">,
  CXX11<"clang", "assert_shared_capability">]>];


Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only 

[PATCH] D36122: Thread Safety Analysis: fix assert_capability.

[Josh Gao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL309725: Thread Safety Analysis: fix assert_capability. 
(authored by jmgao).

Repository:
  rL LLVM

https://reviews.llvm.org/D36122

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/lib/Analysis/ThreadSafety.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp


Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -5686,8 +5686,12 @@
 
 static void handleAssertCapabilityAttr(Sema &S, Decl *D,
const AttributeList &Attr) {
+  SmallVector Args;
+  if (!checkLockFunAttrCommon(S, D, Attr, Args))
+return;
+
   D->addAttr(::new (S.Context) AssertCapabilityAttr(Attr.getRange(), S.Context,
-Attr.getArgAsExpr(0),
+Args.data(), Args.size(),
 Attr.getAttributeSpellingListIndex()));
 }
 
Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -1735,8 +1735,23 @@
 CapExprSet AssertLocks;
 Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
 for (const auto &AssertLock : AssertLocks)
-  Analyzer->addLock(FSet, llvm::make_unique(
-  AssertLock, LK_Shared, Loc, false, true),
+  Analyzer->addLock(FSet,
+llvm::make_unique(
+AssertLock, LK_Shared, Loc, false, true),
+ClassifyDiagnostic(A));
+break;
+  }
+
+  case attr::AssertCapability: {
+AssertCapabilityAttr *A = cast(At);
+CapExprSet AssertLocks;
+Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+for (const auto &AssertLock : AssertLocks)
+  Analyzer->addLock(FSet,
+llvm::make_unique(
+AssertLock,
+A->isShared() ? LK_Shared : LK_Exclusive, Loc,
+false, true),
 ClassifyDiagnostic(A));
 break;
   }
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -2138,7 +2138,7 @@
   let TemplateDependent = 1;
   let ParseArgumentsAsUnevaluated = 1;
   let DuplicatesAllowedWhileMerging = 1;
-  let Args = [ExprArgument<"Expr">];
+  let Args = [VariadicExprArgument<"Args">];
   let Accessors = [Accessor<"isShared",
 [GNU<"assert_shared_capability">,
  CXX11<"clang", "assert_shared_capability">]>];
Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety 
-Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety 
-Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions 
-DUSE_ASSERT_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety 
-Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions 
-DUSE_ASSERT_CAPABILITY=1 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety 
-std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
@@ -13,8 +14,15 @@
 #define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
 #define EXCLUSIVE_LOCK_FUNCTION(...)
__attribute__((exclusive_lock_function(__VA_ARGS__)))
 #define SHARED_LOCK_FUNCTION(...)   
__attribute__((shared_lock_function(__VA_ARGS__)))
+
+#if USE_ASSERT_CAPABILITY
+#define ASSERT_EXCLUSIVE_LOCK(...)  
__attribute__((assert_capability(__VA_ARGS__)))
+#define ASSERT_SHARED_LOCK(...) 
__attribute__((assert_shared_capability(__VA_ARGS__)))
+#else
 #define ASSERT_EXCLUSIVE_LOCK(...)  
__attribute__((assert_exclusive_lock(__VA_ARGS__)))
 #define ASSERT_SHARED_LOCK(...) 
__attribute__((assert_shared_lock(__VA_ARGS__)))
+#endif
+
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) 
__attribute__((exclusive_trylock_function(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)
__attribute__((shared_trylock_function(__VA_ARGS__)))
 #define UNLOCK_FUNCTION(...)
__attribute__((unlock_function(__VA_ARGS__)))


Index: cfe/trunk/lib/Sema/SemaDeclAttr.c

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

[Josh Gao via Phabricator via cfe-commits]

jmgao updated this revision to Diff 109411.
jmgao added a comment.

Remove accidental trailing backslash.


https://reviews.llvm.org/D36237

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-analysis.cpp
  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
@@ -571,11 +571,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 arguments cannot be applied to a free function}}
 
 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 arguments cannot be applied to a free function}}
 
 int elf_testfn(int y) {
   int x EXCLUSIVE_LOCK_FUNCTION() = y; // \
@@ -590,7 +590,7 @@
  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 requires type annotated with 'capability' attribute; type here is 'ElfFoo *'}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -643,11 +643,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 arguments cannot be applied to a free function}}
 
 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 arguments cannot be applied to a free function}}
 
 int slf_testfn(int y) {
   int x SHARED_LOCK_FUNCTION() = y; // \
@@ -665,7 +665,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 requires type annotated with 'capability' attribute; type here is 'SlfFoo *'}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -716,29 +717,32 @@
 // 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));  // \
-  // expected-error {{'exclusive_trylock_function' attribute takes at least 1 argument}}
+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 arguments cannot be applied to a free function}}
 
-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 arguments cannot be applied to a free function}}
 
 int etf_testfn(int y) {
   int x EXCLUSIVE_TRYLOCK_FUNCTION(1) = y; // \
 // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
   return x;
 };
 
 int etf_test_var EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
-  // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
+  // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}} \
 
 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 requires type annotated with 'capability' attribute; type here is 'EtfFoo *'}}
 };
 
 class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
@@ -759,7 +763,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_

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

[Josh Gao via Phabricator via cfe-commits]

jmgao updated this revision to Diff 109412.
jmgao added a comment.

Fix commit messages.


https://reviews.llvm.org/D36237

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-analysis.cpp
  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
@@ -571,11 +571,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 arguments cannot be applied to a free function}}
 
 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 arguments cannot be applied to a free function}}
 
 int elf_testfn(int y) {
   int x EXCLUSIVE_LOCK_FUNCTION() = y; // \
@@ -590,7 +590,7 @@
  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 requires type annotated with 'capability' attribute; type here is 'ElfFoo *'}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -643,11 +643,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 arguments cannot be applied to a free function}}
 
 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 arguments cannot be applied to a free function}}
 
 int slf_testfn(int y) {
   int x SHARED_LOCK_FUNCTION() = y; // \
@@ -665,7 +665,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 requires type annotated with 'capability' attribute; type here is 'SlfFoo *'}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -716,29 +717,32 @@
 // 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));  // \
-  // expected-error {{'exclusive_trylock_function' attribute takes at least 1 argument}}
+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 arguments cannot be applied to a free function}}
 
-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 arguments cannot be applied to a free function}}
 
 int etf_testfn(int y) {
   int x EXCLUSIVE_TRYLOCK_FUNCTION(1) = y; // \
 // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
   return x;
 };
 
 int etf_test_var EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
-  // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
+  // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}} \
 
 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 requires type annotated with 'capability' attribute; type here is 'EtfFoo *'}}
 };
 
 class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
@@ -759,7 +763,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() EXCL

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

[Josh Gao via Phabricator via cfe-commits]

jmgao updated this revision to Diff 110054.
jmgao added a comment.

Reword warnings.


https://reviews.llvm.org/D36237

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/ThreadSafety.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-capabilities.c
  test/SemaCXX/warn-thread-safety-analysis.cpp
  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
@@ -571,11 +571,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 arguments can only be applied to a method 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 arguments can only be applied to a method of a class}}
 
 int elf_testfn(int y) {
   int x EXCLUSIVE_LOCK_FUNCTION() = y; // \
@@ -590,7 +590,7 @@
  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 requires type annotated with 'capability' attribute; type here is 'ElfFoo *'}}
 };
 
 class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
@@ -643,11 +643,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 arguments can only be applied to a method 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 arguments can only be applied to a method of a class}}
 
 int slf_testfn(int y) {
   int x SHARED_LOCK_FUNCTION() = y; // \
@@ -665,7 +665,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 requires type annotated with 'capability' attribute; type here is 'SlfFoo *'}}
 };
 
 class SHARED_LOCK_FUNCTION() SlfTestClass { // \
@@ -716,29 +717,32 @@
 // 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));  // \
-  // expected-error {{'exclusive_trylock_function' attribute takes at least 1 argument}}
+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 arguments can only be applied to a method 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 arguments can only be applied to a method of a class}}
 
 int etf_testfn(int y) {
   int x EXCLUSIVE_TRYLOCK_FUNCTION(1) = y; // \
 // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
   return x;
 };
 
 int etf_test_var EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
-  // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
+  // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}} \
 
 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 requires type annotated with 'capability' attribute; type here is 'EtfFoo *'}}
 };
 
 class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \
@@ -759,7 +763,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, muPoin

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

[Josh Gao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310403: Thread Safety Analysis: warn on nonsensical 
attributes. (authored by jmgao).

Changed prior to commit:
  https://reviews.llvm.org/D36237?vs=110054&id=110259#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36237

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

Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -480,7 +480,7 @@
   return nullptr;
 }
 
-static bool checkRecordTypeForCapability(Sema &S, QualType Ty) {
+template  static bool checkRecordTypeForAttr(Sema &S, QualType Ty) {
   const RecordType *RT = getRecordType(Ty);
 
   if (!RT)
@@ -497,43 +497,51 @@
 
   // Check if the record itself has a capability.
   RecordDecl *RD = RT->getDecl();
-  if (RD->hasAttr())
+  if (RD->hasAttr())
 return true;
 
   // Else check if any base classes have a capability.
   if (CXXRecordDecl *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();
+  return Type->getDecl()->hasAttr();
 }, BPaths))
   return true;
   }
   return false;
 }
 
-static bool checkTypedefTypeForCapability(QualType Ty) {
+template  static bool checkTypedefTypeForAttr(QualType Ty) {
   const auto *TD = Ty->getAs();
   if (!TD)
 return false;
 
   TypedefNameDecl *TN = TD->getDecl();
   if (!TN)
 return false;
 
-  return TN->hasAttr();
+  return TN->hasAttr();
 }
 
-static bool typeHasCapability(Sema &S, QualType Ty) {
-  if (checkTypedefTypeForCapability(Ty))
+template  static bool typeHasAttr(Sema &S, QualType Ty) {
+  if (checkTypedefTypeForAttr(Ty))
 return true;
 
-  if (checkRecordTypeForCapability(S, Ty))
+  if (checkRecordTypeForAttr(S, Ty))
 return true;
 
   return false;
 }
 
+static bool typeHasCapability(Sema &S, QualType Ty) {
+  return typeHasAttr(S, Ty);
+}
+
+static bool typeHasScopedLockable(Sema &S, QualType Ty) {
+  return typeHasAttr(S, Ty);
+}
+
 static bool isCapabilityExpr(Sema &S, const Expr *Ex) {
   // Capability expressions are simple expressions involving the boolean logic
   // operators &&, || or !, a simple DeclRefExpr, CastExpr or a ParenExpr. Once
@@ -570,6 +578,8 @@
SmallVectorImpl &Args,
int Sidx = 0,
bool ParamIdxOk = false) {
+  bool TriedParam = false;
+
   for (unsigned Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) {
 Expr *ArgExp = Attr.getArgAsExpr(Idx);
 
@@ -610,15 +620,18 @@
 const RecordType *RT = getRecordType(ArgTy);
 
 // Now check if we index into a record type function param.
-if(!RT && ParamIdxOk) {
+if (!RT && ParamIdxOk) {
   FunctionDecl *FD = dyn_cast(D);
   IntegerLiteral *IL = dyn_cast(ArgExp);
-  if(FD && IL) {
+  if (FD && IL) {
+// Don't emit free function warnings if an index was given.
+TriedParam = true;
+
 unsigned int NumParams = FD->getNumParams();
 llvm::APInt ArgValue = IL->getValue();
 uint64_t ParamIdxFromOne = ArgValue.getZExtValue();
 uint64_t ParamIdxFromZero = ParamIdxFromOne - 1;
-if(!ArgValue.isStrictlyPositive() || ParamIdxFromOne > NumParams) {
+if (!ArgValue.isStrictlyPositive() || ParamIdxFromOne > NumParams) {
   S.Diag(Attr.getLoc(), diag::err_attribute_argument_out_of_range)
 << Attr.getName() << Idx + 1 << NumParams;
   continue;
@@ -637,6 +650,28 @@
 
 Args.push_back(ArgExp);
   }
+
+  // If we don't have any lockable arguments, verify that we're an instance
+  // method on a lockable type.
+  if (Args.empty() && !TriedParam) {
+if (auto *MD = dyn_cast(D)) {
+  if (MD->isStatic()) {
+S.Diag(Attr.getLoc(), diag::warn_thread_attribute_noargs_static_method)
+<< Attr.getName();
+return;
+  }
+
+  QualType ThisType = MD->getThisType(MD->getASTContext());
+  if (!typeHasCapability(S, ThisType) &&
+  !typeHasScopedLockable(S, ThisType)) {
+S.Diag(Attr.getLoc(), diag::warn_thread_attribute_noargs_not_lockable)
+<< Attr.getName() << ThisType;
+  }
+} else {
+  S.Diag(Attr.getLoc(), diag::warn_thread_attribute_noargs_not_method)
+  << Attr.getName();
+}
+  }
 }
 
 //===--===//
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Ba

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

[Josh Gao via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL310402: Reland "Thread Safety Analysis: fix 
assert_capability." (authored by jmgao).

Changed prior to commit:
  https://reviews.llvm.org/D36237?vs=110054&id=110260#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36237

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/lib/Analysis/ThreadSafety.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/Sema/attr-capabilities.c
  cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp

Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -5700,8 +5700,12 @@
 
 static void handleAssertCapabilityAttr(Sema &S, Decl *D,
const AttributeList &Attr) {
+  SmallVector Args;
+  if (!checkLockFunAttrCommon(S, D, Attr, Args))
+return;
+
   D->addAttr(::new (S.Context) AssertCapabilityAttr(Attr.getRange(), S.Context,
-Attr.getArgAsExpr(0),
+Args.data(), Args.size(),
 Attr.getAttributeSpellingListIndex()));
 }
 
Index: cfe/trunk/lib/Analysis/ThreadSafety.cpp
===
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp
@@ -1735,8 +1735,23 @@
 CapExprSet AssertLocks;
 Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
 for (const auto &AssertLock : AssertLocks)
-  Analyzer->addLock(FSet, llvm::make_unique(
-  AssertLock, LK_Shared, Loc, false, true),
+  Analyzer->addLock(FSet,
+llvm::make_unique(
+AssertLock, LK_Shared, Loc, false, true),
+ClassifyDiagnostic(A));
+break;
+  }
+
+  case attr::AssertCapability: {
+AssertCapabilityAttr *A = cast(At);
+CapExprSet AssertLocks;
+Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+for (const auto &AssertLock : AssertLocks)
+  Analyzer->addLock(FSet,
+llvm::make_unique(
+AssertLock,
+A->isShared() ? LK_Shared : LK_Exclusive, Loc,
+false, true),
 ClassifyDiagnostic(A));
 break;
   }
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -2138,7 +2138,7 @@
   let TemplateDependent = 1;
   let ParseArgumentsAsUnevaluated = 1;
   let DuplicatesAllowedWhileMerging = 1;
-  let Args = [ExprArgument<"Expr">];
+  let Args = [VariadicExprArgument<"Args">];
   let Accessors = [Accessor<"isShared",
 [GNU<"assert_shared_capability">,
  CXX11<"clang", "assert_shared_capability">]>];
Index: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
===
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=0 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=1 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
@@ -13,8 +14,15 @@
 #define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
 #define EXCLUSIVE_LOCK_FUNCTION(...)__attribute__((exclusive_lock_function(__VA_ARGS__)))
 #define SHARED_LOCK_FUNCTION(...)   __attribute__((shared_lock_function(__VA_ARGS__)))
+
+#if USE_ASSERT_CAPABILITY
+#define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__((assert_capability(__VA_ARGS__)))
+#define ASSERT_SHARED_LOCK(...) __attribute__((assert_shared_capability(__VA_ARGS__)))
+#else
 #define ASSERT_EXCLUSIVE_LOCK(...)  __attribute__((assert_exclusive_lock(__VA_ARGS__)))
 #define ASSERT_SHARED_LOCK(...) __attribute__((assert_shared_lock(__VA_ARGS__)))
+#endif
+
 #define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((exclusive_trylock_function(__VA_ARGS__)))
 #define SHARED_TRYLOCK_FUNCTION(...)__attribute__((shared_trylock_function(__VA_ARGS__)))
 #define U

[PATCH] D36237: Reland "Thread Safety Analysis: fix assert_capability.", add warnings

[Josh Gao via Phabricator via cfe-commits]

jmgao added a comment.

Thanks for the review!


Repository:
  rL LLVM

https://reviews.llvm.org/D36237



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