Thread Safety Analysis: Very basic capability alias-analysis

Add a simple form of alias analysis for capabilities by substituting
local pointer variables with their initializers if they are `const` or
never reassigned.

For example, the analysis will no longer generate false positives for
cases such as:

	void testNestedAccess(Container *c) {
	  Foo *ptr = &c->foo;
	  ptr->mu.Lock();
	  c->foo.data = 42;  // OK - no false positive
	  ptr->mu.Unlock();
	}

	void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) {
	  Foo *buf = &c->foo;
	  buf->mu.Lock();  // OK - no false positive warning
	}

This implementation would satisfy the basic needs of addressing the
concerns for Linux kernel application [1].

Current limitations:

* The analysis does not handle pointers that are reassigned; it
  conservatively assumes they could point to anything after the
  reassignment.

* Aliases created through complex control flow are not tracked.

Link: https://lore.kernel.org/all/CANpmjNPquO=W1JAh1FNQb8pMQjgeZAKCPQUAd7qUg=5pjJ6x=Q@mail.gmail.com/ [1]

Author: melver

clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h

@@ -31,10 +31,12 @@
 #include "clang/Analysis/CFG.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
+#include <optional>
 #include <sstream>
 #include <string>
 #include <utility>
@@ -386,6 +388,11 @@ class SExprBuilder {
     SelfVar->setKind(til::Variable::VK_SFun);
   }
 
+  // Create placeholder for this: we don't know the VarDecl on construction yet.
+  til::LiteralPtr *createThisPlaceholder() {
+    return new (Arena) til::LiteralPtr(nullptr);
+  }
+
   // Translate a clang expression in an attribute to a til::SExpr.
   // Constructs the context from D, DeclExp, and SelfDecl.
   CapabilityExpr translateAttrExpr(const Expr *AttrExp, const NamedDecl *D,
@@ -394,8 +401,8 @@ class SExprBuilder {
 
   CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx);
 
-  // Translate a variable reference.
-  til::LiteralPtr *createVariable(const VarDecl *VD);
+  // Translate a VarDecl to its canonical TIL expression.
+  til::SExpr *translateVariable(const VarDecl *VD, CallingContext *Ctx);
 
   // Translate a clang statement or expression to a TIL expression.
   // Also performs substitution of variables; Ctx provides the context.
@@ -445,6 +452,7 @@ class SExprBuilder {
       const AbstractConditionalOperator *C, CallingContext *Ctx);
 
   til::SExpr *translateDeclStmt(const DeclStmt *S, CallingContext *Ctx);
+  til::SExpr *translateStmtExpr(const StmtExpr *SE, CallingContext *Ctx);
 
   // Map from statements in the clang CFG to SExprs in the til::SCFG.
   using StatementMap = llvm::DenseMap<const Stmt *, til::SExpr *>;
@@ -501,6 +509,9 @@ class SExprBuilder {
   void mergeEntryMapBackEdge();
   void mergePhiNodesBackEdge(const CFGBlock *Blk);
 
+  // Returns true if a variable is assumed to be reassigned.
+  bool isVariableReassigned(const VarDecl *VD);
+
 private:
   // Set to true when parsing capability expressions, which get translated
   // inaccurately in order to hack around smart pointers etc.
@@ -531,6 +542,9 @@ class SExprBuilder {
   std::vector<til::Phi *> IncompleteArgs;
   til::BasicBlock *CurrentBB = nullptr;
   BlockInfo *CurrentBlockInfo = nullptr;
+
+  // Set caching local variables that are reassigned.
+  std::optional<llvm::DenseSet<const VarDecl *>> ReassignedLocalVariables;
 };
 
 #ifndef NDEBUG

clang/lib/Analysis/ThreadSafety.cpp

@@ -1141,11 +1141,10 @@ class ThreadSafetyAnalyzer {
 
   void warnIfMutexNotHeld(const FactSet &FSet, const NamedDecl *D,
                           const Expr *Exp, AccessKind AK, Expr *MutexExp,
-                          ProtectedOperationKind POK, til::LiteralPtr *Self,
+                          ProtectedOperationKind POK, til::SExpr *Self,
                           SourceLocation Loc);
   void warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp,
-                       Expr *MutexExp, til::LiteralPtr *Self,
-                       SourceLocation Loc);
+                       Expr *MutexExp, til::SExpr *Self, SourceLocation Loc);
 
   void checkAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK,
                    ProtectedOperationKind POK);
@@ -1599,7 +1598,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
   }
 
   void handleCall(const Expr *Exp, const NamedDecl *D,
-                  til::LiteralPtr *Self = nullptr,
+                  til::SExpr *Self = nullptr,
                   SourceLocation Loc = SourceLocation());
   void examineArguments(const FunctionDecl *FD,
                         CallExpr::const_arg_iterator ArgBegin,
@@ -1629,7 +1628,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
 /// of at least the passed in AccessKind.
 void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
     const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK,
-    Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self,
+    Expr *MutexExp, ProtectedOperationKind POK, til::SExpr *Self,
     SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
   CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
@@ -1688,8 +1687,7 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
 /// Warn if the LSet contains the given lock.
 void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet,
                                            const NamedDecl *D, const Expr *Exp,
-                                           Expr *MutexExp,
-                                           til::LiteralPtr *Self,
+                                           Expr *MutexExp, til::SExpr *Self,
                                            SourceLocation Loc) {
   CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
@@ -1857,7 +1855,7 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
 ///              of an implicitly called cleanup function.
 /// \param Loc   If \p Exp = nullptr, the location.
 void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
-                              til::LiteralPtr *Self, SourceLocation Loc) {
+                              til::SExpr *Self, SourceLocation Loc) {
   CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
   CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
   CapExprSet ScopedReqsAndExcludes;
@@ -1869,7 +1867,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
     const auto *TagT = Exp->getType()->getAs<TagType>();
     if (D->hasAttrs() && TagT && Exp->isPRValue()) {
       til::LiteralPtr *Placeholder =
-          Analyzer->SxBuilder.createVariable(nullptr);
+          Analyzer->SxBuilder.createThisPlaceholder();
       [[maybe_unused]] auto inserted =
           Analyzer->ConstructedObjects.insert({Exp, Placeholder});
       assert(inserted.second && "Are we visiting the same expression again?");
@@ -2545,7 +2543,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
       }
       if (UnderlyingLocks.empty())
         continue;
-      CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(),
+      CapabilityExpr Cp(SxBuilder.translateVariable(Param, nullptr),
+                        StringRef(),
                         /*Neg=*/false, /*Reentrant=*/false);
       auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(
           Cp, Param->getLocation(), FactEntry::Declared);
@@ -2662,17 +2661,19 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
           if (!DD->hasAttrs())
             break;
 
-          LocksetBuilder.handleCall(nullptr, DD,
-                                    SxBuilder.createVariable(AD.getVarDecl()),
-                                    AD.getTriggerStmt()->getEndLoc());
+          LocksetBuilder.handleCall(
+              nullptr, DD,
+              SxBuilder.translateVariable(AD.getVarDecl(), nullptr),
+              AD.getTriggerStmt()->getEndLoc());
           break;
         }
 
         case CFGElement::CleanupFunction: {
           const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>();
-          LocksetBuilder.handleCall(/*Exp=*/nullptr, CF.getFunctionDecl(),
-                                    SxBuilder.createVariable(CF.getVarDecl()),
-                                    CF.getVarDecl()->getLocation());
+          LocksetBuilder.handleCall(
+              /*Exp=*/nullptr, CF.getFunctionDecl(),
+              SxBuilder.translateVariable(CF.getVarDecl(), nullptr),
+              CF.getVarDecl()->getLocation());
           break;
         }
 

clang/lib/Analysis/ThreadSafetyCommon.cpp

@@ -19,6 +19,7 @@
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/OperationKinds.h"
+#include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Type.h"
 #include "clang/Analysis/Analyses/ThreadSafetyTIL.h"
@@ -241,7 +242,17 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
   return CapabilityExpr(E, AttrExp->getType(), Neg);
 }
 
-til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
+til::SExpr *SExprBuilder::translateVariable(const VarDecl *VD,
+                                            CallingContext *Ctx) {
+  assert(VD);
+
+  QualType Ty = VD->getType();
+  if (Ty->isPointerType() && VD->hasInit() && !isVariableReassigned(VD)) {
+    // Substitute local pointer variables with their initializers if they are
+    // explicitly const or never reassigned.
+    return translate(VD->getInit(), Ctx);
+  }
+
   return new (Arena) til::LiteralPtr(VD);
 }
 
@@ -313,6 +324,8 @@ til::SExpr *SExprBuilder::translate(const Stmt *S, CallingContext *Ctx) {
 
   case Stmt::DeclStmtClass:
     return translateDeclStmt(cast<DeclStmt>(S), Ctx);
+  case Stmt::StmtExprClass:
+    return translateStmtExpr(cast<StmtExpr>(S), Ctx);
   default:
     break;
   }
@@ -353,6 +366,9 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
              : cast<ObjCMethodDecl>(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
+  if (const auto *VarD = dyn_cast<VarDecl>(VD))
+    return translateVariable(VarD, Ctx);
+
   // For non-local variables, treat it as a reference to a named object.
   return new (Arena) til::LiteralPtr(VD);
 }
@@ -691,6 +707,15 @@ SExprBuilder::translateDeclStmt(const DeclStmt *S, CallingContext *Ctx) {
   return nullptr;
 }
 
+til::SExpr *SExprBuilder::translateStmtExpr(const StmtExpr *SE,
+                                            CallingContext *Ctx) {
+  // The value of a statement expression is the value of the last statement,
+  // which must be an expression.
+  const CompoundStmt *CS = SE->getSubStmt();
+  return CS->body_empty() ? new (Arena) til::Undefined(SE)
+                          : translate(CS->body_back(), Ctx);
+}
+
 // If (E) is non-trivial, then add it to the current basic block, and
 // update the statement map so that S refers to E.  Returns a new variable
 // that refers to E.
@@ -1012,6 +1037,100 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
   IncompleteArgs.clear();
 }
 
+bool SExprBuilder::isVariableReassigned(const VarDecl *VD) {
+  struct ReassignmentFinder : RecursiveASTVisitor<ReassignmentFinder> {
+    bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+      // Check for self-reference in the initializer. This is not strictly a
+      // reassignment, but undefined behaviour.
+      if (DRE->getDecl()->getCanonicalDecl() == InVarDecl)
+        ReassignedLocalVariables.insert(InVarDecl);
+      return true;
+    }
+
+    bool TraverseVarDecl(VarDecl *VD) {
+      if (!VD->hasInit() || !VD->isLocalVarDecl())
+        return true;
+      // Traverse the initializer to check for self-initialization.
+      InVarDecl = VD;
+      TraverseStmt(VD->getInit());
+      InVarDecl = nullptr;
+      return true;
+    }
+
+    bool VisitUnaryOperator(UnaryOperator *UO) {
+      if (UO->getOpcode() != UO_AddrOf)
+        return true;
+      // If the address of a variable is taken, it has "escaped", assume it may
+      // be reassigned.
+      const Expr *SubExpr = UO->getSubExpr()->IgnoreParenImpCasts();
+      if (const auto *DRE = dyn_cast<DeclRefExpr>(SubExpr))
+        if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+          if (VD->isLocalVarDecl())
+            ReassignedLocalVariables.insert(VD->getCanonicalDecl());
+      return true;
+    }
+
+    bool VisitBinaryOperator(BinaryOperator *BO) {
+      if (InVarDecl)
+        return true;
+      if (!BO->isAssignmentOp())
+        return true;
+      // If a variable appears as LHS of assignment, then it's a reassignment.
+      const auto *LHS = BO->getLHS()->IgnoreParenImpCasts();
+      if (const auto *DRE = dyn_cast<DeclRefExpr>(LHS))
+        if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+          if (VD->isLocalVarDecl())
+            ReassignedLocalVariables.insert(VD->getCanonicalDecl());
+      return true;
+    }
+
+    bool VisitCallExpr(CallExpr *CE) {
+      const FunctionDecl *FD = CE->getDirectCallee();
+      if (!FD)
+        return true;
+      for (unsigned Idx = 0; Idx < CE->getNumArgs(); ++Idx) {
+        if (Idx >= FD->getNumParams())
+          break;
+        const Expr *Arg = CE->getArg(Idx)->IgnoreParenImpCasts();
+        const ParmVarDecl *PVD = FD->getParamDecl(Idx);
+        QualType ParamType = PVD->getType();
+        // Potential reassignment if passed by non-const reference.
+        if (ParamType->isReferenceType() &&
+            !ParamType->getPointeeType().isConstQualified()) {
+          if (const auto *DRE = dyn_cast<DeclRefExpr>(Arg))
+            if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+              if (VD->isLocalVarDecl())
+                ReassignedLocalVariables.insert(VD->getCanonicalDecl());
+        }
+      }
+      return true;
+    }
+
+    const VarDecl *InVarDecl = nullptr;
+    llvm::DenseSet<const VarDecl *> ReassignedLocalVariables;
+  };
+
+  if (VD->getType().isConstQualified())
+    return false; // Assume undefined-behavior freedom.
+  if (!VD->isLocalVarDecl())
+    return true; // Not a local variable (assume reassigned).
+  auto *FD = dyn_cast<FunctionDecl>(VD->getDeclContext());
+  if (!FD)
+    return true; // Assume reassigned.
+
+  if (LLVM_UNLIKELY(!ReassignedLocalVariables.has_value())) {
+    // Lazy init ReassignedLocalVariables set.
+    ReassignmentFinder Visitor;
+    // const_cast ok: FunctionDecl not modified.
+    Visitor.TraverseDecl(const_cast<FunctionDecl *>(FD));
+    ReassignedLocalVariables = std::move(Visitor.ReassignedLocalVariables);
+  }
+
+  // Use the canonical declaration to ensure consistent lookup in the cache.
+  VD = VD->getCanonicalDecl();
+  return ReassignedLocalVariables.value().contains(VD);
+}
+
 #ifndef NDEBUG
 namespace {
 

clang/test/Sema/warn-thread-safety-analysis.c

@@ -184,9 +184,13 @@ int main(void) {
   /// Cleanup functions
   {
     struct Mutex* const __attribute__((cleanup(unlock_scope))) scope = &mu1;
-    mutex_exclusive_lock(scope);  // Note that we have to lock through scope, because no alias analysis!
+    mutex_exclusive_lock(scope);  // Lock through scope works.
     // Cleanup happens automatically -> no warning.
   }
+  {
+    struct Mutex* const __attribute__((unused, cleanup(unlock_scope))) scope = &mu1;
+    mutex_exclusive_lock(&mu1);  // With basic alias analysis lock through mu1 also works.
+  }
 
   foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'mu_' exclusively}}
   *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'bar.other_mu' exclusively}}