[clang][ThreadSafety] Revert stricter typing on trylock attributes (#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.

Author: dmcardle

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
@@ -754,11 +749,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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 

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

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<

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,

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

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

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