r310403 - Thread Safety Analysis: warn on nonsensical attributes.
George Burgess IV via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 10 16:08:13 PDT 2017
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 <typename _Mutex> 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
<cfe-commits at 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<ThreadSafetyAttributes>, DefaultIgnore;
> +def warn_thread_attribute_noargs_not_lockable : Warning<
> + "%0 attribute requires type annotated with 'capability' attribute; "
> + "type here is %1">,
> + InGroup<ThreadSafetyAttributes>, DefaultIgnore;
> +def warn_thread_attribute_noargs_not_method : Warning<
> + "%0 attribute without arguments can only be applied to a method of a class">,
> + InGroup<ThreadSafetyAttributes>, DefaultIgnore;
> +def warn_thread_attribute_noargs_static_method : Warning<
> + "%0 attribute without arguments cannot be applied to a static method">,
> + InGroup<ThreadSafetyAttributes>, DefaultIgnore;
> def warn_thread_attribute_decl_not_pointer : Warning<
> "%0 only applies to pointer types; type here is %1">,
> InGroup<ThreadSafetyAttributes>, 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 <typename T> 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<CapabilityAttr>())
> + if (RD->hasAttr<T>())
> 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<RecordType>();
> - return Type->getDecl()->hasAttr<CapabilityAttr>();
> + return Type->getDecl()->hasAttr<T>();
> }, BPaths))
> return true;
> }
> return false;
> }
>
> -static bool checkTypedefTypeForCapability(QualType Ty) {
> +template <typename T> static bool checkTypedefTypeForAttr(QualType Ty) {
> const auto *TD = Ty->getAs<TypedefType>();
> if (!TD)
> return false;
> @@ -521,19 +521,27 @@ static bool checkTypedefTypeForCapabilit
> if (!TN)
> return false;
>
> - return TN->hasAttr<CapabilityAttr>();
> + return TN->hasAttr<T>();
> }
>
> -static bool typeHasCapability(Sema &S, QualType Ty) {
> - if (checkTypedefTypeForCapability(Ty))
> +template <typename T> static bool typeHasAttr(Sema &S, QualType Ty) {
> + if (checkTypedefTypeForAttr<T>(Ty))
> return true;
>
> - if (checkRecordTypeForCapability(S, Ty))
> + if (checkRecordTypeForAttr<T>(S, Ty))
> return true;
>
> return false;
> }
>
> +static bool typeHasCapability(Sema &S, QualType Ty) {
> + return typeHasAttr<CapabilityAttr>(S, Ty);
> +}
> +
> +static bool typeHasScopedLockable(Sema &S, QualType Ty) {
> + return typeHasAttr<ScopedLockableAttr>(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<Expr *> &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.
> - if(!RT && ParamIdxOk) {
> + if (!RT && ParamIdxOk) {
> FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
> IntegerLiteral *IL = dyn_cast<IntegerLiteral>(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 @@ static void checkAttrArgsAreCapabilityOb
>
> 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<CXXMethodDecl>(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();
> + }
> + }
> }
>
> //===----------------------------------------------------------------------===//
>
> Modified: cfe/trunk/test/Sema/attr-capabilities.c
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/attr-capabilities.c?rev=310403&r1=310402&r2=310403&view=diff
> ==============================================================================
> --- cfe/trunk/test/Sema/attr-capabilities.c (original)
> +++ cfe/trunk/test/Sema/attr-capabilities.c Tue Aug 8 12:44:35 2017
> @@ -37,15 +37,25 @@ void Func6(void) __attribute__((requires
> void Func7(void) __attribute__((assert_capability(GUI))) {}
> void Func8(void) __attribute__((assert_shared_capability(GUI))) {}
>
> +void Func9(void) __attribute__((assert_capability())) {} // expected-warning {{'assert_capability' attribute without arguments can only be applied to a method of a class}}
> +void Func10(void) __attribute__((assert_shared_capability())) {} // expected-warning {{'assert_shared_capability' attribute without arguments can only be applied to a method of a class}}
> +
> void Func11(void) __attribute__((acquire_capability(GUI))) {}
> void Func12(void) __attribute__((acquire_shared_capability(GUI))) {}
>
> +void Func13(void) __attribute__((acquire_capability())) {} // expected-warning {{'acquire_capability' attribute without arguments can only be applied to a method of a class}}
> +void Func14(void) __attribute__((acquire_shared_capability())) {} // expected-warning {{'acquire_shared_capability' attribute without arguments can only be applied to a method of a class}}
> +
> void Func15(void) __attribute__((release_capability(GUI))) {}
> void Func16(void) __attribute__((release_shared_capability(GUI))) {}
> void Func17(void) __attribute__((release_generic_capability(GUI))) {}
>
> -void Func21(void) __attribute__((try_acquire_capability(1))) {}
> -void Func22(void) __attribute__((try_acquire_shared_capability(1))) {}
> +void Func18(void) __attribute__((release_capability())) {} // expected-warning {{'release_capability' attribute without arguments can only be applied to a method of a class}}
> +void Func19(void) __attribute__((release_shared_capability())) {} // expected-warning {{'release_shared_capability' attribute without arguments can only be applied to a method of a class}}
> +void Func20(void) __attribute__((release_generic_capability())) {} // expected-warning {{'release_generic_capability' attribute without arguments can only be applied to a method of a class}}
> +
> +void Func21(void) __attribute__((try_acquire_capability(1))) {} // expected-warning {{'try_acquire_capability' attribute without arguments can only be applied to a method of a class}}
> +void Func22(void) __attribute__((try_acquire_shared_capability(1))) {} // expected-warning {{'try_acquire_shared_capability' attribute without arguments can only be applied to a method of a class}}
>
> void Func23(void) __attribute__((try_acquire_capability(1, GUI))) {}
> void Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {}
>
> Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-parsing.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-parsing.cpp?rev=310403&r1=310402&r2=310403&view=diff
> ==============================================================================
> --- cfe/trunk/test/SemaCXX/warn-thread-safety-parsing.cpp (original)
> +++ cfe/trunk/test/SemaCXX/warn-thread-safety-parsing.cpp Tue Aug 8 12:44:35 2017
> @@ -571,11 +571,11 @@ UnlockableMu ab_var_arg_bad_5 ACQUIRED_B
>
> // 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 @@ class ElfFoo {
> private:
> int test_field EXCLUSIVE_LOCK_FUNCTION(); // \
> // expected-warning {{'exclusive_lock_function' attribute only applies to functions}}
> - void test_method() EXCLUSIVE_LOCK_FUNCTION();
> + void test_method() EXCLUSIVE_LOCK_FUNCTION(); // expected-warning {{'exclusive_lock_function' attribute requires type annotated with 'capability' attribute; type here is 'ElfFoo *'}}
> };
>
> class EXCLUSIVE_LOCK_FUNCTION() ElfTestClass { // \
> @@ -643,11 +643,11 @@ int elf_function_bad_7() EXCLUSIVE_LOCK_
>
> // 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 @@ class SlfFoo {
> private:
> int test_field SHARED_LOCK_FUNCTION(); // \
> // expected-warning {{'shared_lock_function' attribute only applies to functions}}
> - void test_method() SHARED_LOCK_FUNCTION();
> + void test_method() SHARED_LOCK_FUNCTION(); // \
> + // expected-warning {{'shared_lock_function' attribute requires type annotated with 'capability' attribute; type here is 'SlfFoo *'}}
> };
>
> class SHARED_LOCK_FUNCTION() SlfTestClass { // \
> @@ -716,14 +717,16 @@ int slf_function_bad_7() SHARED_LOCK_FUN
> // 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; // \
> @@ -732,13 +735,14 @@ int etf_testfn(int y) {
> };
>
> 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_5() EXCLUSIVE_TRYLOCK_F
> int etf_function_6() EXCLUSIVE_TRYLOCK_FUNCTION(1, muRef);
> int etf_function_7() EXCLUSIVE_TRYLOCK_FUNCTION(1, muDoubleWrapper.getWrapper()->getMu());
> int etf_functetfn_8() EXCLUSIVE_TRYLOCK_FUNCTION(1, muPointer);
> -int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true);
> +int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true); // \
> + // expected-warning {{'exclusive_trylock_function' attribute without arguments can only be applied to a method of a class}}
>
>
> // illegal attribute arguments
> @@ -794,9 +799,11 @@ void stf_function() __attribute__((share
>
> void stf_function_args() SHARED_TRYLOCK_FUNCTION(1, mu2);
>
> -void stf_function_arg() SHARED_TRYLOCK_FUNCTION(1);
> +void stf_function_arg() SHARED_TRYLOCK_FUNCTION(1); // \
> + // expected-warning {{'shared_trylock_function' attribute without arguments can only be applied to a method of a class}}
>
> -int stf_testfn(int y) SHARED_TRYLOCK_FUNCTION(1);
> +int stf_testfn(int y) SHARED_TRYLOCK_FUNCTION(1); // \
> + // expected-warning {{'shared_trylock_function' attribute without arguments can only be applied to a method of a class}}
>
> int stf_testfn(int y) {
> int x SHARED_TRYLOCK_FUNCTION(1) = y; // \
> @@ -815,7 +822,8 @@ class StfFoo {
> private:
> int test_field SHARED_TRYLOCK_FUNCTION(1); // \
> // expected-warning {{'shared_trylock_function' attribute only applies to functions}}
> - void test_method() SHARED_TRYLOCK_FUNCTION(1);
> + void test_method() SHARED_TRYLOCK_FUNCTION(1); // \
> + // expected-warning {{'shared_trylock_function' attribute requires type annotated with 'capability' attribute; type here is 'StfFoo *'}}
> };
>
> class SHARED_TRYLOCK_FUNCTION(1) StfTestClass { // \
> @@ -833,7 +841,8 @@ int stf_function_5() SHARED_TRYLOCK_FUNC
> int stf_function_6() SHARED_TRYLOCK_FUNCTION(1, muRef);
> int stf_function_7() SHARED_TRYLOCK_FUNCTION(1, muDoubleWrapper.getWrapper()->getMu());
> int stf_function_8() SHARED_TRYLOCK_FUNCTION(1, muPointer);
> -int stf_function_9() SHARED_TRYLOCK_FUNCTION(true);
> +int stf_function_9() SHARED_TRYLOCK_FUNCTION(true); // \
> + // expected-warning {{'shared_trylock_function' attribute without arguments can only be applied to a method of a class}}
>
>
> // illegal attribute arguments
> @@ -862,11 +871,14 @@ int stf_function_bad_6() SHARED_TRYLOCK_
>
> // takes zero or more arguments, all locks (vars/fields)
>
> -void uf_function() UNLOCK_FUNCTION();
> +void uf_function() UNLOCK_FUNCTION(); // \
> + // expected-warning {{'unlock_function' attribute without arguments can only be applied to a method of a class}}
> +
>
> void uf_function_args() UNLOCK_FUNCTION(mu1, mu2);
>
> -int uf_testfn(int y) UNLOCK_FUNCTION();
> +int uf_testfn(int y) UNLOCK_FUNCTION(); //\
> + // expected-warning {{'unlock_function' attribute without arguments can only be applied to a method of a class}}
>
> int uf_testfn(int y) {
> int x UNLOCK_FUNCTION() = y; // \
> @@ -881,7 +893,8 @@ class UfFoo {
> private:
> int test_field UNLOCK_FUNCTION(); // \
> // expected-warning {{'unlock_function' attribute only applies to functions}}
> - void test_method() UNLOCK_FUNCTION();
> + void test_method() UNLOCK_FUNCTION(); // \
> + // expected-warning {{'unlock_function' attribute requires type annotated with 'capability' attribute; type here is 'UfFoo *'}}
> };
>
> class NO_THREAD_SAFETY_ANALYSIS UfTestClass { // \
> @@ -1524,4 +1537,4 @@ namespace CRASH_POST_R301735 {
> Mutex mu_;
> };
> }
> -#endif
> \ No newline at end of file
> +#endif
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list