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

@@ -386,6 +386,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 +399,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 *translateVarDecl(const VarDecl *VD, CallingContext *Ctx);
 
   // Translate a clang statement or expression to a TIL expression.
   // Also performs substitution of variables; Ctx provides the context.
@@ -501,6 +506,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 +539,9 @@ class SExprBuilder {
   std::vector<til::Phi *> IncompleteArgs;
   til::BasicBlock *CurrentBB = nullptr;
   BlockInfo *CurrentBlockInfo = nullptr;
+
+  // Map caching if a local variable is reassigned.
+  llvm::DenseMap<const VarDecl *, bool> LocalVariableReassigned;
 };
 
 #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,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
       }
       if (UnderlyingLocks.empty())
         continue;
-      CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(),
+      CapabilityExpr Cp(SxBuilder.translateVarDecl(Param, nullptr), StringRef(),
                         /*Neg=*/false, /*Reentrant=*/false);
       auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(
           Cp, Param->getLocation(), FactEntry::Declared);
@@ -2662,17 +2660,18 @@ 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.translateVarDecl(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.translateVarDecl(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,21 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
   return CapabilityExpr(E, AttrExp->getType(), Neg);
 }
 
-til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
+til::SExpr *SExprBuilder::translateVarDecl(const VarDecl *VD,
+                                           CallingContext *Ctx) {
+  assert(VD);
+  // Substitute local pointer variables with their initializers if they are
+  // explicitly const or never reassigned.
+  QualType Ty = VD->getType();
+  if (Ty->isPointerType() && VD->hasInit() && !isVariableReassigned(VD)) {
+    const Expr *Init = VD->getInit()->IgnoreParenImpCasts();
+    // Check for self-initialization to prevent infinite recursion.
+    if (const auto *InitDRE = dyn_cast<DeclRefExpr>(Init)) {
+      if (InitDRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl())
+        return new (Arena) til::LiteralPtr(VD);
+    }
+    return translate(Init, Ctx);
+  }
   return new (Arena) til::LiteralPtr(VD);
 }
 
@@ -353,6 +368,9 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
              : cast<ObjCMethodDecl>(D)->getCanonicalDecl()->getParamDecl(I);
   }
 
+  if (const auto *VarD = dyn_cast<VarDecl>(VD))
+    return translateVarDecl(VarD, Ctx);
+
   // For non-local variables, treat it as a reference to a named object.
   return new (Arena) til::LiteralPtr(VD);
 }
@@ -1012,6 +1030,63 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
   IncompleteArgs.clear();
 }
 
+bool SExprBuilder::isVariableReassigned(const VarDecl *VD) {
+  // Note: The search is performed lazily per-variable and result is cached. An
+  // alternative would have been to eagerly create a set of all reassigned
+  // variables, but that would consume significantly more memory. The number of
+  // variables needing the reassignment check in a single function is expected
+  // to be small. Also see createVariable().
+  struct ReassignmentFinder : RecursiveASTVisitor<ReassignmentFinder> {
+    explicit ReassignmentFinder(const VarDecl *VD) : QueryVD(VD) {
+      assert(QueryVD->getCanonicalDecl() == QueryVD);
+    }
+
+    bool VisitBinaryOperator(BinaryOperator *BO) {
+      if (!BO->isAssignmentOp())
+        return true;
+      auto *DRE = dyn_cast<DeclRefExpr>(BO->getLHS()->IgnoreParenImpCasts());
+      if (!DRE)
+        return true;
+      auto *AssignedVD = dyn_cast<VarDecl>(DRE->getDecl());
+      if (!AssignedVD)
+        return true;
+      // Don't count the initialization itself as a reassignment.
+      if (AssignedVD->hasInit() &&
+          AssignedVD->getInit()->getBeginLoc() == BO->getBeginLoc())
+        return true;
+      // If query variable appears as LHS of assignment.
+      if (QueryVD == AssignedVD->getCanonicalDecl()) {
+        FoundReassignment = true;
+        return false; // stop
+      }
+      return true;
+    }
+
+    const VarDecl *QueryVD;
+    bool FoundReassignment = false;
+  };
+
+  if (VD->getType().isConstQualified())
+    return false; // Assume UB-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.
+
+  // Try to look up in cache; use the canonical declaration to ensure consistent
+  // lookup in the cache.
+  VD = VD->getCanonicalDecl();
+  auto It = LocalVariableReassigned.find(VD);
+  if (It != LocalVariableReassigned.end())
+    return It->second;
+
+  ReassignmentFinder Visitor(VD);
+  // const_cast ok: FunctionDecl not modified.
+  Visitor.TraverseDecl(const_cast<FunctionDecl *>(FD));
+  return LocalVariableReassigned[VD] = Visitor.FoundReassignment;
+}
+
 #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}}

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

@@ -1556,9 +1556,9 @@ void main() {
   Child *c;
   Base *b = c;
 
-  b->func1(); // expected-warning {{calling function 'func1' requires holding mutex 'b->mu_' exclusively}}
+  b->func1(); // expected-warning {{calling function 'func1' requires holding mutex 'c->mu_' exclusively}}
   b->mu_.Lock();
-  b->func2(); // expected-warning {{cannot call function 'func2' while mutex 'b->mu_' is held}}
+  b->func2(); // expected-warning {{cannot call function 'func2' while mutex 'c->mu_' is held}}
   b->mu_.Unlock();
 
   c->func1(); // expected-warning {{calling function 'func1' requires holding mutex 'c->mu_' exclusively}}
@@ -7224,3 +7224,101 @@ class TestNegativeWithReentrantMutex {
 };
 
 } // namespace Reentrancy
+
+// Tests for tracking aliases of capabilities.
+namespace CapabilityAliases {
+struct Foo {
+  Mutex mu;
+  int data GUARDED_BY(mu);
+};
+
+void testBasicPointerAlias(Foo *f) {
+  Foo *ptr = f;
+  ptr->mu.Lock();         // lock through alias
+  f->data = 42;           // access through original
+  ptr->mu.Unlock();       // unlock through alias
+}
+
+// FIXME: No alias tracking for non-const reassigned pointers.
+void testReassignment() {
+  Foo f1, f2;
+  Foo *ptr = &f1;
+  ptr->mu.Lock();
+  f1.data = 42;           // expected-warning{{writing variable 'data' requires holding mutex 'f1.mu' exclusively}} \
+                          // expected-note{{found near match 'ptr->mu'}}
+  ptr->mu.Unlock();
+
+  ptr = &f2;              // reassign
+  ptr->mu.Lock();
+  f2.data = 42;           // expected-warning{{writing variable 'data' requires holding mutex 'f2.mu' exclusively}} \
+                          // expected-note{{found near match 'ptr->mu'}}
+  f1.data = 42;           // expected-warning{{writing variable 'data' requires holding mutex 'f1.mu'}} \
+                          // expected-note{{found near match 'ptr->mu'}}
+  ptr->mu.Unlock();
+}
+
+// Nested field access through pointer
+struct Container {
+  Foo foo;
+};
+
+void testNestedAccess(Container *c) {
+  Foo *ptr = &c->foo; // pointer to nested field
+  ptr->mu.Lock();
+  c->foo.data = 42;
+  ptr->mu.Unlock();
+}
+
+void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) {
+  Foo *buf = &c->foo;
+  buf->mu.Lock();
+}
+
+struct ContainerOfPtr {
+  Foo *foo_ptr;
+};
+
+void testIndirectAccess(ContainerOfPtr *fc) {
+  Foo *ptr = fc->foo_ptr; // get pointer
+  ptr->mu.Lock();
+  fc->foo_ptr->data = 42; // access via original
+  ptr->mu.Unlock();
+}
+
+// FIXME: No alias tracking through complex control flow.
+void testSimpleControlFlow(Foo *f1, Foo *f2, bool cond) {
+  Foo *ptr;
+  if (cond) {
+    ptr = f1;
+  } else {
+    ptr = f2;
+  }
+  ptr->mu.Lock();
+  if (cond) {
+    f1->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f1->mu' exclusively}} \
+                   // expected-note{{found near match 'ptr->mu'}}
+  } else {
+    f2->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f2->mu' exclusively}} \
+                   // expected-note{{found near match 'ptr->mu'}}
+  }
+  ptr->mu.Unlock();
+}
+
+void testLockFunction(Foo *f) EXCLUSIVE_LOCK_FUNCTION(&f->mu) {
+  Mutex *mu = &f->mu;
+  mu->Lock();
+}
+
+void testUnlockFunction(Foo *f) UNLOCK_FUNCTION(&f->mu) {
+  Mutex *mu = &f->mu;
+  mu->Unlock();
+}
+
+// Semantically UB, but let's not crash the compiler with this (should be
+// handled by -Wuninitialized).
+void testSelfInit() {
+  Mutex *mu = mu; // don't do this at home
+  mu->Lock();
+  mu->Unlock();
+}
+}  // namespace CapabilityAliases