https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/211973
>From 58733e13c9d93706dae05d205fe9e16fd4ff4962 Mon Sep 17 00:00:00 2001 From: Jameson Nash <[email protected]> Date: Fri, 24 Jul 2026 18:55:22 +0000 Subject: [PATCH] Thread Safety Analysis: Honor try-lock branches of a void conditional operator Try-locks are not acquired on the arms of a ?:, because the arms would then disagree about whether the capability is held and warn where they join. That reasoning assumes the ?: produces a result that is branched on later, which is where the acquisition is handled instead. A void ?: has no such result. Its branch is all there is to interpret, and its arms might not join at all because one of them does not return (this is how glibc before 2.32 spells `assert()`). So asserting that a try-lock succeeded left the capability unheld on every path, and a function documented to acquire it warned that it did not, if written like so: void lock(void) ACQUIRE(mu) { int got = trylock(); assert(got); // warning: expecting mutex 'mu' to be held at the end of function } Only bail out for a ?: that has a result to be used, so that a void ?: acquires on its branch like any other terminator (for example, including `&&` and `||`). Assisted-by: Claude Opus 5 (1M context) <[email protected]> --- clang/lib/Analysis/ThreadSafety.cpp | 9 +++++-- clang/test/Sema/warn-thread-safety-analysis.c | 19 +++++++++++++++ .../SemaCXX/warn-thread-safety-analysis.cpp | 24 +++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index fe94910e905a3..b3d7cb238bb46 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1691,10 +1691,15 @@ ThreadSafetyAnalyzer::getTerminatorTrylockCall(const CFGBlock *Block, 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>(Block->getTerminatorStmt())) + if (!Cond) return {}; + // We don't acquire try-locks on ?: branches, except when its result is used. + if (const auto *COp = + dyn_cast_if_present<ConditionalOperator>(Block->getTerminatorStmt())) + if (!COp->getType()->isVoidType()) + return {}; + const LocalVarContext &LVarCtx = BlockInfo[Block->getBlockID()].ExitContext; std::optional<llvm::scope_exit<std::function<void()>>> Cleanup; diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index 6613f65e4b359..d9ebc2a91dadc 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -354,6 +354,25 @@ void test_trylock_conditional(void) { mutex_unlock(&mu1); } +// How glibc before 2.32 spells assert(): a void conditional operator whose +// false arm does not return. There is no result to branch on later, so the +// try-lock is acquired on the branch itself. +void assert_fail(void) __attribute__((noreturn)); +#define assert_trylock(e) ((e) ? (void)0 : assert_fail()) + +void test_trylock_void_conditional(void) { + assert_trylock(mutex_exclusive_trylock(&mu1)); + work_data = 1; + mutex_unlock(&mu1); +} + +void test_trylock_void_conditional_via_var(void) { + int got = mutex_exclusive_trylock(&mu1); + assert_trylock(got); + 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 f82da827bd068..5fc4bb4d7cd0a 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -2197,6 +2197,30 @@ struct TestTryLock { if (mu.TryLock() ? 0 : 1) // expected-note{{mutex acquired here}} mu.Unlock(); // expected-warning{{releasing mutex 'mu' that was not held}} } // expected-warning{{mutex 'mu' is not held on every path through here}} + + // A void conditional operator has no result to branch on later, so unlike + // foo13-foo15 the branch itself is honored. This is how glibc before 2.32 + // spells assert(). + void foo16() { + mu.TryLock() ? static_cast<void>(0) : fail(); + a = 3; + mu.Unlock(); + } + + void foo17() { + !mu.TryLock() ? fail() : static_cast<void>(0); + a = 3; + mu.Unlock(); + } + + // Both arms return here, so the join disagrees -- as it would for an if. + void foo18() { + mu.TryLock() ? static_cast<void>(0) : static_cast<void>(0); // expected-note{{mutex acquired here}} \ + expected-warning{{mutex 'mu' is not held on every path through here}} + mu.Unlock(); // expected-warning{{releasing mutex 'mu' that was not held}} + } + + static void fail() __attribute__((noreturn)); }; // end TestTrylock } // end namespace TrylockTest _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
