Re: r310403 - Thread Safety Analysis: warn on nonsensical attributes.
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.
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.
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.
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.
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.
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->
r310403 - Thread Safety Analysis: warn on nonsensical attributes.
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, 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 @@ static void checkAttrArgsAreCapabilityOb const RecordType *RT = getRecordType(ArgTy); // Now check if we index into a record type function param. -i