llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Dan McArdle (dmcardle) <details> <summary>Changes</summary> This PR reverts #<!-- -->95290 and the one-liner followup PR #<!-- -->96494. I received some substantial feedback on #<!-- -->95290, which I plan to address in a future PR. I've also received feedback that because the change emits errors where they were not emitted before, we should at least have a flag to disable the stricter warnings. --- Patch is 32.75 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/97293.diff 10 Files Affected: - (modified) clang/docs/ReleaseNotes.rst (-10) - (modified) clang/docs/ThreadSafetyAnalysis.rst (+4-49) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+5-18) - (modified) clang/include/clang/Sema/ParsedAttr.h (-2) - (modified) clang/lib/Analysis/ThreadSafety.cpp (+36-46) - (modified) clang/lib/Sema/SemaDeclAttr.cpp (+10-24) - (modified) clang/test/Sema/attr-capabilities.c (+6-6) - (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+2-121) - (modified) clang/test/SemaCXX/warn-thread-safety-parsing.cpp (+16-91) - (modified) clang/unittests/AST/ASTImporterTest.cpp (+3-3) ``````````diff diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index b7a2d97f00087..c7bb25d611971 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -71,11 +71,6 @@ 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 @@ -751,11 +746,6 @@ 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 1513719caa464..dcde0c706c704 100644 --- a/clang/docs/ThreadSafetyAnalysis.rst +++ b/clang/docs/ThreadSafetyAnalysis.rst @@ -420,17 +420,10 @@ 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, 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. +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. 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 @@ -452,44 +445,6 @@ 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 5dc36c594bcb7..ffd3d81e588b4 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3282,23 +3282,11 @@ 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" - "|nullptr or a bool, int, or enum literal}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}2">; def err_attribute_argument_type : Error< - "%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">; + "%0 attribute requires %select{int or bool|an integer " + "constant|a string|an identifier}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< @@ -3750,8 +3738,7 @@ def warn_attribute_wrong_decl_type : Warning< "|types and namespaces" "|variables, functions and classes" "|kernel functions" - "|non-K&R-style functions" - "|functions that return bool, integer, or a pointer type}2">, + "|non-K&R-style functions}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 65d73f6cd44f6..22cbd0d90ee43 100644 --- a/clang/include/clang/Sema/ParsedAttr.h +++ b/clang/include/clang/Sema/ParsedAttr.h @@ -1083,7 +1083,6 @@ enum AttributeArgumentNType { AANT_ArgumentIdentifier, AANT_ArgumentConstantExpr, AANT_ArgumentBuiltinFunction, - AANT_ArgumentNullptrOrBoolIntOrEnumLiteral, }; /// These constants match the enumerated choices of @@ -1102,7 +1101,6 @@ 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 550c2a3ffe522..e25b843c9bf83 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -37,7 +37,6 @@ #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" @@ -1035,10 +1034,9 @@ class ThreadSafetyAnalyzer { template <class AttrType> void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, - const NamedDecl *D, const CFGBlock *PredBlock, - const CFGBlock *CurrBlock, - const ASTContext &TrylockAttrContext, - Expr *TrylockAttrSuccessValue, bool Neg); + const NamedDecl *D, + const CFGBlock *PredBlock, const CFGBlock *CurrBlock, + Expr *BrE, bool Neg); const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C, bool &Negate); @@ -1361,18 +1359,17 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp, const NamedDecl *D, const CFGBlock *PredBlock, const CFGBlock *CurrBlock, - 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; + 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; // If we've taken the trylock branch, then add the lock int i = 0; @@ -1393,15 +1390,8 @@ 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; } @@ -1507,27 +1497,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, 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; + 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; } } diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 41489789919d0..b8842e9003e10 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -14,7 +14,6 @@ #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" @@ -26,7 +25,6 @@ #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" @@ -67,13 +65,10 @@ #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; @@ -171,6 +166,13 @@ 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) { @@ -606,31 +608,15 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL, if (!AL.checkAtLeastNumArgs(S, 1)) return false; - // 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()) { + if (!isIntOrBool(AL.getArgAsExpr(0))) { S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type) - << AL << 1 << AANT_ArgumentNullptrOrBoolIntOrEnumLiteral; + << AL << 1 << AANT_ArgumentIntOrBool; return false; } - // All remaining arguments must be lockable objects. + // check that all arguments are 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 91a43c79d5b91..5138803bd5eb7 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}} -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 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 Func23(void) __attribute__((try_acquire_capability(1, GUI))) {} -int Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {} +void Func23(void) __attribute__((try_acquire_capability(1, GUI))) {} +void Func24(void) __attribute__((try_acquire_shared_capability(1, GUI))) {} -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}} +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}} // 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 a6ddf028e1fad..73cc946ca0ce1 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1954,125 +1954,6 @@ 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 -// different 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. -namespac... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/97293 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
