https://github.com/ziqingluo-90 updated https://github.com/llvm/llvm-project/pull/178952
>From b5da6954cd581ee649c0280ecc14f23dc748ccfe Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Thu, 29 Jan 2026 18:59:23 -0800 Subject: [PATCH 1/3] [Thread Analysis] Fix a bug in context update in alias-analysis Previously, in 'updateLocalVarMapCtx', context was updated to the one immediately after the provided statement 'S'. It is incorrect, because 'S' hasn't been processed at that point. This issue could result in false positives. For example, ``` void f(Lock_t* F) { Lock_t* L = F; // 'L' aliases with 'F' L->Lock(); // 'L' holds the lock // Before the fix, the analyzer saw the definition of 'L' being cleared before 'L' was unlocked. unlock(&L); // unlock (*L) } ``` This commit fixes the issue. In addition, this commit adds `updateLocalVarMapCtx` for cleanup functions, which was missed before. rdar://169236809 --- clang/lib/Analysis/ThreadSafety.cpp | 20 ++++++++++++------- .../SemaCXX/warn-thread-safety-analysis.cpp | 16 ++++++++++++++- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 4ed3ef1993eda..c0a165d482855 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1727,20 +1727,25 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> { FactSet FSet; // The fact set for the function on exit. const FactSet &FunctionExitFSet; + // Use `LVarCtx` to keep track of the context at each program point, which + // will be used to translate DREs (by `SExprBuilder::translateDeclRefExpr`) + // to their canonical definitions: LocalVariableMap::Context LVarCtx; unsigned CtxIndex; // To update and adjust the context. void updateLocalVarMapCtx(const Stmt *S) { + if (Analyzer->Handler.issueBetaWarnings()) { + // The lookup closure needs to be reconstructed with the LVarCtx at the + // point right before 'S'. This is the context used for visiting 'S'. + Analyzer->SxBuilder.setLookupLocalVarExpr( + [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * { + return Analyzer->LocalVarMap.lookupExpr(D, Ctx); + }); + } if (S) + // Update the context to the point right after '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 @@ -2842,6 +2847,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::CleanupFunction: { const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>(); + LocksetBuilder.updateLocalVarMapCtx(nullptr); LocksetBuilder.handleCall( /*Exp=*/nullptr, CF.getFunctionDecl(), SxBuilder.translateVariable(CF.getVarDecl(), nullptr), diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index e57299e93aa48..b3266a5f2ac7e 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7493,7 +7493,21 @@ void testPointerAliasEscapeMultiple(Foo *F) { escapeAliasMultiple(&L, &L, &Fp); Fp->mu.Unlock(); // expected-warning{{releasing mutex 'Fp->mu' that was not held}} } // expected-warning{{mutex 'F->mu' is still held at the end of function}} - + +void unlockFooWithEscapablePointer(Foo **Fp) EXCLUSIVE_UNLOCK_FUNCTION((*Fp)->mu); +void testEscapeInvalidationHappensRightAfterTheCall(Foo* F) { + Foo* L; + L = F; + L->mu.Lock(); + // Release the lock hold by 'L' before clear its definition. + unlockFooWithEscapablePointer(&L); +} + +void testCleanUpFunctionWithLocalVarUpdated(Foo* F) { + F->mu.Lock(); + Foo * __attribute__((unused, cleanup(unlockFooWithEscapablePointer))) L = F; +} + void testPointerAliasTryLock1() { Foo *ptr = returnsFoo(); if (ptr->mu.TryLock()) { >From 6a5d3de894fc3447ba651642267ea880082b6357 Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Wed, 4 Feb 2026 18:43:28 -0800 Subject: [PATCH 2/3] A simpler and better fix --- clang/lib/Analysis/ThreadSafety.cpp | 37 ++++++++----------- .../SemaCXX/warn-thread-safety-analysis.cpp | 12 ++++++ 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index c0a165d482855..8b14b6d6cf539 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1728,24 +1728,23 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> { // The fact set for the function on exit. const FactSet &FunctionExitFSet; // Use `LVarCtx` to keep track of the context at each program point, which - // will be used to translate DREs (by `SExprBuilder::translateDeclRefExpr`) + // will be used to translate variables (by `SExprBuilder::translateVariable`) // to their canonical definitions: LocalVariableMap::Context LVarCtx; unsigned CtxIndex; - // To update and adjust the context. + // To update the context used in attr-expr translation. If `S` is non-null, + // the context is updated to the program point right after 'S'. void updateLocalVarMapCtx(const Stmt *S) { - if (Analyzer->Handler.issueBetaWarnings()) { - // The lookup closure needs to be reconstructed with the LVarCtx at the - // point right before 'S'. This is the context used for visiting 'S'. - Analyzer->SxBuilder.setLookupLocalVarExpr( - [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * { - return Analyzer->LocalVarMap.lookupExpr(D, Ctx); - }); - } if (S) - // Update the context to the point right after '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 @@ -2278,9 +2277,9 @@ void BuildLockset::VisitUnaryOperator(const UnaryOperator *UO) { void BuildLockset::VisitBinaryOperator(const BinaryOperator *BO) { if (!BO->isAssignmentOp()) return; - - updateLocalVarMapCtx(BO); + checkAccess(BO->getLHS(), AK_Written); + updateLocalVarMapCtx(BO); } /// Whenever we do an LValue to Rvalue cast, we are reading a variable and @@ -2325,8 +2324,6 @@ void BuildLockset::examineArguments(const FunctionDecl *FD, } void BuildLockset::VisitCallExpr(const CallExpr *Exp) { - updateLocalVarMapCtx(Exp); - if (const auto *CE = dyn_cast<CXXMemberCallExpr>(Exp)) { const auto *ME = dyn_cast<MemberExpr>(CE->getCallee()); // ME can be null when calling a method pointer @@ -2390,9 +2387,9 @@ void BuildLockset::VisitCallExpr(const CallExpr *Exp) { } auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl()); - if (!D) - return; - handleCall(Exp, D); + if (D) + handleCall(Exp, D); + updateLocalVarMapCtx(Exp); } void BuildLockset::VisitCXXConstructExpr(const CXXConstructExpr *Exp) { @@ -2421,8 +2418,6 @@ static const Expr *UnpackConstruction(const Expr *E) { } void BuildLockset::VisitDeclStmt(const DeclStmt *S) { - updateLocalVarMapCtx(S); - for (auto *D : S->getDeclGroup()) { if (auto *VD = dyn_cast_or_null<VarDecl>(D)) { const Expr *E = VD->getInit(); @@ -2442,6 +2437,7 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) { } } } + updateLocalVarMapCtx(S); } void BuildLockset::VisitMaterializeTemporaryExpr( @@ -2847,7 +2843,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { case CFGElement::CleanupFunction: { const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>(); - LocksetBuilder.updateLocalVarMapCtx(nullptr); LocksetBuilder.handleCall( /*Exp=*/nullptr, CF.getFunctionDecl(), SxBuilder.translateVariable(CF.getVarDecl(), nullptr), diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index b3266a5f2ac7e..5bc726a09f04d 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7503,6 +7503,18 @@ void testEscapeInvalidationHappensRightAfterTheCall(Foo* F) { unlockFooWithEscapablePointer(&L); } + +void testEscapeInvalidationHappensRightAfterTheCtorCall(Foo* F) { + Foo* L = F; + MutexLock ScopeLock(&L->mu); + + struct { + int DataMember GUARDED_BY(F->mu); + } Data; + + Data.DataMember = 0; +} + void testCleanUpFunctionWithLocalVarUpdated(Foo* F) { F->mu.Lock(); Foo * __attribute__((unused, cleanup(unlockFooWithEscapablePointer))) L = F; >From 0a9499b389275e4af375cc2b004cd929ea8eff73 Mon Sep 17 00:00:00 2001 From: Ziqing Luo <[email protected]> Date: Wed, 4 Feb 2026 18:51:35 -0800 Subject: [PATCH 3/3] fix clang-format --- clang/lib/Analysis/ThreadSafety.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 8b14b6d6cf539..901624acd6f75 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -2277,7 +2277,6 @@ void BuildLockset::VisitUnaryOperator(const UnaryOperator *UO) { void BuildLockset::VisitBinaryOperator(const BinaryOperator *BO) { if (!BO->isAssignmentOp()) return; - checkAccess(BO->getLHS(), AK_Written); updateLocalVarMapCtx(BO); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
