https://github.com/dmcardle updated
https://github.com/llvm/llvm-project/pull/95290
>From e53ddc9e9ce0ddd8b5dfd5dfd4c8654afe643ce2 Mon Sep 17 00:00:00 2001
From: Dan McArdle <dmcar...@google.com>
Date: Mon, 17 Jun 2024 17:53:30 -0400
Subject: [PATCH] [clang][ThreadSafety] Enforce trylock function return type
With this change, Clang will generate an error when functions or methods
with disallowed return types are annotated as trylock functions. The
only allowed return types are boolean, integer, and pointer types.
This effect is that annotating a constructor, destructor, void-returning
function, etc. will fail to compile, where it silently passed before.
Issue #92408
---
clang/docs/ThreadSafetyAnalysis.rst | 12 ++++--
.../clang/Basic/DiagnosticSemaKinds.td | 3 +-
clang/include/clang/Sema/ParsedAttr.h | 1 +
clang/lib/Analysis/ThreadSafety.cpp | 3 ++
clang/lib/Sema/SemaDeclAttr.cpp | 21 +++++++++-
clang/test/Sema/attr-capabilities.c | 12 +++---
.../SemaCXX/warn-thread-safety-analysis.cpp | 4 +-
.../SemaCXX/warn-thread-safety-parsing.cpp | 40 ++++++++++++++-----
clang/unittests/AST/ASTImporterTest.cpp | 6 +--
9 files changed, 76 insertions(+), 26 deletions(-)
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index dcde0c706c704..999271320793b 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -420,10 +420,14 @@ 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 must be ``true`` or ``false``, to specify which
+return value indicates success. Integer literals are also accepted, but the
+analysis effectively converts them to boolean before use. 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
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 9b8f5b7e80e7e..072d7e8f91b36 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3721,7 +3721,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..7a78056b6b7b1 100644
--- a/clang/include/clang/Sema/ParsedAttr.h
+++ b/clang/include/clang/Sema/ParsedAttr.h
@@ -1101,6 +1101,7 @@ enum AttributeDeclKind {
ExpectedFunctionVariableOrClass,
ExpectedKernelFunction,
ExpectedFunctionWithProtoType,
+ ExpectedFunctionReturningBoolIntegerOrPointer,
};
inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index e25b843c9bf83..582181f79063c 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -32,6 +32,7 @@
#include "clang/Analysis/Analyses/ThreadSafetyUtil.h"
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Analysis/CFG.h"
+#include "clang/Basic/AttrKinds.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/OperatorKinds.h"
@@ -1366,6 +1367,8 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs,
AttrType *Attr,
branch = BLE->getValue();
else if (const auto *ILE = dyn_cast_or_null<IntegerLiteral>(BrE))
branch = ILE->getValue().getBoolValue();
+ else
+ llvm_unreachable("Trylock success values must be bool or integer.");
int branchnum = branch ? 0 : 1;
if (Neg)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index ce6b5b1ff6f93..d07f9d6c5568b 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -25,6 +25,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"
@@ -69,6 +70,7 @@
#include "llvm/Support/MathExtras.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/Triple.h"
+#include <cassert>
#include <optional>
using namespace clang;
@@ -605,6 +607,9 @@ static void handleAllocSizeAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
SmallVectorImpl<Expr *> &Args) {
+ // The attribute's first argument indicates which value the annotated
function
+ // will return when it has successfully acquired the lock. Semantically, it's
+ // a boolean value, but we'll also accept integers.
if (!AL.checkAtLeastNumArgs(S, 1))
return false;
@@ -614,9 +619,23 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D,
const ParsedAttr &AL,
return false;
}
- // check that all arguments are lockable objects
+ // Check that all remaining arguments are lockable objects.
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 1);
+ // Check that the attribute is applied to a function.
+ if (!D->isFunctionOrFunctionTemplate()) {
+ return false;
+ }
+ // Check that the function returns a boolean, integer, or pointer.
+ QualType ReturnType = D->getAsFunction()->getReturnType();
+ if (!ReturnType->isBooleanType() && !ReturnType->isIntegerType() &&
+ !ReturnType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_attribute_wrong_decl_type)
+ << AL << AL.isRegularKeywordAttribute()
+ << ExpectedFunctionReturningBoolIntegerOrPointer;
+ 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..48e78846ff787 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -3657,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_);
+ bool 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..6181bb0e26c06 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,14 @@ void etf_fun_params(int lvar
EXCLUSIVE_TRYLOCK_FUNCTION(1)); // \
// Check argument parsing.
-// legal attribute arguments
+// legal permutations of the first argument and function return type
+int etf_func_args_succ_1_ret_int() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2);
+bool etf_func_args_succ_1_ret_bool() EXCLUSIVE_TRYLOCK_FUNCTION(1, mu2);
+int etf_func_args_succ_true_ret_int() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu2);
+bool etf_func_args_succ_true_ret_bool() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu2);
+bool etf_func_args_succ_false() EXCLUSIVE_TRYLOCK_FUNCTION(false, 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());
@@ -799,9 +821,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 +846,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}}
};
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
