[clang] [clang][ThreadSafety] Revert stricter typing on trylock attributes (PR #97293)
Dan McArdle via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 1 06:42:30 PDT 2024
https://github.com/dmcardle created https://github.com/llvm/llvm-project/pull/97293
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.
>From 5202d53b5e32c79b4dec723f80e4276bee77a0e6 Mon Sep 17 00:00:00 2001
From: Dan McArdle <dmcardle at google.com>
Date: Mon, 1 Jul 2024 09:32:42 -0400
Subject: [PATCH 1/2] Revert "[clang][ThreadSafety] Fix code block syntax in
ThreadSafetyAnalysis.rst (#96494)"
This reverts commit 34026207c87116bd8e7fb0a464ea8db947f8239a.
---
clang/docs/ThreadSafetyAnalysis.rst | 1 -
1 file changed, 1 deletion(-)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst b/clang/docs/ThreadSafetyAnalysis.rst
index 1513719caa464..0ecbebe7a692f 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -464,7 +464,6 @@ 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 };
>From 783f56be1bacdce768ebc11e8566171a9693becf Mon Sep 17 00:00:00 2001
From: Dan McArdle <dmcardle at google.com>
Date: Mon, 1 Jul 2024 09:33:03 -0400
Subject: [PATCH 2/2] Revert "[clang][ThreadSafety] Check trylock function
success and return types (#95290)"
This reverts commit c1bde0a2cb640b3607e9568b9a57b292e1f82666.
---
clang/docs/ReleaseNotes.rst | 10 --
clang/docs/ThreadSafetyAnalysis.rst | 52 +-------
.../clang/Basic/DiagnosticSemaKinds.td | 23 +---
clang/include/clang/Sema/ParsedAttr.h | 2 -
clang/lib/Analysis/ThreadSafety.cpp | 82 +++++-------
clang/lib/Sema/SemaDeclAttr.cpp | 34 ++---
clang/test/Sema/attr-capabilities.c | 12 +-
.../SemaCXX/warn-thread-safety-analysis.cpp | 123 +-----------------
.../SemaCXX/warn-thread-safety-parsing.cpp | 107 +++------------
clang/unittests/AST/ASTImporterTest.cpp | 6 +-
10 files changed, 82 insertions(+), 369 deletions(-)
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 0ecbebe7a692f..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,43 +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.
-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 {
@@ -3776,8 +3657,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 4921a75b84e39..0c5b0cc85897b 100644
--- a/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-parsing.cpp
@@ -716,17 +716,15 @@ int slf_function_bad_7() SHARED_LOCK_FUNCTION(0); // \
#error "Should support exclusive_trylock_function attribute"
#endif
-// 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.
+// 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_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_args() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2);
-int etf_function_arg() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+void 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); // \
@@ -745,23 +743,10 @@ class EtfFoo {
private:
int test_field EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
// expected-warning {{'exclusive_trylock_function' attribute only applies to functions}}
- bool test_method() EXCLUSIVE_TRYLOCK_FUNCTION(1); // \
+ void 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}}
};
@@ -771,68 +756,7 @@ void etf_fun_params(int lvar EXCLUSIVE_TRYLOCK_FUNCTION(1)); // \
// Check argument parsing.
-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
+// legal 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());
@@ -844,13 +768,14 @@ 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 nullptr or a bool, int, or enum literal}}
+ // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be int or bool}}
int etf_function_bad_2() EXCLUSIVE_TRYLOCK_FUNCTION("mu"); // \
- // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}}
+ // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be int or bool}}
int etf_function_bad_3() EXCLUSIVE_TRYLOCK_FUNCTION(muDoublePointer); // \
- // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}}
+ // expected-error {{'exclusive_trylock_function' attribute requires parameter 1 to be int or bool}}
int etf_function_bad_4() EXCLUSIVE_TRYLOCK_FUNCTION(1, "mu"); // \
// expected-warning {{ignoring 'exclusive_trylock_function' attribute because its argument is invalid}}
@@ -874,9 +799,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}}
-bool stf_function_args() SHARED_TRYLOCK_FUNCTION(1, mu2);
+void stf_function_args() SHARED_TRYLOCK_FUNCTION(1, mu2);
-bool stf_function_arg() SHARED_TRYLOCK_FUNCTION(1); // \
+void 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); // \
@@ -899,7 +824,7 @@ class StfFoo {
private:
int test_field SHARED_TRYLOCK_FUNCTION(1); // \
// expected-warning {{'shared_trylock_function' attribute only applies to functions}}
- bool test_method() SHARED_TRYLOCK_FUNCTION(1); // \
+ void 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}}
};
@@ -924,11 +849,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 nullptr or a bool, int, or enum literal}}
+ // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be int or bool}}
int stf_function_bad_2() SHARED_TRYLOCK_FUNCTION("mu"); // \
- // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}}
+ // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be int or bool}}
int stf_function_bad_3() SHARED_TRYLOCK_FUNCTION(muDoublePointer); // \
- // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be nullptr or a bool, int, or enum literal}}
+ // expected-error {{'shared_trylock_function' attribute requires parameter 1 to be int or bool}}
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 4c33546de6f92..92f9bae6cb064 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>(
- "bool test(int A1, int A2) __attribute__((try_acquire_capability(1, A1, "
+ "void 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>(
- "bool test(int A1, int A2) __attribute__((exclusive_trylock_function(1, "
+ "void 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>(
- "bool test(int A1, int A2) __attribute__((shared_trylock_function(1, A1, "
+ "void test(int A1, int A2) __attribute__((shared_trylock_function(1, A1, "
"A2)));",
FromAttr, ToAttr);
checkImported(FromAttr->getSuccessValue(), ToAttr->getSuccessValue());
More information about the cfe-commits
mailing list