[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

Author: dmcardle

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

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<

clang/include/clang/Sema/ParsedAttr.h

@@ -1101,6 +1101,7 @@ enum AttributeDeclKind {
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
+  ExpectedFunctionReturningBoolIntegerOrPointer,
 };
 
 inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

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)

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;
 }
 

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)));

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();

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}}
 };
 

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());