[clang] Thread Safety Analysis: Very basic capability alias-analysis (PR #142955)
Marco Elver via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 17 09:21:59 PDT 2025
https://github.com/melver updated https://github.com/llvm/llvm-project/pull/142955
>From 608c4f657e2bcc0591e2fc32606a6738445cade6 Mon Sep 17 00:00:00 2001
From: Marco Elver <elver at google.com>
Date: Wed, 21 May 2025 23:49:48 +0200
Subject: [PATCH] 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]
---
.../Analysis/Analyses/ThreadSafetyCommon.h | 8 +-
clang/lib/Analysis/ThreadSafety.cpp | 16 ++-
clang/lib/Analysis/ThreadSafetyCommon.cpp | 78 +++++++++++++-
clang/test/Sema/warn-thread-safety-analysis.c | 6 +-
.../SemaCXX/warn-thread-safety-analysis.cpp | 102 +++++++++++++++++-
5 files changed, 196 insertions(+), 14 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index 6c97905a2d7f9..cd2c278c2278f 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -395,7 +395,7 @@ class SExprBuilder {
CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx);
// Translate a variable reference.
- til::LiteralPtr *createVariable(const VarDecl *VD);
+ til::SExpr *createVariable(const VarDecl *VD, CallingContext *Ctx = nullptr);
// Translate a clang statement or expression to a TIL expression.
// Also performs substitution of variables; Ctx provides the context.
@@ -501,6 +501,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 +534,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
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 80e7c8eff671a..a0ab241125d1c 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/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);
+ cast<til::LiteralPtr>(Analyzer->SxBuilder.createVariable(nullptr));
[[maybe_unused]] auto inserted =
Analyzer->ConstructedObjects.insert({Exp, Placeholder});
assert(inserted.second && "Are we visiting the same expression again?");
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index ddbd0a9ca904b..d95dfa087fdf0 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/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,22 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
return CapabilityExpr(E, AttrExp->getType(), Neg);
}
-til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
+til::SExpr *SExprBuilder::createVariable(const VarDecl *VD,
+ CallingContext *Ctx) {
+ if (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 +369,9 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
: cast<ObjCMethodDecl>(D)->getCanonicalDecl()->getParamDecl(I);
}
+ if (const auto *VarD = dyn_cast<VarDecl>(VD))
+ return createVariable(VarD, Ctx);
+
// For non-local variables, treat it as a reference to a named object.
return new (Arena) til::LiteralPtr(VD);
}
@@ -1012,6 +1031,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 {
diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c
index b43f97af9162e..549cb1231baa6 100644
--- a/clang/test/Sema/warn-thread-safety-analysis.c
+++ b/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}}
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index d64ed1e5f260a..da5fd350daf74 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/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
More information about the cfe-commits
mailing list