https://github.com/melver created
https://github.com/llvm/llvm-project/pull/142955
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]
>From c47dac51fa0cc55492582f413241948df0ae2227 Mon Sep 17 00:00:00 2001
From: Marco Elver <el...@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 | 2 +-
clang/lib/Analysis/ThreadSafety.cpp | 16 ++-
clang/lib/Analysis/ThreadSafetyCommon.cpp | 62 ++++++++++-
clang/test/Sema/warn-thread-safety-analysis.c | 6 +-
.../SemaCXX/warn-thread-safety-analysis.cpp | 102 +++++++++++++++++-
5 files changed, 174 insertions(+), 14 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index 6c97905a2d7f9..f0aca6a4ff22f 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.
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index d4d06b6dedcae..3d124c6b73c73 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..4fbee11d02628 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"
@@ -114,6 +115,46 @@ static bool isCalleeArrow(const Expr *E) {
return ME ? ME->isArrow() : false;
}
+static bool isPointerReassigned(const VarDecl *VD) {
+ class AssignmentFinder : public RecursiveASTVisitor<AssignmentFinder> {
+ const VarDecl *VD;
+
+ public:
+ explicit AssignmentFinder(const VarDecl *VD) : VD(VD) {}
+
+ bool VisitBinaryOperator(BinaryOperator *BO) {
+ if (!BO->isAssignmentOp())
+ return true;
+
+ if (const auto *DRE =
+ dyn_cast<DeclRefExpr>(BO->getLHS()->IgnoreParenImpCasts())) {
+ // If target variable appears as LHS of assignment
+ if (DRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl()) {
+ // Skip the initializer
+ if (BO->getBeginLoc() != VD->getInit()->getBeginLoc()) {
+ FoundReassignment = true;
+ return false; // stop
+ }
+ }
+ }
+
+ return true;
+ }
+
+ bool FoundReassignment = false;
+ };
+
+ const DeclContext *DC = VD->getDeclContext();
+ if (const auto *FD = dyn_cast<FunctionDecl>(DC)) {
+ AssignmentFinder Visitor(VD);
+ Visitor.TraverseDecl(const_cast<FunctionDecl *>(FD));
+ return Visitor.FoundReassignment;
+ }
+
+ // Assume it might be reassigned.
+ return true;
+}
+
/// Translate a clang expression in an attribute to a til::SExpr.
/// Constructs the context from D, DeclExp, and SelfDecl.
///
@@ -241,7 +282,23 @@ 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 (VD->isLocalVarDecl() && Ty->isPointerType() && VD->hasInit() &&
+ (Ty.isConstQualified() || !isPointerReassigned(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 +410,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);
}
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
