[clang] Thread Safety Analysis: Fix recursive capability alias resolution (PR #159921)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 22 00:27:36 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-analysis
Author: Marco Elver (melver)
<details>
<summary>Changes</summary>
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@<!-- -->lst.de>
Link: https://lkml.kernel.org/r/20250919140803.GA23745@<!-- -->lst.de
---
Full diff: https://github.com/llvm/llvm-project/pull/159921.diff
4 Files Affected:
- (modified) clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h (+5-1)
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+24-21)
- (modified) clang/lib/Analysis/ThreadSafetyCommon.cpp (+10-2)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+18)
``````````diff
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..d19f86a2223d8 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1668,13 +1668,13 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
const CFGBlockInfo *PredBlockInfo = &BlockInfo[PredBlock->getBlockID()];
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);
- });
+ if (Handler.issueBetaWarnings()) {
+ // Temporarily set the lookup context for SExprBuilder.
+ SxBuilder.setLookupLocalVarExpr(
+ [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * {
+ return LocalVarMap.lookupExpr(D, Ctx);
+ });
+ }
auto Cleanup = llvm::make_scope_exit(
[this] { SxBuilder.setLookupLocalVarExpr(nullptr); });
@@ -1722,6 +1722,19 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
LocalVariableMap::Context LVarCtx;
unsigned CtxIndex;
+ // To update and adjust the context.
+ void updateLocalVarMapCtx(const Stmt *S) {
+ if (S)
+ LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
+ if (!Analyzer->Handler.issueBetaWarnings())
+ return;
+ // The lookup closure needs to be reconstructed with the refreshed LVarCtx.
+ Analyzer->SxBuilder.setLookupLocalVarExpr(
+ [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * {
+ return Analyzer->LocalVarMap.lookupExpr(D, Ctx);
+ });
+ }
+
// helper functions
void checkAccess(const Expr *Exp, AccessKind AK,
@@ -1747,13 +1760,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
: 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);
- });
+ updateLocalVarMapCtx(nullptr);
}
~BuildLockset() { Analyzer->SxBuilder.setLookupLocalVarExpr(nullptr); }
@@ -2259,9 +2266,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 +2312,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 +2408,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;
``````````
</details>
https://github.com/llvm/llvm-project/pull/159921
More information about the cfe-commits
mailing list