https://github.com/melver updated https://github.com/llvm/llvm-project/pull/159921
>From ac8aa378015cd4167e5b11de9b11d48aa7732ce2 Mon Sep 17 00:00:00 2001 From: Marco Elver <el...@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 <h...@lst.de> Link: https://lkml.kernel.org/r/20250919140803.ga23...@lst.de --- .../Analysis/Analyses/ThreadSafetyCommon.h | 6 ++- clang/lib/Analysis/ThreadSafety.cpp | 45 ++++++++++--------- clang/lib/Analysis/ThreadSafetyCommon.cpp | 12 ++++- .../SemaCXX/warn-thread-safety-analysis.cpp | 18 ++++++++ 4 files changed, 57 insertions(+), 24 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..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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
