[clang][ThreadSafety] Check trylock function success and return types (#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 #92408

1 files changed, 46 insertions, 36 deletions

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index e25b843..550c2a3 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/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 @@ public:
 
   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;
     }
   }