Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.

[NAKAMURA Takumi via cfe-commits]

Thanks!

On Fri, Aug 11, 2017 at 4:56 PM Josh Gao  wrote:

> Reverted in r310698.
>
> On Fri, Aug 11, 2017 at 12:51 AM, Josh Gao  wrote:
>
>> Sorry for the breakage, I'm reverting the patch that caused this now.
>>
>> On Fri, Aug 11, 2017 at 12:36 AM, NAKAMURA Takumi 
>> wrote:
>>
>>> It causes failure in check-libcxx with just-built clang.
>>>
>>>
>>> http://bb.pgr.jp/builders/bootstrap-clang-libcxx-lld-i686-linux/builds/758
>>>
>>>
>>> On Wed, Aug 9, 2017 at 4:45 AM Josh Gao via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: jmgao
 Date: Tue Aug  8 12:44:35 2017
 New Revision: 310403

 URL: http://llvm.org/viewvc/llvm-project?rev=310403&view=rev
 Log:
 Thread Safety Analysis: warn on nonsensical attributes.

 Add warnings in cases where an implicit `this` argument is expected to
 attributes because either `this` doesn't exist because the attribute is
 on a free function, or because `this` is on a type that doesn't have a
 corresponding capability/lockable/scoped_lockable attribute.

 Reviewers: delesley, aaron.ballman

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

 Modified:
 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

 Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=310403&r1=310402&r2=310403&view=diff

 ==
 --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
 +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Aug  8
 12:44:35 2017
 @@ -2932,6 +2932,16 @@ def warn_thread_attribute_decl_not_locka
"%0 attribute can only be applied in a context annotated "
"with 'capability(\"mutex\")' attribute">,
InGroup, DefaultIgnore;
 +def warn_thread_attribute_noargs_not_lockable : Warning<
 +  "%0 attribute requires type annotated with 'capability' attribute; "
 +  "type here is %1">,
 +  InGroup, DefaultIgnore;
 +def warn_thread_attribute_noargs_not_method : Warning<
 +  "%0 attribute without arguments can only be applied to a method of a
 class">,
 +  InGroup, DefaultIgnore;
 +def warn_thread_attribute_noargs_static_method : Warning<
 +  "%0 attribute without arguments cannot be applied to a static
 method">,
 +  InGroup, DefaultIgnore;
  def warn_thread_attribute_decl_not_pointer : Warning<
"%0 only applies to pointer types; type here is %1">,
InGroup, DefaultIgnore;

 Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=310403&r1=310402&r2=310403&view=diff

 ==
 --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
 +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Aug  8 12:44:35 2017
 @@ -480,7 +480,7 @@ static const RecordType *getRecordType(Q
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,7 +497,7 @@ static bool checkRecordTypeForCapability

// 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.
 @@ -505,14 +505,14 @@ static bool checkRecordTypeForCapability
  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;
 @@ -521,19 +521,27 @@ static bool checkTypedefTypeForCapabilit
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

Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.

[Josh Gao via cfe-commits]

Reverted in r310698.

On Fri, Aug 11, 2017 at 12:51 AM, Josh Gao  wrote:

> Sorry for the breakage, I'm reverting the patch that caused this now.
>
> On Fri, Aug 11, 2017 at 12:36 AM, NAKAMURA Takumi 
> wrote:
>
>> It causes failure in check-libcxx with just-built clang.
>>
>> http://bb.pgr.jp/builders/bootstrap-clang-libcxx-lld-i686-
>> linux/builds/758
>>
>>
>> On Wed, Aug 9, 2017 at 4:45 AM Josh Gao via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: jmgao
>>> Date: Tue Aug  8 12:44:35 2017
>>> New Revision: 310403
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=310403&view=rev
>>> Log:
>>> Thread Safety Analysis: warn on nonsensical attributes.
>>>
>>> Add warnings in cases where an implicit `this` argument is expected to
>>> attributes because either `this` doesn't exist because the attribute is
>>> on a free function, or because `this` is on a type that doesn't have a
>>> corresponding capability/lockable/scoped_lockable attribute.
>>>
>>> Reviewers: delesley, aaron.ballman
>>>
>>> Differential Revision: https://reviews.llvm.org/D36237
>>>
>>> Modified:
>>> 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
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>>> Basic/DiagnosticSemaKinds.td?rev=310403&r1=310402&r2=310403&view=diff
>>> 
>>> ==
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Aug  8
>>> 12:44:35 2017
>>> @@ -2932,6 +2932,16 @@ def warn_thread_attribute_decl_not_locka
>>>"%0 attribute can only be applied in a context annotated "
>>>"with 'capability(\"mutex\")' attribute">,
>>>InGroup, DefaultIgnore;
>>> +def warn_thread_attribute_noargs_not_lockable : Warning<
>>> +  "%0 attribute requires type annotated with 'capability' attribute; "
>>> +  "type here is %1">,
>>> +  InGroup, DefaultIgnore;
>>> +def warn_thread_attribute_noargs_not_method : Warning<
>>> +  "%0 attribute without arguments can only be applied to a method of a
>>> class">,
>>> +  InGroup, DefaultIgnore;
>>> +def warn_thread_attribute_noargs_static_method : Warning<
>>> +  "%0 attribute without arguments cannot be applied to a static
>>> method">,
>>> +  InGroup, DefaultIgnore;
>>>  def warn_thread_attribute_decl_not_pointer : Warning<
>>>"%0 only applies to pointer types; type here is %1">,
>>>InGroup, DefaultIgnore;
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaD
>>> eclAttr.cpp?rev=310403&r1=310402&r2=310403&view=diff
>>> 
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Aug  8 12:44:35 2017
>>> @@ -480,7 +480,7 @@ static const RecordType *getRecordType(Q
>>>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,7 +497,7 @@ static bool checkRecordTypeForCapability
>>>
>>>// 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.
>>> @@ -505,14 +505,14 @@ static bool checkRecordTypeForCapability
>>>  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;
>>> @@ -521,19 +521,27 @@ static bool checkTypedefTypeForCapabilit
>>>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 typeH

Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.

[Josh Gao via cfe-commits]

Sorry for the breakage, I'm reverting the patch that caused this now.

On Fri, Aug 11, 2017 at 12:36 AM, NAKAMURA Takumi 
wrote:

> It causes failure in check-libcxx with just-built clang.
>
> http://bb.pgr.jp/builders/bootstrap-clang-libcxx-lld-i686-linux/builds/758
>
>
> On Wed, Aug 9, 2017 at 4:45 AM Josh Gao via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: jmgao
>> Date: Tue Aug  8 12:44:35 2017
>> New Revision: 310403
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310403&view=rev
>> Log:
>> Thread Safety Analysis: warn on nonsensical attributes.
>>
>> Add warnings in cases where an implicit `this` argument is expected to
>> attributes because either `this` doesn't exist because the attribute is
>> on a free function, or because `this` is on a type that doesn't have a
>> corresponding capability/lockable/scoped_lockable attribute.
>>
>> Reviewers: delesley, aaron.ballman
>>
>> Differential Revision: https://reviews.llvm.org/D36237
>>
>> Modified:
>> 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
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/
>> DiagnosticSemaKinds.td?rev=310403&r1=310402&r2=310403&view=diff
>> 
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Aug  8
>> 12:44:35 2017
>> @@ -2932,6 +2932,16 @@ def warn_thread_attribute_decl_not_locka
>>"%0 attribute can only be applied in a context annotated "
>>"with 'capability(\"mutex\")' attribute">,
>>InGroup, DefaultIgnore;
>> +def warn_thread_attribute_noargs_not_lockable : Warning<
>> +  "%0 attribute requires type annotated with 'capability' attribute; "
>> +  "type here is %1">,
>> +  InGroup, DefaultIgnore;
>> +def warn_thread_attribute_noargs_not_method : Warning<
>> +  "%0 attribute without arguments can only be applied to a method of a
>> class">,
>> +  InGroup, DefaultIgnore;
>> +def warn_thread_attribute_noargs_static_method : Warning<
>> +  "%0 attribute without arguments cannot be applied to a static method">,
>> +  InGroup, DefaultIgnore;
>>  def warn_thread_attribute_decl_not_pointer : Warning<
>>"%0 only applies to pointer types; type here is %1">,
>>InGroup, DefaultIgnore;
>>
>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
>> SemaDeclAttr.cpp?rev=310403&r1=310402&r2=310403&view=diff
>> 
>> ==
>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Aug  8 12:44:35 2017
>> @@ -480,7 +480,7 @@ static const RecordType *getRecordType(Q
>>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,7 +497,7 @@ static bool checkRecordTypeForCapability
>>
>>// 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.
>> @@ -505,14 +505,14 @@ static bool checkRecordTypeForCapability
>>  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;
>> @@ -521,19 +521,27 @@ static bool checkTypedefTypeForCapabilit
>>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 express

Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.

[NAKAMURA Takumi via cfe-commits]

It causes failure in check-libcxx with just-built clang.

http://bb.pgr.jp/builders/bootstrap-clang-libcxx-lld-i686-linux/builds/758

On Wed, Aug 9, 2017 at 4:45 AM Josh Gao via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: jmgao
> Date: Tue Aug  8 12:44:35 2017
> New Revision: 310403
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310403&view=rev
> Log:
> Thread Safety Analysis: warn on nonsensical attributes.
>
> Add warnings in cases where an implicit `this` argument is expected to
> attributes because either `this` doesn't exist because the attribute is
> on a free function, or because `this` is on a type that doesn't have a
> corresponding capability/lockable/scoped_lockable attribute.
>
> Reviewers: delesley, aaron.ballman
>
> Differential Revision: https://reviews.llvm.org/D36237
>
> Modified:
> 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
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=310403&r1=310402&r2=310403&view=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Aug  8
> 12:44:35 2017
> @@ -2932,6 +2932,16 @@ def warn_thread_attribute_decl_not_locka
>"%0 attribute can only be applied in a context annotated "
>"with 'capability(\"mutex\")' attribute">,
>InGroup, DefaultIgnore;
> +def warn_thread_attribute_noargs_not_lockable : Warning<
> +  "%0 attribute requires type annotated with 'capability' attribute; "
> +  "type here is %1">,
> +  InGroup, DefaultIgnore;
> +def warn_thread_attribute_noargs_not_method : Warning<
> +  "%0 attribute without arguments can only be applied to a method of a
> class">,
> +  InGroup, DefaultIgnore;
> +def warn_thread_attribute_noargs_static_method : Warning<
> +  "%0 attribute without arguments cannot be applied to a static method">,
> +  InGroup, DefaultIgnore;
>  def warn_thread_attribute_decl_not_pointer : Warning<
>"%0 only applies to pointer types; type here is %1">,
>InGroup, DefaultIgnore;
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=310403&r1=310402&r2=310403&view=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Aug  8 12:44:35 2017
> @@ -480,7 +480,7 @@ static const RecordType *getRecordType(Q
>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,7 +497,7 @@ static bool checkRecordTypeForCapability
>
>// 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.
> @@ -505,14 +505,14 @@ static bool checkRecordTypeForCapability
>  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;
> @@ -521,19 +521,27 @@ static bool checkTypedefTypeForCapabilit
>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 @@ static void checkAttrArgsAreCapabilityOb
> SmallVectorImpl &Args,

Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.

[George Burgess IV via cfe-commits]

Sorry, I meant

bin/clang -Wthread-safety-attributes -Wthread-safety-analysis
/tmp/tc.cpp -std=c++17 -c -o/dev/null

(had -Wthread-safety-attributes twice in the email)

George

On Thu, Aug 10, 2017 at 4:08 PM, George Burgess IV
 wrote:
> Hello!
>
> It looks like this is causing buildbot failures related to libc++'s
> lock_guard and scoped_lock:
> http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan_check/4070/consoleFull
>
> Here's a reduced test-case (from libc++'s __mutex_base):
>
> struct __attribute__((capability("mutex"))) mutex {
>   mutex();
>   ~mutex();
> };
>
> template  struct __attribute__((scoped_lockable)) Foo {
>   _Mutex &__m_;
>
>   explicit Foo(_Mutex &__m) __attribute__((acquire_capability(__m)))
>   : __m_(__m) {}
>
>   ~Foo() __attribute__((release_capability())) {}
> };
>
> int main() {
>   ::mutex m;
>   Foo f(m);
> }
>
>
> Built with `clang -Wthread-safety-attributes
> -Wthread-safety-attributes /tmp/tc.cpp -std=c++17 -c -o/dev/null`, I
> see the following warning:
> warning: 'release_capability' attribute requires type annotated with
> 'capability' attribute; type here is 'Foo<_Mutex> *'
> [-Wthread-safety-attributes]
>
> If I change ~Foo to release_capability(__m_), I get warnings about
> both 'm' being held at the end of main, and about releasing f.__m_,
> which was not held.
>
> Since the buildbot uses -Werror, ...
>
> Can you look into this, please? :)
>
> Thanks,
> George
>
> On Tue, Aug 8, 2017 at 12:44 PM, Josh Gao via cfe-commits
>  wrote:
>> Author: jmgao
>> Date: Tue Aug  8 12:44:35 2017
>> New Revision: 310403
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310403&view=rev
>> Log:
>> Thread Safety Analysis: warn on nonsensical attributes.
>>
>> Add warnings in cases where an implicit `this` argument is expected to
>> attributes because either `this` doesn't exist because the attribute is
>> on a free function, or because `this` is on a type that doesn't have a
>> corresponding capability/lockable/scoped_lockable attribute.
>>
>> Reviewers: delesley, aaron.ballman
>>
>> Differential Revision: https://reviews.llvm.org/D36237
>>
>> Modified:
>> 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
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=310403&r1=310402&r2=310403&view=diff
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Aug  8 12:44:35 
>> 2017
>> @@ -2932,6 +2932,16 @@ def warn_thread_attribute_decl_not_locka
>>"%0 attribute can only be applied in a context annotated "
>>"with 'capability(\"mutex\")' attribute">,
>>InGroup, DefaultIgnore;
>> +def warn_thread_attribute_noargs_not_lockable : Warning<
>> +  "%0 attribute requires type annotated with 'capability' attribute; "
>> +  "type here is %1">,
>> +  InGroup, DefaultIgnore;
>> +def warn_thread_attribute_noargs_not_method : Warning<
>> +  "%0 attribute without arguments can only be applied to a method of a 
>> class">,
>> +  InGroup, DefaultIgnore;
>> +def warn_thread_attribute_noargs_static_method : Warning<
>> +  "%0 attribute without arguments cannot be applied to a static method">,
>> +  InGroup, DefaultIgnore;
>>  def warn_thread_attribute_decl_not_pointer : Warning<
>>"%0 only applies to pointer types; type here is %1">,
>>InGroup, DefaultIgnore;
>>
>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=310403&r1=310402&r2=310403&view=diff
>> ==
>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Aug  8 12:44:35 2017
>> @@ -480,7 +480,7 @@ static const RecordType *getRecordType(Q
>>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,7 +497,7 @@ static bool checkRecordTypeForCapability
>>
>>// 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.
>> @@ -505,14 +505,14 @@ static bool checkRecordTypeForCapability
>>  CXXBasePaths BPaths(false, false);
>>  if (CRD->lookupInBases([](const CXXBaseSpecifier *BS, CXXBasePath &) {
>>const auto *Type = BS->getType()->getAs();
>> -  return Type->getDecl()->hasAttr();
>> +  return Type->getDecl()->ha

Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.

[George Burgess IV via cfe-commits]

Hello!

It looks like this is causing buildbot failures related to libc++'s
lock_guard and scoped_lock:
http://green.lab.llvm.org/green/job/clang-stage2-cmake-RgSan_check/4070/consoleFull

Here's a reduced test-case (from libc++'s __mutex_base):

struct __attribute__((capability("mutex"))) mutex {
  mutex();
  ~mutex();
};

template  struct __attribute__((scoped_lockable)) Foo {
  _Mutex &__m_;

  explicit Foo(_Mutex &__m) __attribute__((acquire_capability(__m)))
  : __m_(__m) {}

  ~Foo() __attribute__((release_capability())) {}
};

int main() {
  ::mutex m;
  Foo f(m);
}


Built with `clang -Wthread-safety-attributes
-Wthread-safety-attributes /tmp/tc.cpp -std=c++17 -c -o/dev/null`, I
see the following warning:
warning: 'release_capability' attribute requires type annotated with
'capability' attribute; type here is 'Foo<_Mutex> *'
[-Wthread-safety-attributes]

If I change ~Foo to release_capability(__m_), I get warnings about
both 'm' being held at the end of main, and about releasing f.__m_,
which was not held.

Since the buildbot uses -Werror, ...

Can you look into this, please? :)

Thanks,
George

On Tue, Aug 8, 2017 at 12:44 PM, Josh Gao via cfe-commits
 wrote:
> Author: jmgao
> Date: Tue Aug  8 12:44:35 2017
> New Revision: 310403
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310403&view=rev
> Log:
> Thread Safety Analysis: warn on nonsensical attributes.
>
> Add warnings in cases where an implicit `this` argument is expected to
> attributes because either `this` doesn't exist because the attribute is
> on a free function, or because `this` is on a type that doesn't have a
> corresponding capability/lockable/scoped_lockable attribute.
>
> Reviewers: delesley, aaron.ballman
>
> Differential Revision: https://reviews.llvm.org/D36237
>
> Modified:
> 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
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=310403&r1=310402&r2=310403&view=diff
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Aug  8 12:44:35 
> 2017
> @@ -2932,6 +2932,16 @@ def warn_thread_attribute_decl_not_locka
>"%0 attribute can only be applied in a context annotated "
>"with 'capability(\"mutex\")' attribute">,
>InGroup, DefaultIgnore;
> +def warn_thread_attribute_noargs_not_lockable : Warning<
> +  "%0 attribute requires type annotated with 'capability' attribute; "
> +  "type here is %1">,
> +  InGroup, DefaultIgnore;
> +def warn_thread_attribute_noargs_not_method : Warning<
> +  "%0 attribute without arguments can only be applied to a method of a 
> class">,
> +  InGroup, DefaultIgnore;
> +def warn_thread_attribute_noargs_static_method : Warning<
> +  "%0 attribute without arguments cannot be applied to a static method">,
> +  InGroup, DefaultIgnore;
>  def warn_thread_attribute_decl_not_pointer : Warning<
>"%0 only applies to pointer types; type here is %1">,
>InGroup, DefaultIgnore;
>
> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=310403&r1=310402&r2=310403&view=diff
> ==
> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Tue Aug  8 12:44:35 2017
> @@ -480,7 +480,7 @@ static const RecordType *getRecordType(Q
>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,7 +497,7 @@ static bool checkRecordTypeForCapability
>
>// 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.
> @@ -505,14 +505,14 @@ static bool checkRecordTypeForCapability
>  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;
> @@ -521,19 +521,27 @@ static bool checkTypedefTypeForCapabilit
>if (!TN)
>  return false;
>
> -  return TN->