[clang] Thread Safety Analysis: Fix recursive capability alias resolution (PR #159921)
Marco Elver via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 20 05:42:46 PDT 2025
https://github.com/melver created https://github.com/llvm/llvm-project/pull/159921
Fix a false positive in thread safety alias analysis caused by incorrect late resolution of aliases. The analysis previously failed to distinguish between an alias and its defining expression; reassigning a variable within that expression (e.g., `ptr` in `alias = ptr->field`) would incorrectly invalidate the alias.
The fix is to properly use LocalVariableMap::lookupExpr's updated context in a recursive lookup.
Reported-by: Christoph Hellwig <hch at lst.de>
Link: https://lkml.kernel.org/r/20250919140803.GA23745@lst.de
>From 6aa987b465e3f269543c075b9f6b12d06f20566a Mon Sep 17 00:00:00 2001
From: Marco Elver <elver at google.com>
Date: Sat, 20 Sep 2025 14:37:44 +0200
Subject: [PATCH] Thread Safety Analysis: Fix recursive capability alias
resolution
Fix a false positive in thread safety alias analysis caused by incorrect
late resolution of aliases. The analysis previously failed to
distinguish between an alias and its defining expression; reassigning a
variable within that expression (e.g., `ptr` in `alias = ptr->field`)
would incorrectly invalidate the alias.
The fix is to properly use LocalVariableMap::lookupExpr's updated
context in a recursive lookup.
Reported-by: Christoph Hellwig <hch at lst.de>
Link: https://lkml.kernel.org/r/20250919140803.GA23745@lst.de
---
.../Analysis/Analyses/ThreadSafetyCommon.h | 6 ++-
clang/lib/Analysis/ThreadSafety.cpp | 44 +++++++++----------
clang/lib/Analysis/ThreadSafetyCommon.cpp | 12 ++++-
.../SemaCXX/warn-thread-safety-analysis.cpp | 18 ++++++++
4 files changed, 55 insertions(+), 25 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index d20f172f446e6..ffdfde8b7d453 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -543,10 +543,14 @@ class SExprBuilder {
til::BasicBlock *CurrentBB = nullptr;
BlockInfo *CurrentBlockInfo = nullptr;
+ // The closure that captures state required for the lookup; this may be
+ // mutable, so we have to save/restore before/after recursive lookups.
+ using LookupLocalVarExprClosure =
+ std::function<const Expr *(const NamedDecl *)>;
// Recursion guard.
llvm::DenseSet<const ValueDecl *> VarsBeingTranslated;
// Context-dependent lookup of currently valid definitions of local variables.
- std::function<const Expr *(const NamedDecl *)> LookupLocalVarExpr;
+ LookupLocalVarExprClosure LookupLocalVarExpr;
};
#ifndef NDEBUG
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index cee98d58a6112..69e71f0e0ab47 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1669,12 +1669,12 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
const LocalVarContext &LVarCtx = PredBlockInfo->ExitContext;
// Temporarily set the lookup context for SExprBuilder.
- SxBuilder.setLookupLocalVarExpr([&](const NamedDecl *D) -> const Expr * {
- if (!Handler.issueBetaWarnings())
- return nullptr;
- auto Ctx = LVarCtx;
- return LocalVarMap.lookupExpr(D, Ctx);
- });
+ SxBuilder.setLookupLocalVarExpr(
+ [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * {
+ if (!Handler.issueBetaWarnings())
+ return nullptr;
+ return LocalVarMap.lookupExpr(D, Ctx);
+ });
auto Cleanup = llvm::make_scope_exit(
[this] { SxBuilder.setLookupLocalVarExpr(nullptr); });
@@ -1722,6 +1722,18 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
LocalVariableMap::Context LVarCtx;
unsigned CtxIndex;
+ // To update and adjust the context.
+ void updateLocalVarMapCtx(const Stmt *S) {
+ LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
+ // The lookup closure needs to be reconstructed with the refreshed LVarCtx.
+ Analyzer->SxBuilder.setLookupLocalVarExpr(
+ [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * {
+ if (!Analyzer->Handler.issueBetaWarnings())
+ return nullptr;
+ return Analyzer->LocalVarMap.lookupExpr(D, Ctx);
+ });
+ }
+
// helper functions
void checkAccess(const Expr *Exp, AccessKind AK,
@@ -1746,15 +1758,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
const FactSet &FunctionExitFSet)
: ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
- CtxIndex(Info.EntryIndex) {
- Analyzer->SxBuilder.setLookupLocalVarExpr(
- [this](const NamedDecl *D) -> const Expr * {
- if (!Analyzer->Handler.issueBetaWarnings())
- return nullptr;
- auto Ctx = LVarCtx;
- return Analyzer->LocalVarMap.lookupExpr(D, Ctx);
- });
- }
+ CtxIndex(Info.EntryIndex) {}
~BuildLockset() { Analyzer->SxBuilder.setLookupLocalVarExpr(nullptr); }
@@ -2259,9 +2263,7 @@ void BuildLockset::VisitBinaryOperator(const BinaryOperator *BO) {
if (!BO->isAssignmentOp())
return;
- // adjust the context
- LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, BO, LVarCtx);
-
+ updateLocalVarMapCtx(BO);
checkAccess(BO->getLHS(), AK_Written);
}
@@ -2307,8 +2309,7 @@ void BuildLockset::examineArguments(const FunctionDecl *FD,
}
void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
- // adjust the context
- LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, Exp, LVarCtx);
+ updateLocalVarMapCtx(Exp);
if (const auto *CE = dyn_cast<CXXMemberCallExpr>(Exp)) {
const auto *ME = dyn_cast<MemberExpr>(CE->getCallee());
@@ -2404,8 +2405,7 @@ static const Expr *UnpackConstruction(const Expr *E) {
}
void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
- // adjust the context
- LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
+ updateLocalVarMapCtx(S);
for (auto *D : S->getDeclGroup()) {
if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index 25ad673b58db6..ef48ae439c5f3 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -248,9 +248,17 @@ til::SExpr *SExprBuilder::translateVariable(const VarDecl *VD,
// defining VD, use its pre-assignment value to break the cycle.
if (VarsBeingTranslated.contains(VD->getCanonicalDecl()))
return new (Arena) til::LiteralPtr(VD);
- VarsBeingTranslated.insert(VD->getCanonicalDecl());
+
+ // The closure captures state that is updated to correctly translate chains of
+ // aliases. Restore it when we are done with recursive translation.
auto Cleanup = llvm::make_scope_exit(
- [&] { VarsBeingTranslated.erase(VD->getCanonicalDecl()); });
+ [&, RestoreClosure =
+ VarsBeingTranslated.empty() ? LookupLocalVarExpr : nullptr] {
+ VarsBeingTranslated.erase(VD->getCanonicalDecl());
+ if (VarsBeingTranslated.empty())
+ LookupLocalVarExpr = RestoreClosure;
+ });
+ VarsBeingTranslated.insert(VD->getCanonicalDecl());
QualType Ty = VD->getType();
if (!VD->isStaticLocal() && Ty->isPointerType()) {
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index ef662b78fb6f1..0e91639a271c5 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -7463,6 +7463,7 @@ void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) {
struct ContainerOfPtr {
Foo *foo_ptr;
+ ContainerOfPtr *next;
};
void testIndirectAccess(ContainerOfPtr *fc) {
@@ -7472,6 +7473,23 @@ void testIndirectAccess(ContainerOfPtr *fc) {
ptr->mu.Unlock();
}
+void testAliasChainUnrelatedReassignment1(ContainerOfPtr *list) {
+ Foo *eb = list->foo_ptr;
+ eb->mu.Lock();
+ list = list->next;
+ eb->data = 42;
+ eb->mu.Unlock();
+}
+
+void testAliasChainUnrelatedReassignment2(ContainerOfPtr *list) {
+ ContainerOfPtr *busyp = list;
+ Foo *eb = busyp->foo_ptr;
+ eb->mu.Lock();
+ busyp = busyp->next;
+ eb->data = 42;
+ eb->mu.Unlock();
+}
+
void testControlFlowDoWhile(Foo *f, int x) {
Foo *ptr = f;
More information about the cfe-commits
mailing list