[clang] [clang][ThreadSafety] Revert stricter typing on trylock attributes (PR #97293)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 1 06:43:02 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
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
More information about the cfe-commits
mailing list