Author: Dan McArdle Date: 2024-06-24T09:29:13-04:00 New Revision: c1bde0a2cb640b3607e9568b9a57b292e1f82666
URL: https://github.com/llvm/llvm-project/commit/c1bde0a2cb640b3607e9568b9a57b292e1f82666 DIFF: https://github.com/llvm/llvm-project/commit/c1bde0a2cb640b3607e9568b9a57b292e1f82666.diff LOG: [clang][ThreadSafety] Check trylock function success and return types (#95290) With this change, Clang will generate errors when trylock functions have improper return types. Today, it silently fails to apply the trylock attribute to these functions which may incorrectly lead users to believe they have correctly acquired locks before accessing guarded data. As a side effect of explicitly checking the success argument type, I seem to have fixed a false negative in the analysis that could occur when a trylock's success argument is an enumerator. I've added a regression test to warn-thread-safety-analysis.cpp named `TrylockSuccessEnumFalseNegative`. This change also improves the documentation with descriptions of of the subtle gotchas that arise from the analysis interpreting the success arg as a boolean. Issue #92408 Added: Modified: clang/docs/ReleaseNotes.rst clang/docs/ThreadSafetyAnalysis.rst clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Sema/ParsedAttr.h clang/lib/Analysis/ThreadSafety.cpp clang/lib/Sema/SemaDeclAttr.cpp clang/test/Sema/attr-capabilities.c clang/test/SemaCXX/warn-thread-safety-analysis.cpp clang/test/SemaCXX/warn-thread-safety-parsing.cpp clang/unittests/AST/ASTImporterTest.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 136c72cf682a9..df579ae398c5e 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -71,6 +71,11 @@ C++ Specific Potentially Breaking Changes To fix this, update libstdc++ to version 14.1.1 or greater. +- Clang now emits errors when Thread Safety Analysis trylock attributes are + applied to functions or methods with incompatible return values, such as + constructors, destructors, and void-returning functions. This only affects the + ``TRY_ACQUIRE`` and ``TRY_ACQUIRE_SHARED`` attributes (and any synonyms). + ABI Changes in This Version --------------------------- - Fixed Microsoft name mangling of implicitly defined variables used for thread @@ -729,6 +734,11 @@ Bug Fixes in This Version - Fixed `static_cast` to array of unknown bound. Fixes (#GH62863). +- Clang's Thread Safety Analysis now evaluates trylock success arguments of enum + types rather than silently defaulting to false. This fixes a class of false + negatives where the analysis failed to detect unchecked access to guarded + data. + Bug Fixes to Compiler Builtins ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst index dcde0c706c704..0ecbebe7a692f 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/clang/docs/ThreadSafetyAnalysis.rst @@ -420,10 +420,17 @@ TRY_ACQUIRE(<bool>, ...), TRY_ACQUIRE_SHARED(<bool>, ...) *Previously:* ``EXCLUSIVE_TRYLOCK_FUNCTION``, ``SHARED_TRYLOCK_FUNCTION`` These are attributes on a function or method that tries to acquire the given -capability, and returns a boolean value indicating success or failure. -The first argument must be ``true`` or ``false``, to specify which return value -indicates success, and the remaining arguments are interpreted in the same way -as ``ACQUIRE``. See :ref:`mutexheader`, below, for example uses. +capability, and returns a boolean, integer, or pointer value indicating success +or failure. + +The attribute's first argument defines whether a zero or non-zero return value +indicates success. Syntactically, it accepts ``NULL`` or ``nullptr``, ``bool`` +and ``int`` literals, as well as enumerator values. *The analysis only cares +whether this success value is zero or non-zero.* This leads to some subtle +consequences, discussed in the next section. + +The remaining arguments are interpreted in the same way as ``ACQUIRE``. See +:ref:`mutexheader`, below, for example uses. Because the analysis doesn't support conditional locking, a capability is treated as acquired after the first branch on the return value of a try-acquire @@ -445,6 +452,43 @@ function. } } +Subtle Consequences of Non-Boolean Success Values +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The trylock attributes accept non-boolean expressions for the success value, but +the analysis only cares whether the value is zero or non-zero. + +Suppose you define an enum with two non-zero enumerators: ``LockAcquired = 1`` +and ``LockNotAcquired = 2``. If your trylock function returns ``LockAcquired`` +on success and ``LockNotAcquired`` on failure, the analysis may fail to detect +access to guarded data without holding the mutex because they are both non-zero. + +.. code-block:: c++ + // *** Beware: this code demonstrates incorrect usage. *** + + enum TrylockResult { LockAcquired = 1, LockNotAcquired = 2 }; + + class CAPABILITY("mutex") Mutex { + public: + TrylockResult TryLock() TRY_ACQUIRE(LockAcquired); + }; + + Mutex mu; + int a GUARDED_BY(mu); + + void foo() { + if (mu.TryLock()) { // This branch satisfies the analysis, but permits + // unguarded access to `a`! + a = 0; + mu.Unlock(); + } + } + +It's also possible to return a pointer from the trylock function. Similarly, all +that matters is whether the success value is zero or non-zero. For instance, a +success value of `true` means the function returns a non-null pointer on +success. + ASSERT_CAPABILITY(...) and ASSERT_SHARED_CAPABILITY(...) -------------------------------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 134a4e4d547dd..46ad359751d7d 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3272,11 +3272,23 @@ def warn_aligned_attr_underaligned : Warning<err_alignas_underaligned.Summary>, def err_attribute_sizeless_type : Error< "%0 attribute cannot be applied to sizeless type %1">; def err_attribute_argument_n_type : Error< - "%0 attribute requires parameter %1 to be %select{int or bool|an integer " - "constant|a string|an identifier|a constant expression|a builtin function}2">; + "%0 attribute requires parameter %1 to be %select{" + "int or bool" + "|an integer constant" + "|a string" + "|an identifier" + "|a constant expression" + "|a builtin function" + "|nullptr or a bool, int, or enum literal}2">; def err_attribute_argument_type : Error< - "%0 attribute requires %select{int or bool|an integer " - "constant|a string|an identifier}1">; + "%0 attribute requires %select{" + "int or bool" + "|an integer constant" + "|a string" + "|an identifier" + "|a constant expression" + "|a builtin function" + "|a pointer or a bool, int, or enum literal}1">; def err_attribute_argument_out_of_range : Error< "%0 attribute requires integer constant between %1 and %2 inclusive">; def err_init_priority_object_attr : Error< @@ -3728,7 +3740,8 @@ def warn_attribute_wrong_decl_type : Warning< "|types and namespaces" "|variables, functions and classes" "|kernel functions" - "|non-K&R-style functions}2">, + "|non-K&R-style functions" + "|functions that return bool, integer, or a pointer type}2">, InGroup<IgnoredAttributes>; def err_attribute_wrong_decl_type : Error<warn_attribute_wrong_decl_type.Summary>; def warn_type_attribute_wrong_type : Warning< diff --git a/clang/include/clang/Sema/ParsedAttr.h b/clang/include/clang/Sema/ParsedAttr.h index 22cbd0d90ee43..65d73f6cd44f6 100644 --- a/clang/include/clang/Sema/ParsedAttr.h +++ b/clang/include/clang/Sema/ParsedAttr.h @@ -1083,6 +1083,7 @@ enum AttributeArgumentNType { AANT_ArgumentIdentifier, AANT_ArgumentConstantExpr, AANT_ArgumentBuiltinFunction, + AANT_ArgumentNullptrOrBoolIntOrEnumLiteral, }; /// These constants match the enumerated choices of @@ -1101,6 +1102,7 @@ enum AttributeDeclKind { ExpectedFunctionVariableOrClass, ExpectedKernelFunction, ExpectedFunctionWithProtoType, + ExpectedFunctionReturningPointerBoolIntOrEnum, }; inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB, diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index e25b843c9bf83..550c2a3ffe522 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -37,6 +37,7 @@ #include "clang/Basic/OperatorKinds.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/Specifiers.h" +#include "llvm/ADT/APSInt.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/ImmutableMap.h" @@ -1034,9 +1035,10 @@ class ThreadSafetyAnalyzer { template <class AttrType> void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, - const NamedDecl *D, - const CFGBlock *PredBlock, const CFGBlock *CurrBlock, - Expr *BrE, bool Neg); + const NamedDecl *D, const CFGBlock *PredBlock, + const CFGBlock *CurrBlock, + const ASTContext &TrylockAttrContext, + Expr *TrylockAttrSuccessValue, bool Neg); const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C, bool &Negate); @@ -1359,17 +1361,18 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, const NamedDecl *D, const CFGBlock *PredBlock, const CFGBlock *CurrBlock, - Expr *BrE, bool Neg) { - // Find out which branch has the lock - bool branch = false; - if (const auto *BLE = dyn_cast_or_null<CXXBoolLiteralExpr>(BrE)) - branch = BLE->getValue(); - else if (const auto *ILE = dyn_cast_or_null<IntegerLiteral>(BrE)) - branch = ILE->getValue().getBoolValue(); - - int branchnum = branch ? 0 : 1; - if (Neg) - branchnum = !branchnum; + const ASTContext &TrylockAttrContext, + Expr *TrylockAttrSuccess, + bool Neg) { + // Evaluate the trylock's success value as a boolean. + bool trylockSuccessValue = false; + if (!TrylockAttrSuccess->EvaluateAsBooleanCondition( + trylockSuccessValue, TrylockAttrContext, + /*InConstantContext=*/true)) { + llvm_unreachable("Trylock success value could not be evaluated."); + } + + const int branchnum = Neg ? !!trylockSuccessValue : !trylockSuccessValue; // If we've taken the trylock branch, then add the lock int i = 0; @@ -1390,8 +1393,15 @@ static bool getStaticBooleanValue(Expr *E, bool &TCond) { } else if (const auto *ILE = dyn_cast<IntegerLiteral>(E)) { TCond = ILE->getValue().getBoolValue(); return true; - } else if (auto *CE = dyn_cast<ImplicitCastExpr>(E)) + } else if (auto *CE = dyn_cast<ImplicitCastExpr>(E)) { return getStaticBooleanValue(CE->getSubExpr(), TCond); + } else if (auto *DRE = dyn_cast<DeclRefExpr>(E)) { + if (auto *ECD = dyn_cast<EnumConstantDecl>(DRE->getDecl())) { + llvm::APSInt IV = ECD->getInitVal(); + TCond = IV.getBoolValue(); + return true; + } + } return false; } @@ -1497,27 +1507,27 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result, // If the condition is a call to a Trylock function, then grab the attributes for (const auto *Attr : FunDecl->attrs()) { switch (Attr->getKind()) { - case attr::TryAcquireCapability: { - auto *A = cast<TryAcquireCapabilityAttr>(Attr); - getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, - Exp, FunDecl, PredBlock, CurrBlock, A->getSuccessValue(), - Negate); - break; - }; - case attr::ExclusiveTrylockFunction: { - const auto *A = cast<ExclusiveTrylockFunctionAttr>(Attr); - getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, - A->getSuccessValue(), Negate); - break; - } - case attr::SharedTrylockFunction: { - const auto *A = cast<SharedTrylockFunctionAttr>(Attr); - getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, - A->getSuccessValue(), Negate); - break; - } - default: - break; + case attr::TryAcquireCapability: { + auto *A = cast<TryAcquireCapabilityAttr>(Attr); + getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A, + Exp, FunDecl, PredBlock, CurrBlock, FunDecl->getASTContext(), + A->getSuccessValue(), Negate); + break; + }; + case attr::ExclusiveTrylockFunction: { + const auto *A = cast<ExclusiveTrylockFunctionAttr>(Attr); + getMutexIDs(ExclusiveLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, + FunDecl->getASTContext(), A->getSuccessValue(), Negate); + break; + } + case attr::SharedTrylockFunction: { + const auto *A = cast<SharedTrylockFunctionAttr>(Attr); + getMutexIDs(SharedLocksToAdd, A, Exp, FunDecl, PredBlock, CurrBlock, + FunDecl->getASTContext(), A->getSuccessValue(), Negate); + break; + } + default: + break; } } diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index ce6b5b1ff6f93..99b400307d644 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -14,6 +14,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/ASTMutationListener.h" #include "clang/AST/CXXInheritance.h" +#include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" @@ -25,6 +26,7 @@ #include "clang/Basic/CharInfo.h" #include "clang/Basic/Cuda.h" #include "clang/Basic/DarwinSDKInfo.h" +#include "clang/Basic/DiagnosticSema.h" #include "clang/Basic/HLSLRuntime.h" #include "clang/Basic/IdentifierTable.h" #include "clang/Basic/LangOptions.h" @@ -65,10 +67,13 @@ #include "llvm/Demangle/Demangle.h" #include "llvm/IR/Assumptions.h" #include "llvm/MC/MCSectionMachO.h" +#include "llvm/Support/Casting.h" #include "llvm/Support/Error.h" +#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/MathExtras.h" #include "llvm/Support/raw_ostream.h" #include "llvm/TargetParser/Triple.h" +#include <cassert> #include <optional> using namespace clang; @@ -166,13 +171,6 @@ bool Sema::checkStringLiteralArgumentAttr(const ParsedAttr &AL, unsigned ArgNum, return checkStringLiteralArgumentAttr(AL, ArgExpr, Str, ArgLocation); } -/// Check if the passed-in expression is of type int or bool. -static bool isIntOrBool(Expr *Exp) { - QualType QT = Exp->getType(); - return QT->isBooleanType() || QT->isIntegerType(); -} - - // Check to see if the type is a smart pointer of some kind. We assume // it's a smart pointer if it defines both operator-> and operator*. static bool threadSafetyCheckIsSmartPointer(Sema &S, const RecordType* RT) { @@ -608,15 +606,31 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL, if (!AL.checkAtLeastNumArgs(S, 1)) return false; - if (!isIntOrBool(AL.getArgAsExpr(0))) { + // The attribute's first argument defines the success value. + const Expr *SuccessArg = AL.getArgAsExpr(0); + if (!isa<CXXNullPtrLiteralExpr>(SuccessArg) && + !isa<GNUNullExpr>(SuccessArg) && !isa<CXXBoolLiteralExpr>(SuccessArg) && + !isa<IntegerLiteral>(SuccessArg) && !SuccessArg->getEnumConstantDecl()) { S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type) - << AL << 1 << AANT_ArgumentIntOrBool; + << AL << 1 << AANT_ArgumentNullptrOrBoolIntOrEnumLiteral; return false; } - // check that all arguments are lockable objects + // All remaining arguments must be lockable objects. checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 1); + // The function must return a pointer, boolean, integer, or enum. We already + // know that `D` is a function because `ExclusiveTrylockFunction` and friends + // are defined in Attr.td with subject lists that only include functions. + QualType ReturnType = D->getAsFunction()->getReturnType(); + if (!ReturnType->isPointerType() && !ReturnType->isBooleanType() && + !ReturnType->isIntegerType() && !ReturnType->isEnumeralType()) { + S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type) + << AL << AL.isRegularKeywordAttribute() + << ExpectedFunctionReturningPointerBoolIntOrEnum; + return false; + } + return true; } diff --git a/clang/test/Sema/attr-capabilities.c b/clang/test/Sema/attr-capabilities.c index 5138803bd5eb7..91a43c79d5b91 100644 --- a/clang/test/Sema/attr-capabilities.c +++ b/clang/test/Sema/attr-capabilities.c @@ -54,14 +54,14 @@ void Func18(void) __attribute__((release_capability())) {} // expected-warning { void Func19(void) __attribute__((release_shared_capability())) {} // expected-warning {{'release_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}} void Func20(void) __attribute__((release_generic_capability())) {} // expected-warning {{'release_generic_capability' attribute without capability arguments can only be applied to non-static methods of a class}} -void Func21(void) __attribute__((try_acquire_capability(1))) {} // expected-warning {{'try_acquire_capability' attribute without capability arguments can only be applied to non-static methods of a class}} -void Func22(void) __attribute__((try_acquire_shared_capability(1))) {} // expected-warning {{'try_acquire_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}} +int Func21(void) __attribute__((try_acquire_capability(1))) {} // expected-warning {{'try_acquire_capability' attribute without capability arguments can only be applied to non-static methods of a class}} +int Func22(void) __attribute__((try_acquire_shared_capability(1))) {} // expected-warning {{'try_acquire_shared_capability' attribute without capability arguments can only be applied to non-static methods of a class}} -void Func23(void) __attribute__((try_acquire_capability(1, GUI))) {} -void Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {} +int Func23(void) __attribute__((try_acquire_capability(1, GUI))) {} +int Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {} -void Func25(void) __attribute__((try_acquire_capability())) {} // expected-error {{'try_acquire_capability' attribute takes at least 1 argument}} -void Func26(void) __attribute__((try_acquire_shared_capability())) {} // expected-error {{'try_acquire_shared_capability' attribute takes at least 1 argument}} +int Func25(void) __attribute__((try_acquire_capability())) {} // expected-error {{'try_acquire_capability' attribute takes at least 1 argument}} +int Func26(void) __attribute__((try_acquire_shared_capability())) {} // expected-error {{'try_acquire_shared_capability' attribute takes at least 1 argument}} // Test that boolean logic works with capability attributes void Func27(void) __attribute__((requires_capability(!GUI))); diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 73cc946ca0ce1..a6ddf028e1fad 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1954,6 +1954,125 @@ struct TestTryLock { } // end namespace TrylockTest +// Regression test for trylock attributes with an enumerator success argument. +// Prior versions of the analysis silently failed to evaluate success arguments +// that were neither `CXXBoolLiteralExpr` nor `IntegerLiteral` and assumed the +// value was false. +namespace TrylockSuccessEnumFalseNegative { + +enum TrylockResult { Failure = 0, Success = 1 }; + +class LOCKABLE Mutex { +public: + TrylockResult TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(Success); + void Unlock() EXCLUSIVE_UNLOCK_FUNCTION(); +}; + +class TrylockTest { + Mutex mu_; + int a_ GUARDED_BY(mu_) = 0; + + void test_bool_expression() { + if (!mu_.TryLock()) { // expected-note {{mutex acquired here}} + a_++; // expected-warning {{writing variable 'a_' requires holding mutex 'mu_' exclusively}} + mu_.Unlock(); // expected-warning {{releasing mutex 'mu_' that was not held}} + } + } // expected-warning {{mutex 'mu_' is not held on every path through here}} +}; +} // namespace TrylockSuccessEnumFalseNegative + +// This test demonstrates that the analysis does not distinguish between +// diff erent non-zero enumerators. +namespace TrylockFalseNegativeWhenEnumHasMultipleFailures { + +enum TrylockResult { Failure1 = 0, Failure2, Success }; + +class LOCKABLE Mutex { +public: + TrylockResult TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(Success); + void Unlock() EXCLUSIVE_UNLOCK_FUNCTION(); +}; + +class TrylockTest { + Mutex mu_; + int a_ GUARDED_BY(mu_) = 0; + + void test_eq_success() { + if (mu_.TryLock() == Success) { + a_++; + mu_.Unlock(); + } + } + + void test_bool_expression() { + // This branch checks whether the trylock function returned a non-zero + // value. This satisfies the analysis, but fails to account for `Failure2`. + // From the analysis's perspective, `Failure2` and `Success` are equivalent! + if (mu_.TryLock()) { + a_++; + mu_.Unlock(); + } + } +}; +} // namespace TrylockSuccessEnumMultipleFailureModesFalseNegative + + +// This test demonstrates that the analysis does not detect when all enumerators +// are positive, and thus incapable of representing a failure. +namespace TrylockSuccessEnumNoZeroFalseNegative { + +enum TrylockResult { Failure = 1, Success = 2 }; + +class LOCKABLE Mutex { +public: + TrylockResult TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(Success); + void Unlock() EXCLUSIVE_UNLOCK_FUNCTION(); +}; + +class TrylockTest { + Mutex mu_; + int a_ GUARDED_BY(mu_) = 0; + + void test_bool_expression() { + // This branch checks whether the trylock function returned a non-zero value + // This satisfies the analysis, but is actually incorrect because `Failure` + // and `Success` are both non-zero. + if (mu_.TryLock()) { + a_++; + mu_.Unlock(); + } + } +}; +} // namespace TrylockSuccessEnumNoZeroFalseNegative + + +// Demonstrate a mutex with a trylock function that conditionally vends a +// pointer to guarded data. +namespace TrylockFunctionReturnPointer { + +class LOCKABLE OwningMutex { +public: + int *TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true); + void Unlock() EXCLUSIVE_UNLOCK_FUNCTION(); + +private: + int guarded_ GUARDED_BY(this) = 0; +}; + +class TrylockTest { + OwningMutex mu_; + + void test_ptr_return() { + if (int *p = mu_.TryLock()) { + *p = 0; + mu_.Unlock(); + } + } +}; + +} // namespace TrylockFunctionReturnPointer + + namespace TestTemplateAttributeInstantiation { class Foo1 { @@ -3657,8 +3776,8 @@ class Foo { int a GUARDED_BY(mu_); bool c; - int tryLockMutexI() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_); - Mutex* tryLockMutexP() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_); + int tryLockMutexI() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_); + Mutex *tryLockMutexP() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu_); void unlock() UNLOCK_FUNCTION(mu_); void test1(); diff --git a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp index 0c5b0cc85897b..4921a75b84e39 100644 --- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp @@ -716,15 +716,17 @@ int slf_function_bad_7() SHARED_LOCK_FUNCTION(0); // \ #error "Should support exclusive_trylock_function attribute" #endif -// takes a mandatory boolean or integer argument specifying the retval -// plus an optional list of locks (vars/fields) +// The attribute takes a mandatory boolean or integer argument specifying the +// retval plus an optional list of locks (vars/fields). The annotated function's +// return type should be boolean or integer. 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_args() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2); // \ + // expected-error {{'exclusive_trylock_function' attribute only applies to functions that return bool, integer, or a pointer type}} -void etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \ +int etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \ // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}} int etf_testfn(int y) EXCLUSIVE_TRYLOCK_FUNCTION(1); // \ @@ -743,10 +745,23 @@ 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); // \ + bool test_method() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \ // expected-warning {{'exclusive_trylock_function' attribute without capability arguments refers to 'this', but 'EtfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}} }; +// It does not make sense to annotate constructors/destructors as exclusive +// trylock functions because they cannot return a value. See +// <https://github.com/llvm/llvm-project/issues/92408>. +class EtfConstructorDestructor { + EtfConstructorDestructor() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu); // \ + // expected-error {{'exclusive_trylock_function' attribute only applies to functions that return bool, integer, or a pointer type}} + + ~EtfConstructorDestructor() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu); // \ + // expected-error {{'exclusive_trylock_function' attribute only applies to functions that return bool, integer, or a pointer type}} + + Mutex mu; +}; + class EXCLUSIVE_TRYLOCK_FUNCTION(1) EtfTestClass { // \ // expected-warning {{'exclusive_trylock_function' attribute only applies to functions}} }; @@ -756,7 +771,68 @@ void etf_fun_params(int lvar EXCLUSIVE_TRYLOCK_FUNCTION(1)); // \ // Check argument parsing. -// legal attribute arguments +int global_int = 0; +int* etf_fun_with_global_ptr_success() EXCLUSIVE_TRYLOCK_FUNCTION(&global_int, &mu1); //\ + // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} + +#ifdef CPP11 +constexpr int kTrylockSuccess = 1; +int etf_succ_constexpr() EXCLUSIVE_TRYLOCK_FUNCTION(kTrylockSuccess, mu2); // \ + // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} + +int success_value_non_constant = 1; + +bool etf_succ_variable() EXCLUSIVE_TRYLOCK_FUNCTION(success_value_non_constant, mu2); //\ + // expected-error {{attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} +#endif + + +// Legal permutations of the first argument and function return type. +struct TrylockResult; + +#ifdef CPP11 +int* etf_fun_with_nullptr_success() EXCLUSIVE_TRYLOCK_FUNCTION(nullptr, &mu1); +#endif + +#ifndef __cplusplus +int* etf_fun_with_nullptr_success() EXCLUSIVE_TRYLOCK_FUNCTION(NULL, &mu1); +#endif + +int etf_succ_1_ret_i() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2); +bool etf_succ_1_ret_b() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2); +TrylockResult *etf_succ_1_ret_p() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2); + +int etf_succ_true_ret_i() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu2); +bool etf_succ_true_ret_b() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu2); +TrylockResult *etf_succ_true_ret_p() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu2); + +int etf_succ_false_ret_i() EXCLUSIVE_TRYLOCK_FUNCTION(false, mu2); +bool etf_succ_false_ret_b() EXCLUSIVE_TRYLOCK_FUNCTION(false, mu2); +TrylockResult *etf_succ_false_ret_p() EXCLUSIVE_TRYLOCK_FUNCTION(false, mu2); + +enum TrylockSuccessEnum { TrylockNotAcquired = 0, TrylockAcquired }; +int etf_succ_enum_ret_i() EXCLUSIVE_TRYLOCK_FUNCTION(TrylockAcquired, mu2); +bool etf_succ_enum_ret_b() EXCLUSIVE_TRYLOCK_FUNCTION(TrylockAcquired, mu2); +TrylockResult *etf_succ_enum_ret_p() + EXCLUSIVE_TRYLOCK_FUNCTION(TrylockAcquired, mu2); + +#ifdef CPP11 +enum class TrylockSuccessEnumClass { NotAcquired = 0, Acquired}; +int etf_succ_enum_class_ret_i() + EXCLUSIVE_TRYLOCK_FUNCTION(TrylockSuccessEnumClass::Acquired, mu2); +bool etf_succ_enum_class_ret_b() + EXCLUSIVE_TRYLOCK_FUNCTION(TrylockSuccessEnumClass::Acquired, mu2); +TrylockResult *etf_succ_enum_class_ret_p() + EXCLUSIVE_TRYLOCK_FUNCTION(TrylockSuccessEnumClass::Acquired, mu2); +#endif + +// Demonstrate that we do not detect enum type mismatches between the success +// argument and the function's return type. +enum SomeOtherEnum { Foo = 1 }; +TrylockSuccessEnum etf_succ_enum_mismatch() + EXCLUSIVE_TRYLOCK_FUNCTION(Foo, mu2); + +// legal capability attribute arguments int etf_function_1() EXCLUSIVE_TRYLOCK_FUNCTION(1, muWrapper.mu); int etf_function_2() EXCLUSIVE_TRYLOCK_FUNCTION(1, muDoubleWrapper.muWrapper->mu); int etf_function_3() EXCLUSIVE_TRYLOCK_FUNCTION(1, muWrapper.getMu()); @@ -768,14 +844,13 @@ int etf_functetfn_8() EXCLUSIVE_TRYLOCK_FUNCTION(1, muPointer); int etf_function_9() EXCLUSIVE_TRYLOCK_FUNCTION(true); // \ // expected-warning {{'exclusive_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}} - // illegal attribute arguments int etf_function_bad_1() EXCLUSIVE_TRYLOCK_FUNCTION(mu1); // \ - // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be int or bool}} + // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} int etf_function_bad_2() EXCLUSIVE_TRYLOCK_FUNCTION("mu"); // \ - // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be int or bool}} + // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} int etf_function_bad_3() EXCLUSIVE_TRYLOCK_FUNCTION(muDoublePointer); // \ - // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be int or bool}} + // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} int etf_function_bad_4() EXCLUSIVE_TRYLOCK_FUNCTION(1, "mu"); // \ // expected-warning {{ignoring 'exclusive_trylock_function' attribute because its argument is invalid}} @@ -799,9 +874,9 @@ int etf_function_bad_6() EXCLUSIVE_TRYLOCK_FUNCTION(1, umu); // \ void stf_function() __attribute__((shared_trylock_function)); // \ // expected-error {{'shared_trylock_function' attribute takes at least 1 argument}} -void stf_function_args() SHARED_TRYLOCK_FUNCTION(1, mu2); +bool stf_function_args() SHARED_TRYLOCK_FUNCTION(1, mu2); -void stf_function_arg() SHARED_TRYLOCK_FUNCTION(1); // \ +bool stf_function_arg() SHARED_TRYLOCK_FUNCTION(1); // \ // expected-warning {{'shared_trylock_function' attribute without capability arguments can only be applied to non-static methods of a class}} int stf_testfn(int y) SHARED_TRYLOCK_FUNCTION(1); // \ @@ -824,7 +899,7 @@ 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); // \ + bool test_method() SHARED_TRYLOCK_FUNCTION(1); // \ // expected-warning {{'shared_trylock_function' attribute without capability arguments refers to 'this', but 'StfFoo' isn't annotated with 'capability' or 'scoped_lockable' attribute}} }; @@ -849,11 +924,11 @@ int stf_function_9() SHARED_TRYLOCK_FUNCTION(true); // \ // illegal attribute arguments int stf_function_bad_1() SHARED_TRYLOCK_FUNCTION(mu1); // \ - // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be int or bool}} + // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} int stf_function_bad_2() SHARED_TRYLOCK_FUNCTION("mu"); // \ - // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be int or bool}} + // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} int stf_function_bad_3() SHARED_TRYLOCK_FUNCTION(muDoublePointer); // \ - // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be int or bool}} + // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}} int stf_function_bad_4() SHARED_TRYLOCK_FUNCTION(1, "mu"); // \ // expected-warning {{ignoring 'shared_trylock_function' attribute because its argument is invalid}} diff --git a/clang/unittests/AST/ASTImporterTest.cpp b/clang/unittests/AST/ASTImporterTest.cpp index 92f9bae6cb064..4c33546de6f92 100644 --- a/clang/unittests/AST/ASTImporterTest.cpp +++ b/clang/unittests/AST/ASTImporterTest.cpp @@ -7851,7 +7851,7 @@ TEST_P(ImportAttributes, ImportAcquireCapability) { TEST_P(ImportAttributes, ImportTryAcquireCapability) { TryAcquireCapabilityAttr *FromAttr, *ToAttr; importAttr<FunctionDecl>( - "void test(int A1, int A2) __attribute__((try_acquire_capability(1, A1, " + "bool test(int A1, int A2) __attribute__((try_acquire_capability(1, A1, " "A2)));", FromAttr, ToAttr); checkImported(FromAttr->getSuccessValue(), ToAttr->getSuccessValue()); @@ -7948,7 +7948,7 @@ TEST_P(ImportAttributes, ImportAssertSharedLock) { TEST_P(ImportAttributes, ImportExclusiveTrylockFunction) { ExclusiveTrylockFunctionAttr *FromAttr, *ToAttr; importAttr<FunctionDecl>( - "void test(int A1, int A2) __attribute__((exclusive_trylock_function(1, " + "bool test(int A1, int A2) __attribute__((exclusive_trylock_function(1, " "A1, A2)));", FromAttr, ToAttr); checkImported(FromAttr->getSuccessValue(), ToAttr->getSuccessValue()); @@ -7958,7 +7958,7 @@ TEST_P(ImportAttributes, ImportExclusiveTrylockFunction) { TEST_P(ImportAttributes, ImportSharedTrylockFunction) { SharedTrylockFunctionAttr *FromAttr, *ToAttr; importAttr<FunctionDecl>( - "void test(int A1, int A2) __attribute__((shared_trylock_function(1, A1, " + "bool test(int A1, int A2) __attribute__((shared_trylock_function(1, A1, " "A2)));", FromAttr, ToAttr); checkImported(FromAttr->getSuccessValue(), ToAttr->getSuccessValue()); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
