[llvm-branch-commits] [clang] release/23.x: Thread Safety Analysis: Cherry-pick fixes for try-lock handling (PR #212221)
via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Mon Jul 27 03:39:54 PDT 2026
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
More information about the llvm-branch-commits
mailing list