llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Marco Elver (melver) <details> <summary>Changes</summary> 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). --- Full diff: https://github.com/llvm/llvm-project/pull/212221.diff 3 Files Affected: - (modified) clang/lib/Analysis/ThreadSafety.cpp (+108-32) - (modified) clang/test/Sema/warn-thread-safety-analysis.c (+15) - (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+68) ``````````diff diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 099e8590b50e3..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) { @@ -1659,44 +1669,66 @@ 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; } -/// 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; @@ -1718,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 @@ -2587,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]; @@ -2602,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); } @@ -2614,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) @@ -2827,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 @@ -2853,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 d70d5f99cb515..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; @@ -2008,6 +2009,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 +2024,66 @@ struct TestTryLock { } } + void foo3_builtin_expect_stmtexpr() { + if (({ bool b = mu.TryLock(); __builtin_expect(b, true); })) { + a = 3; + mu.Unlock(); + } + } + + 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; `````````` </details> https://github.com/llvm/llvm-project/pull/212221 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
