https://github.com/melver created https://github.com/llvm/llvm-project/pull/212221
Backport a5ec12ab9596 and afd49c36f03b which fix false positives in try-lock handling with the Thread Safety Analysis. These problems have existed for a while, but are showing up in larger C codebases that are adopting the Thread Safety Analysis (in particular, the Linux kernel which enables TSA with Clang 23). >From a5ec12ab959650d0d82e25a2b723942b44a744a5 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Wed, 15 Jul 2026 00:35:50 +0200 Subject: [PATCH 1/2] Thread Safety Analysis: Handle statement expressions in try-lock conditions (#209330) Previously, statement expressions (`({ bool b = mu.TryLock(); b; })`) used as try-lock conditions were not supported. Handle StmtExpr in getTrylockCallExpr() by recursively analyzing the last statement of the statement expression. --- clang/lib/Analysis/ThreadSafety.cpp | 5 +++++ clang/test/SemaCXX/warn-thread-safety-analysis.cpp | 14 ++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 099e8590b50e3..70df7731aef7a 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1659,6 +1659,11 @@ const CallExpr* ThreadSafetyAnalyzer::getTrylockCallExpr(const Stmt *Cond, return getTrylockCallExpr(COP->getCond(), C, Negate); } } + } else if (const auto *SE = dyn_cast<StmtExpr>(Cond)) { + if (const auto *CS = SE->getSubStmt(); CS && !CS->body_empty()) { + if (const auto *E = dyn_cast<Expr>(CS->body_back())) + return getTrylockCallExpr(E, C, Negate); + } } return nullptr; } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index d70d5f99cb515..9f11fdcf78f01 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2008,6 +2008,13 @@ struct TestTryLock { } } + void foo3_stmtexpr() { + if (({ bool b = mu.TryLock(); b; })) { + a = 3; + mu.Unlock(); + } + } + void foo3_builtin_expect() { bool b = mu.TryLock(); if (__builtin_expect(b, true)) { @@ -2016,6 +2023,13 @@ struct TestTryLock { } } + void foo3_builtin_expect_stmtexpr() { + if (({ bool b = mu.TryLock(); __builtin_expect(b, true); })) { + a = 3; + mu.Unlock(); + } + } + void foo4() { bool b = mu.TryLock(); if (!b) return; >From afd49c36f03bf1074344212fe5c53f7686a07b08 Mon Sep 17 00:00:00 2001 From: Timothy Day <[email protected]> Date: Wed, 22 Jul 2026 05:22:52 -0400 Subject: [PATCH 2/2] Thread Safety Analysis: Don't warn at joins that re-branch on a try-lock result (#209796) Previously, when the result of a try-lock call is branched on more than once, the paths between the branches would disagree on whether the capability is held while remaining consistent at each branch. The analysis then gave a false positive warning at the intermediate join: mutex 'lock' is not held on every path through here Create getTerminatorTrylockCall() helper from getEdgeLockset(); if the terminator of a block branches on the result of a call to a try_acquire_capability-function (perhaps negated or stored in a local variable), this helper returns that call and its callee. Use this new helper in getTerminatorTrylockCaps(), which will return the capabilities acquired by a trylock; feed these capabilites to intersectAndWarn() during a branch join, in order to avoid false positives. Soundness is preserved because intersectAndWarn() still removes the capabilities from the join block's entry lockset, ensuring any guarded access inside the join block before the terminator is diagnosed. getEdgeLockset() then re-adds the capability on the success edge. This change resolves the false positive warning reported in [1]. Reported-by: https://lore.kernel.org/linux-fsdevel/[email protected]/ [1] Assisted-by: AI was used a bit to understand the code and to help with testing and validation --- clang/lib/Analysis/ThreadSafety.cpp | 135 +++++++++++++----- clang/test/Sema/warn-thread-safety-analysis.c | 15 ++ .../SemaCXX/warn-thread-safety-analysis.cpp | 54 +++++++ 3 files changed, 172 insertions(+), 32 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 70df7731aef7a..1aec2e0226b7f 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1257,16 +1257,26 @@ class ThreadSafetyAnalyzer { const CallExpr* getTrylockCallExpr(const Stmt *Cond, LocalVarContext C, bool &Negate); + using TerminatorTrylockCall = + std::tuple<const CallExpr *, const NamedDecl *, + std::optional<llvm::scope_exit<std::function<void()>>>>; + + TerminatorTrylockCall getTerminatorTrylockCall(const CFGBlock *Block, + bool &Negate); + void getEdgeLockset(FactSet &Result, const FactSet &ExitSet, const CFGBlock* PredBlock, const CFGBlock *CurrBlock); + void getTerminatorTrylockCaps(const CFGBlock *Block, CapExprSet &Caps); + bool join(const FactEntry &A, const FactEntry &B, SourceLocation JoinLoc, LockErrorKind EntryLEK); void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet, SourceLocation JoinLoc, LockErrorKind EntryLEK, - LockErrorKind ExitLEK); + LockErrorKind ExitLEK, + const CapExprSet *TrylockRebranchCaps = nullptr); void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet, SourceLocation JoinLoc, LockErrorKind LEK) { @@ -1668,40 +1678,57 @@ const CallExpr* ThreadSafetyAnalyzer::getTrylockCallExpr(const Stmt *Cond, return nullptr; } -/// Find the lockset that holds on the edge between PredBlock -/// and CurrBlock. The edge set is the exit set of PredBlock (passed -/// as the ExitSet parameter) plus any trylocks, which are conditionally held. -void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result, - const FactSet &ExitSet, - const CFGBlock *PredBlock, - const CFGBlock *CurrBlock) { - Result = ExitSet; - - const Stmt *Cond = PredBlock->getTerminatorCondition(); +/// If the terminator of \p Block branches on the result of a call to a +/// function annotated with try_acquire_capability (possibly negated or stored +/// in a local variable), return that call and its callee. \p Negate is set if +/// the branch tests the negated result of the call. In beta mode, this leaves +/// the local variable lookup closure of SExprBuilder installed so that callers +/// can translate the callee's attribute expressions +ThreadSafetyAnalyzer::TerminatorTrylockCall +ThreadSafetyAnalyzer::getTerminatorTrylockCall(const CFGBlock *Block, + bool &Negate) { + assert(!Negate && "Must be called with Negate initialized to false"); + + const Stmt *Cond = Block->getTerminatorCondition(); // We don't acquire try-locks on ?: branches, only when its result is used. - if (!Cond || isa<ConditionalOperator>(PredBlock->getTerminatorStmt())) - return; + if (!Cond || isa<ConditionalOperator>(Block->getTerminatorStmt())) + return {}; - bool Negate = false; - const CFGBlockInfo *PredBlockInfo = &BlockInfo[PredBlock->getBlockID()]; - const LocalVarContext &LVarCtx = PredBlockInfo->ExitContext; + const LocalVarContext &LVarCtx = BlockInfo[Block->getBlockID()].ExitContext; + std::optional<llvm::scope_exit<std::function<void()>>> Cleanup; 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); }); + Cleanup.emplace([this] { SxBuilder.setLookupLocalVarExpr(nullptr); }); } - llvm::scope_exit Cleanup( - [this] { SxBuilder.setLookupLocalVarExpr(nullptr); }); const auto *Exp = getTrylockCallExpr(Cond, LVarCtx, Negate); if (!Exp) - return; + return {}; auto *FunDecl = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl()); if (!FunDecl || !FunDecl->hasAttr<TryAcquireCapabilityAttr>()) + return {}; + + return {Exp, FunDecl, std::move(Cleanup)}; +} + +/// Find the lockset that holds on the edge between PredBlock +/// and CurrBlock. The edge set is the exit set of PredBlock (passed +/// as the ExitSet parameter) plus any trylocks, which are conditionally held. +void ThreadSafetyAnalyzer::getEdgeLockset(FactSet &Result, + const FactSet &ExitSet, + const CFGBlock *PredBlock, + const CFGBlock *CurrBlock) { + Result = ExitSet; + + bool Negate = false; + auto [Exp, FunDecl, Cleanup] = getTerminatorTrylockCall(PredBlock, Negate); + if (!Exp) return; CapExprSet ExclusiveLocksToAdd; @@ -1723,6 +1750,20 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result, LK_Shared, Loc)); } +/// If the terminator of \p Block branches on the result of a try-lock call +/// (possibly stored in a local variable), add the capabilities acquired by +/// that call to \p Caps. +void ThreadSafetyAnalyzer::getTerminatorTrylockCaps(const CFGBlock *Block, + CapExprSet &Caps) { + bool Negate = false; + auto [Exp, FunDecl, Cleanup] = getTerminatorTrylockCall(Block, Negate); + if (!Exp) + return; + + for (const auto *Attr : FunDecl->specific_attrs<TryAcquireCapabilityAttr>()) + getMutexIDs(Caps, Attr, Exp, FunDecl); +} + namespace { /// We use this class to visit different types of expressions in @@ -2592,13 +2633,24 @@ bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B, /// \param JoinLoc The location of the join point for error reporting /// \param EntryLEK The warning if a mutex is missing from \p EntrySet. /// \param ExitLEK The warning if a mutex is missing from \p ExitSet. -void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet, - const FactSet &ExitSet, - SourceLocation JoinLoc, - LockErrorKind EntryLEK, - LockErrorKind ExitLEK) { +/// \param TrylockRebranchCaps Capabilities acquired by a try-lock whose result +/// the joining block's terminator branches on; differences in these are not +/// diagnosed because the paths re-diverge at the terminator (but they are +/// still removed from the intersection, and conditionally re-added on the +/// outgoing edges by getEdgeLockset()). +void ThreadSafetyAnalyzer::intersectAndWarn( + FactSet &EntrySet, const FactSet &ExitSet, SourceLocation JoinLoc, + LockErrorKind EntryLEK, LockErrorKind ExitLEK, + const CapExprSet *TrylockRebranchCaps) { FactSet EntrySetOrig = EntrySet; + auto IsTrylockRebranched = [TrylockRebranchCaps](const FactEntry &FE) { + return TrylockRebranchCaps && + llvm::any_of(*TrylockRebranchCaps, [&FE](const CapabilityExpr &CE) { + return !CE.shouldIgnore() && FE.matches(CE); + }); + }; + // Find locks in ExitSet that conflict or are not in EntrySet, and warn. for (const auto &Fact : ExitSet) { const FactEntry &ExitFact = FactMan[Fact]; @@ -2607,7 +2659,8 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet, if (EntryIt != EntrySet.end()) { if (join(FactMan[*EntryIt], ExitFact, JoinLoc, EntryLEK)) *EntryIt = Fact; - } else if (!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) { + } else if ((!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) && + !IsTrylockRebranched(ExitFact)) { ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc, EntryLEK, Handler); } @@ -2619,8 +2672,9 @@ void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet, const FactEntry *ExitFact = ExitSet.findLock(FactMan, *EntryFact); if (!ExitFact) { - if (!EntryFact->managed() || ExitLEK == LEK_LockedSomeLoopIterations || - ExitLEK == LEK_NotLockedAtEndOfFunction) + if ((!EntryFact->managed() || ExitLEK == LEK_LockedSomeLoopIterations || + ExitLEK == LEK_NotLockedAtEndOfFunction) && + !IsTrylockRebranched(*EntryFact)) EntryFact->handleRemovalFromIntersection(EntrySetOrig, FactMan, JoinLoc, ExitLEK, Handler); if (ExitLEK == LEK_LockedSomePredecessors) @@ -2832,6 +2886,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { // union because the real error is probably that we forgot to unlock M on // all code paths. bool LocksetInitialized = false; + // Capabilities acquired by a try-lock whose result this block's + // terminator branches on. Computed lazily on the first join. + CapExprSet TerminatorTrylockCaps; + bool TerminatorTrylockCapsComputed = false; for (CFGBlock::const_pred_iterator PI = CurrBlock->pred_begin(), PE = CurrBlock->pred_end(); PI != PE; ++PI) { // if *PI -> CurrBlock is a back edge @@ -2858,11 +2916,24 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { // Surprisingly 'continue' doesn't always produce back edges, because // the CFG has empty "transition" blocks where they meet with the end // of the regular loop body. We still want to diagnose them as loop. - intersectAndWarn( - CurrBlockInfo->EntrySet, PrevLockset, CurrBlockInfo->EntryLoc, - isa_and_nonnull<ContinueStmt>((*PI)->getTerminatorStmt()) - ? LEK_LockedSomeLoopIterations - : LEK_LockedSomePredecessors); + if (isa_and_nonnull<ContinueStmt>((*PI)->getTerminatorStmt())) { + // Loop join: warn on locks held for only some iterations. + intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset, + CurrBlockInfo->EntryLoc, + LEK_LockedSomeLoopIterations, + LEK_LockedSomeLoopIterations, nullptr); + } else { + // Branch join: a lockset difference is harmless if the terminator + // re-branches on the try-lock result. + if (!TerminatorTrylockCapsComputed) { + // Compute once; the result depends only on CurrBlock, not on *PI. + getTerminatorTrylockCaps(CurrBlock, TerminatorTrylockCaps); + TerminatorTrylockCapsComputed = true; + } + intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset, + CurrBlockInfo->EntryLoc, LEK_LockedSomePredecessors, + LEK_LockedSomePredecessors, &TerminatorTrylockCaps); + } } } diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index a0e9e7ce724cc..6613f65e4b359 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -61,6 +61,7 @@ struct A { }; // Declare mutex lock/unlock functions. +int mutex_exclusive_trylock(struct Mutex *mu) EXCLUSIVE_TRYLOCK_FUNCTION(1, mu); void mutex_exclusive_lock(struct Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu); void mutex_shared_lock(struct Mutex *mu) SHARED_LOCK_FUNCTION(mu); void mutex_unlock(struct Mutex *mu) UNLOCK_FUNCTION(mu); @@ -339,6 +340,20 @@ void test_bdev_ops_fail(struct BDevOps *ops, struct BDev *bdev) { ops->unlock(bdev); // expected-warning {{releasing mutex 'bdev->lock' that was not held}} } +// Test unusual trylock patterns +void do_some_work(void); +int work_data GUARDED_BY(mu1); + +void test_trylock_conditional(void) { + if (({ int do_work = !!(!mutex_exclusive_trylock(&mu1)); + if (__builtin_expect(do_work, 0)) + do_some_work(); + __builtin_expect(do_work, 0); })) + return; + work_data = 1; + mutex_unlock(&mu1); +} + // We had a problem where we'd skip all attributes that follow a late-parsed // attribute in a single __attribute__. void run(void) __attribute__((guarded_by(mu1), guarded_by(mu1))); // expected-warning 2{{only applies to non-static data members and global variables}} diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 9f11fdcf78f01..7510a413cf330 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1977,6 +1977,7 @@ namespace TryLockTest { struct TestTryLock { Mutex mu; + Mutex mu2; int a GUARDED_BY(mu); bool cond; @@ -2030,6 +2031,59 @@ struct TestTryLock { } } + void foo3_double_branch() { + bool failed = !mu.TryLock(); + if (failed) + cond = true; // does not return; rejoins the success path + if (failed) // paths re-diverge consistently here, so no warning at + return; // the preceding join + a = 3; + mu.Unlock(); + } + + // Mimic the logic of the previous test, but in a statement expression. + // This pattern is typically found in macros. + void foo3_double_branch_statement_expression() { + if (({ bool failed = !mu.TryLock(); if (failed) cond = true; failed; })) + return; + a = 3; + mu.Unlock(); + } + + void foo3_no_rebranch_at_join() { + bool failed = !mu.TryLock(); // expected-note {{mutex acquired here}} + if (failed) + cond = true; + // Lock state genuinely differs at this join: nothing re-branches on + // 'failed' here, so the warning must be retained. + a = 3; // expected-warning {{mutex 'mu' is not held on every path through here}} \ + // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}} + mu.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}} + } + + void foo3_rebranch_after_reassign() { + bool failed = !mu.TryLock(); // expected-note {{mutex acquired here}} + if (failed) + cond = true; + failed = true; // expected-warning {{mutex 'mu' is not held on every path through here}} + if (failed) // no longer the try-lock result: the join above must warn + return; + a = 3; // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}} + mu.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}} + } + + void foo3_rebranch_other_mutex_still_warns() { + bool failed = !mu.TryLock(); + if (failed) + mu2.Lock(); // expected-note {{mutex acquired here}} + // The re-branch on 'failed' only suppresses the warning for 'mu', the + // capability the try-lock acquires; 'mu2' must still warn at the join. + if (failed) // expected-warning {{mutex 'mu2' is not held on every path through here}} + return; + a = 3; + mu.Unlock(); + } + void foo4() { bool b = mu.TryLock(); if (!b) return; _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
