[clang][ThreadSafety] Check trylock function success and return types (llvm#95290)

With this change, Clang will generate errors when trylock functions have
improper return types. Today, it silently fails to apply the trylock
attribute to these functions which may incorrectly lead users to believe
they have correctly acquired locks before accessing guarded data.

As a side effect of explicitly checking the success argument type, I
seem to have fixed a false negative in the analysis that could occur
when a trylock's success argument is an enumerator. I've added a
regression test to warn-thread-safety-analysis.cpp named
`TrylockSuccessEnumFalseNegative`.

This change also improves the documentation with descriptions of of the
subtle gotchas that arise from the analysis interpreting the success arg
as a boolean.

Issue llvm#92408

Author: dmcardle

clang/docs/ReleaseNotes.rst

@@ -71,6 +71,11 @@ 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
@@ -729,6 +734,11 @@ 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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 

clang/docs/ThreadSafetyAnalysis.rst

@@ -420,10 +420,17 @@ 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 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.
 
 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
@@ -445,6 +452,43 @@ 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(...)
 --------------------------------------------------------

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -3272,11 +3272,23 @@ 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}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"
+  "|nullptr or a bool, int, or enum literal}2">;
 def err_attribute_argument_type : Error<
-  "%0 attribute requires %select{int or bool|an integer "
-  "constant|a string|an identifier}1">;
+  "%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">;
 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<
@@ -3728,7 +3740,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

@@ -1083,6 +1083,7 @@ enum AttributeArgumentNType {
   AANT_ArgumentIdentifier,
   AANT_ArgumentConstantExpr,
   AANT_ArgumentBuiltinFunction,
+  AANT_ArgumentNullptrOrBoolIntOrEnumLiteral,
 };
 
 /// These constants match the enumerated choices of
@@ -1101,6 +1102,7 @@ enum AttributeDeclKind {
   ExpectedFunctionVariableOrClass,
   ExpectedKernelFunction,
   ExpectedFunctionWithProtoType,
+  ExpectedFunctionReturningPointerBoolIntOrEnum,
 };
 
 inline const StreamingDiagnostic &operator<<(const StreamingDiagnostic &DB,

clang/lib/Analysis/ThreadSafety.cpp

@@ -37,6 +37,7 @@
 #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"
@@ -1034,9 +1035,10 @@ class ThreadSafetyAnalyzer {
 
   template <class AttrType>
   void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
-                   const NamedDecl *D,
-                   const CFGBlock *PredBlock, const CFGBlock *CurrBlock,
-                   Expr *BrE, bool Neg);
+                   const NamedDecl *D, const CFGBlock *PredBlock,
+                   const CFGBlock *CurrBlock,
+                   const ASTContext &TrylockAttrContext,
+                   Expr *TrylockAttrSuccessValue, bool Neg);
 
   const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C,
                                      bool &Negate);
@@ -1359,17 +1361,18 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
                                        const Expr *Exp, const NamedDecl *D,
                                        const CFGBlock *PredBlock,
                                        const CFGBlock *CurrBlock,
-                                       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;
+                                       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;
 
   // If we've taken the trylock branch, then add the lock
   int i = 0;
@@ -1390,8 +1393,15 @@ 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;
 }
 
@@ -1497,27 +1507,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, 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;
+    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;
     }
   }
 

clang/lib/Sema/SemaDeclAttr.cpp

@@ -14,6 +14,7 @@
 #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"
@@ -25,6 +26,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"
@@ -65,10 +67,13 @@
 #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;
@@ -166,13 +171,6 @@ 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) {
@@ -608,15 +606,31 @@ static bool checkTryLockFunAttrCommon(Sema &S, Decl *D, const ParsedAttr &AL,
   if (!AL.checkAtLeastNumArgs(S, 1))
     return false;
 
-  if (!isIntOrBool(AL.getArgAsExpr(0))) {
+  // 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()) {
     S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
-        << AL << 1 << AANT_ArgumentIntOrBool;
+        << AL << 1 << AANT_ArgumentNullptrOrBoolIntOrEnumLiteral;
     return false;
   }
 
-  // check that all arguments are lockable objects
+  // All remaining arguments must be 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;
 }
 

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