[llvm-branch-commits] [clang] Thread Safety Analysis: Track try-acquired capabilities as a ternary try-held state (PR #220635)
Jameson Nash via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Wed Sep 2 09:08:49 PDT 2026
https://github.com/vtjnash created https://github.com/llvm/llvm-project/pull/220635
Context: I was a bit frustrated by using conditional capabilities in
real code, since it was giving both incorrect and incomplete analysis
results. There is already this really nice fact manager here;
conditional modelling was simply ignoring it and essentially
reimplementing a simplified duplicate model on top of it which tried to
re-derive facts and inject them into the real model. But the underlying
model already seemed really close to be able to hold these facts too,
instead of leaving them invisible and wrongly diagnosed. This PR is the
beginning of a series of about 10 PRs to reduce those correctness and
completeness gaps. Claude is already happy with the state of those other
branches, but I'm not happy with them yet, so I'll drip those out as I
get time to polish them up. Throughout this series, I have tried to
often suppress new warnings except when passing `-Wthread-safety-beta`,
so that users can opt-in explicitly to the more complete and accurate
checks. The stack of branches is:
1. (users/vtjnash/tsa-tryheld-regardless-of-result) Diagnose a try-acquire that acquires a capability regardless of its result
2. (users/vtjnash/tsa-tryheld-never-checked) Diagnose a try-acquire whose result is never checked
3. (users/vtjnash/tsa-tryheld-edge-resolve) Resolve try-acquire facts at every branch on the result
4. (users/vtjnash/tsa-tryheld-stored-results) Resolve branches on stored try-acquire results through merges and phis
5. (users/vtjnash/tsa-tryheld-same-origin-joins) Demote same-origin joins silently
6. (users/vtjnash/tsa-tryheld-value-precise) Resolve try-acquire results by exact value, not just truthiness
7. (users/vtjnash/tsa-tryheld-cond-operator) Resolve try-acquire results merged by a conditional operator
8. (users/vtjnash/tsa-tryheld-scoped) Fold scoped lockables into the try-held model
9. (users/vtjnash/tsa-tryheld-elidable-copies) Look through elidable copies of scoped lockables
A capability now has a ternary per-program-point state -- not-held,
try-held, or held -- instead of try-acquires being invisible until a
branch on their result. A call to a try_acquire_capability function
immediately records a try-held fact anchored to the CallExpr, with the
attributes' capabilities translated in the call's own context and
recorded there; a branch on the call's result resolves each fact against
that record as before, promoting it to held on its attribute's success
edge and removing it on the other, so attributes with mixed success
values resolve per capability. Leaving the try-held state any other way
is diagnosed with a warning (new "may hold"-flavors of the existing
diagnostics) or handled conservatively.
Design details:
* Capabilities are recorded once, at the call, in its own context
(before the lockset walk): a branch never re-translates attribute
arguments, so an intervening reassignment cannot misresolve facts.
* The acquisition checks (negative capabilities, acquired_before/
acquired_after ordering) run once, at the call; the success edge
consumes the negative capability.
* A held/try-held join difference is ignored only when the terminator
re-branches on the same call that created both facts. Origins merged
from different calls, or overwritten by an acquire or assert, are
cleared: such facts are conservatively never resolved by a branch
again.
* Releasing a try-held capability warns "may not be held" and leaves a
negative fact. Try-held never satisfies REQUIRES/GUARDED_BY, and
still violates LOCKS_EXCLUDED and negative requirements. Losing track
of the fact at a join or the end of the function is deliberately not
yet diagnosed (a follow-up commit adds that under
-Wthread-safety-beta).
* An acquire over a try-held capability warns ("may already be held"; asserts
exempt); a reentrant capability deepens instead, keeping its
conditional top level. A try-acquire deepens a definite hold; over a
merely try-held fact it is diagnosed and not tracked (can only track
one fact per capability at a time).
* Success values decode by constant evaluation, defaulting to false,
unchanged from before.
* Stored results (`if ((b = mu.TryLock()))`, short-circuit forms) now
resolve like a branch on the call itself.
* A scoped guard's explicit unlock of a try-held mutex diagnoses like a
release; its definite re-acquire only deepens the fact. Scoped
lockables are otherwise not yet tracked conditionally.
* A join of unequal reentrancy depths warns once, then keeps the fact
that guarantees more (a try-held top level counts half) to minimize
follow-on warnings, following the same logic as before. A mixed
held/try-held pair keeps the try-held side, after warning.
* NFC: checkAcquiredCapability split out of addLock; "may" `%select{}`
flavors on the unlock/double-lock/excludes warnings;
`FactSet::isEmpty(FactManager&)` renamed `holdsNoCapability`.
Co-Authored-By: Claude Fable 5 <noreply at anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply at anthropic.com>
---
<sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub>
>From cc633370820cc6c6aa3422ea9bedbd380caa594d Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash at gmail.com>
Date: Tue, 1 Sep 2026 15:16:58 +0000
Subject: [PATCH] Thread Safety Analysis: Track try-acquired capabilities as a
ternary try-held state
Context: I was a bit frustrated by using conditional capabilities in
real code, since it was giving both incorrect and incomplete analysis
results. There is already this really nice fact manager here;
conditional modelling was simply ignoring it and essentially
reimplementing a simplified duplicate model on top of it which tried to
re-derive facts and inject them into the real model. But the underlying
model already seemed really close to be able to hold these facts too,
instead of leaving them invisible and wrongly diagnosed. This PR is the
beginning of a series of about 10 PRs to reduce those correctness and
completeness gaps. Claude is already happy with the state of those other
branches, but I'm not happy with them yet, so I'll drip those out as I
get time to polish them up. Throughout this series, I have tried to
often suppress new warnings except when passing `-Wthread-safety-beta`,
so that users can opt-in explicitly to the more complete and accurate
checks. The stack of branches is:
1. (users/vtjnash/tsa-tryheld-regardless-of-result) Diagnose a try-acquire that acquires a capability regardless of its result
2. (users/vtjnash/tsa-tryheld-never-checked) Diagnose a try-acquire whose result is never checked
3. (users/vtjnash/tsa-tryheld-edge-resolve) Resolve try-acquire facts at every branch on the result
4. (users/vtjnash/tsa-tryheld-stored-results) Resolve branches on stored try-acquire results through merges and phis
5. (users/vtjnash/tsa-tryheld-same-origin-joins) Demote same-origin joins silently
6. (users/vtjnash/tsa-tryheld-value-precise) Resolve try-acquire results by exact value, not just truthiness
7. (users/vtjnash/tsa-tryheld-cond-operator) Resolve try-acquire results merged by a conditional operator
8. (users/vtjnash/tsa-tryheld-scoped) Fold scoped lockables into the try-held model
9. (users/vtjnash/tsa-tryheld-elidable-copies) Look through elidable copies of scoped lockables
A capability now has a ternary per-program-point state -- not-held,
try-held, or held -- instead of try-acquires being invisible until a
branch on their result. A call to a try_acquire_capability function
immediately records a try-held fact anchored to the CallExpr, with the
attributes' capabilities translated in the call's own context and
recorded there; a branch on the call's result resolves each fact against
that record as before, promoting it to held on its attribute's success
edge and removing it on the other, so attributes with mixed success
values resolve per capability. Leaving the try-held state any other way
is diagnosed with a warning (new "may hold"-flavors of the existing
diagnostics) or handled conservatively.
Design details:
* Capabilities are recorded once, at the call, in its own context
(before the lockset walk): a branch never re-translates attribute
arguments, so an intervening reassignment cannot misresolve facts.
* The acquisition checks (negative capabilities, acquired_before/
acquired_after ordering) run once, at the call; the success edge
consumes the negative capability.
* A held/try-held join difference is ignored only when the terminator
re-branches on the same call that created both facts. Origins merged
from different calls, or overwritten by an acquire or assert, are
cleared: such facts are conservatively never resolved by a branch
again.
* Releasing a try-held capability warns "may not be held" and leaves a
negative fact. Try-held never satisfies REQUIRES/GUARDED_BY, and
still violates LOCKS_EXCLUDED and negative requirements. Losing track
of the fact at a join or the end of the function is deliberately not
yet diagnosed (a follow-up commit adds that under
-Wthread-safety-beta).
* An acquire over a try-held capability warns ("may already be held"; asserts
exempt); a reentrant capability deepens instead, keeping its
conditional top level. A try-acquire deepens a definite hold; over a
merely try-held fact it is diagnosed and not tracked (can only track
one fact per capability at a time).
* Success values decode by constant evaluation, defaulting to false,
unchanged from before.
* Stored results (`if ((b = mu.TryLock()))`, short-circuit forms) now
resolve like a branch on the call itself.
* A scoped guard's explicit unlock of a try-held mutex diagnoses like a
release; its definite re-acquire only deepens the fact. Scoped
lockables are otherwise not yet tracked conditionally.
* A join of unequal reentrancy depths warns once, then keeps the fact
that guarantees more (a try-held top level counts half) to minimize
follow-on warnings, following the same logic as before. A mixed
held/try-held pair keeps the try-held side, after warning.
* NFC: checkAcquiredCapability split out of addLock; "may" `%select{}`
flavors on the unlock/double-lock/excludes warnings;
`FactSet::isEmpty(FactManager&)` renamed `holdsNoCapability`.
Co-Authored-By: Claude Fable 5 <noreply at anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply at anthropic.com>
---
clang/docs/ReleaseNotes.md | 13 +
clang/docs/ThreadSafetyAnalysis.md | 19 +-
.../clang/Analysis/Analyses/ThreadSafety.h | 14 +-
.../clang/Basic/DiagnosticSemaKinds.td | 8 +-
clang/lib/Analysis/ThreadSafety.cpp | 798 ++++++++++++++----
clang/lib/Sema/AnalysisBasedWarnings.cpp | 15 +-
.../SemaCXX/warn-thread-safety-analysis.cpp | 420 +++++++++
.../SemaCXX/warn-thread-safety-negative.cpp | 41 +
8 files changed, 1147 insertions(+), 181 deletions(-)
diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md
index bf295981710ac..1ba0b67ae75a6 100644
--- a/clang/docs/ReleaseNotes.md
+++ b/clang/docs/ReleaseNotes.md
@@ -278,6 +278,19 @@ features cannot lower the translation-unit ABI level;
initialization, while not diagnosing parameters passed to the selected
allocation function or promise constructor. (#GH217501)
+- Thread safety analysis now tracks capabilities acquired by functions
+ annotated with `try_acquire_capability` accurately: the capability is
+ conditionally ("try") held from the call until a branch on its result, and
+ then is held only on the success path. A try-held capability does not satisfy
+ capability requirements, positive or negative, and acquiring or releasing it
+ before branching on the result warns that it may (or may not) be held. Some
+ diagnostics under `-Wthread-safety` change accordingly, although the main
+ improvements are intended to be behind `-Wthread-safety-negative` and
+ `-Wthread-safety-beta`. A try-acquire over a still-unresolved try-acquire of
+ the same capability will now warn, as the analysis cannot track both, and so
+ will an acquisition, blocking or try, that changes the kind (shared vs.
+ exclusive) of a held or try-held capability, even for a re-entrant one.
+
- Fixed bug in `-Wdocumentation` so that it correctly handles explicit
function template instantiations (#64087).
diff --git a/clang/docs/ThreadSafetyAnalysis.md b/clang/docs/ThreadSafetyAnalysis.md
index f29b5bbc55e1e..9e9ab92f78eb7 100644
--- a/clang/docs/ThreadSafetyAnalysis.md
+++ b/clang/docs/ThreadSafetyAnalysis.md
@@ -474,9 +474,22 @@ The first argument must be `true` or `false`, to specify which return value
indicates success, and the remaining arguments are interpreted in the same way
as `ACQUIRE`. See {ref}`mutexheader`, below, for example uses.
-Because the analysis doesn't support conditional locking, a capability is
-treated as acquired after the first branch on the return value of a try-acquire
-function.
+The capability is tracked as conditionally ("try") held from the call until a
+recognized branch on its return value: on the success path the capability is
+held, on the failure path it is not. A conditionally held capability does not
+satisfy requirements such as `GUARDED_BY` or `REQUIRES` and it also violates
+`LOCKS_EXCLUDED` and negative requirements (`REQUIRES(!mu)`). Acquiring a
+non-reentrant lock again before branching on the return value warns that it may
+already be held, and releasing the try-held capability warns that it may not be
+held. A try-acquire of a capability that is already held adds a further
+conditionally held level onto the hold -- the success branch may be statically
+unreachable, but the modeling doesn't assume that. An acquisition of the other
+kind (shared vs. exclusive) than the existing hold, blocking or try, generates
+a warning since the tracking for a hold has only a single kind. A try-acquire
+of a capability that is already conditionally held also warns: the analysis
+keeps one fact per capability and cannot track two unresolved try-acquires at
+once. Asserting the capability (`ASSERT_CAPABILITY`) upgrades it to held
+without a warning.
```c++
Mutex mu;
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 4fb04fb16b8c0..a96a445accee7 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -125,9 +125,12 @@ class ThreadSafetyHandler {
/// in the error message.
/// \param Loc -- The SourceLocation of the Unlock
/// \param LocPreviousUnlock -- If valid, the location of a previous Unlock.
+ /// \param MaybeHeld -- The capability is try-held: acquired by a
+ /// try-acquire whose result was not checked, so it is only possibly held.
virtual void handleUnmatchedUnlock(StringRef Kind, Name LockName,
SourceLocation Loc,
- SourceLocation LocPreviousUnlock) {}
+ SourceLocation LocPreviousUnlock,
+ bool MaybeHeld) {}
/// Warn about an unlock function call that attempts to unlock a lock with
/// the incorrect lock kind. For instance, a shared lock being unlocked
@@ -150,9 +153,11 @@ class ThreadSafetyHandler {
/// in the error message.
/// \param LocLocked -- The location of the first lock expression.
/// \param LocDoubleLock -- The location of the second lock expression.
+ /// \param MaybeHeld -- The first acquisition was a try-acquire whose result
+ /// was not checked, so the capability is only possibly held.
virtual void handleDoubleLock(StringRef Kind, Name LockName,
SourceLocation LocLocked,
- SourceLocation LocDoubleLock) {}
+ SourceLocation LocDoubleLock, bool MaybeHeld) {}
/// Warn about situations where a mutex is sometimes held and sometimes not.
/// The three situations are:
@@ -244,8 +249,11 @@ class ThreadSafetyHandler {
/// \param LockName -- A StringRef name for the lock expression, to be printed
/// in the error message.
/// \param Loc -- The location of the function call.
+ /// \param MaybeHeld -- The capability is try-held: acquired by a
+ /// try-acquire whose result was not checked, so it is only possibly held.
virtual void handleFunExcludesLock(StringRef Kind, Name FunName,
- Name LockName, SourceLocation Loc) {}
+ Name LockName, SourceLocation Loc,
+ bool MaybeHeld) {}
/// Warn when an actual underlying mutex of a scoped lockable does not match
/// the expected.
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b5100b67dccd7..b058588defb70 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4365,13 +4365,15 @@ def err_attribute_argument_out_of_bounds_extra_info : Error<
":must be between 1 and %2}2">;
// Thread Safety Analysis
-def warn_unlock_but_no_lock : Warning<"releasing %0 '%1' that was not held">,
+def warn_unlock_but_no_lock : Warning<
+ "releasing %0 '%1' that %select{was not|may not be}2 held">,
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
def warn_unlock_kind_mismatch : Warning<
"releasing %0 '%1' using %select{shared|exclusive}2 access, expected "
"%select{shared|exclusive}3 access">,
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
-def warn_double_lock : Warning<"acquiring %0 '%1' that is already held">,
+def warn_double_lock : Warning<
+ "acquiring %0 '%1' that %select{is already|may already be}2 held">,
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
def warn_no_unlock : Warning<
"%0 '%1' is still held at the end of function">,
@@ -4406,7 +4408,7 @@ def warn_requires_any_of_locks : Warning<
"at least one of %2">,
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
def warn_fun_excludes_mutex : Warning<
- "cannot call function '%1' while %0 '%2' is held">,
+ "cannot call function '%1' while %0 '%2' %select{is|may be}3 held">,
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
def warn_cannot_resolve_lock : Warning<
"cannot resolve lock expression">,
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index ba432243b942d..440d5b064fd9e 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -38,6 +38,7 @@
#include "clang/Basic/Specifiers.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/ImmutableMap.h"
+#include "llvm/ADT/PointerIntPair.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
@@ -93,11 +94,14 @@ namespace {
/// attributes on a function.
class CapExprSet : public SmallVector<CapabilityExpr, 4> {
public:
+ bool contains(const CapabilityExpr &CapE) const {
+ return llvm::any_of(
+ *this, [&](const CapabilityExpr &CapE2) { return CapE.equals(CapE2); });
+ }
+
/// Push M onto list, but discard duplicates.
void push_back_nodup(const CapabilityExpr &CapE) {
- if (llvm::none_of(*this, [=](const CapabilityExpr &CapE2) {
- return CapE.equals(CapE2);
- }))
+ if (!contains(CapE))
push_back(CapE);
}
};
@@ -106,9 +110,56 @@ class FactManager;
class FactSet;
/// This is a helper class that stores a fact that is known at a
-/// particular point in program execution. Currently, a fact is a capability,
+/// particular point in program execution. Concretely, a fact is a capability,
/// along with additional information, such as where it was acquired, whether
/// it is exclusive or shared, etc.
+///
+/// Per capability the analysis tracks a ternary state: not-held (no fact in
+/// the FactSet), try-held, or held. Permitted transitions:
+///
+/// not-held --acquire-----------------------------------------> held
+/// not-held --try-acquire (BuildLockset::handleCall)----------> try-held
+/// try-held --branch on the try-acquire result: success edge--> held
+/// try-held --branch on the try-acquire result: failure edge--> not-held
+/// try-held --acquire or assert (addLock)---------------------> held
+/// held -----join with a failed path of the same try-acquire,
+/// when the join re-branches on its result
+/// (intersectAndWarn)-------------------------------> try-held
+/// held -----release------------------------------------------> not-held
+///
+/// A fact additionally tracks a reentrancy depth, and so two of those
+/// transitions may deepen that instead of colliding: a try-acquire over
+/// a held capability -- whatever its reentrancy, since at runtime such a
+/// call fails rather than deadlocks -- and, for a reentrant capability
+/// only, an unconditional acquire over a try-held capability. Either way
+/// the fact becomes try-held one level deeper with the try-acquire call as
+/// its origin: held at that depth if that call succeeded, one level shallower
+/// otherwise -- and resolved at a branch on the result and unwound one
+/// level per release.
+///
+/// Try-held means "held if the try-acquire succeeded", so it warns
+/// wherever a definite state is required: it does not satisfy capability
+/// requirements, it violates exclusions and negative requirements,
+/// releasing it warns (may not be held), and acquiring it again warns
+/// (may already be held).
+/// Asserts and same-kind reentrant acquires (which deepen instead) are
+/// exempt from the acquire warning: they legitimately acquire a
+/// possibly-held capability. An acquire of the other kind (shared vs.
+/// exclusive) warns even for a reentrant capability: the tracked hold has
+/// a single kind.
+///
+/// When the analysis loses track of a try-held fact -- at a join with a
+/// path that does not hold it, or at the end of the function -- the
+/// try-acquire result was never checked and the capability may be leaked;
+/// this is diagnosed in beta mode.
+///
+/// Any other try-acquire of a capability that is already tracked is a
+/// conflict the model cannot represent: over a try-held fact a second
+/// unresolved try-acquire cannot be tracked (one origin per fact),
+/// reentrant or not, and over a definite hold a try-acquire of the other
+/// kind (shared vs. exclusive) cannot share the fact's single lock kind.
+/// Either way the existing fact wins unchanged, the call's acquisition
+/// goes untracked, and the conflict is diagnosed at the call.
class FactEntry : public CapabilityExpr {
public:
enum FactEntryKind { Lockable, ScopedLockable };
@@ -133,6 +184,25 @@ class FactEntry : public CapabilityExpr {
/// Where it was acquired.
SourceLocation AcquireLoc;
+ /// The try-acquire call this fact originates from (or null), and whether
+ /// the capability is still only conditionally ("try") held: acquired by
+ /// that call but not yet branched on its result, so held only on the paths
+ /// where the call succeeded. Facts promoted to held on the call's success
+ /// edge keep their origin (with the flag cleared), so that a later join
+ /// with a path where the try-acquire failed can be recognized. Facts
+ /// upgraded by an unconditional acquire or assert clear this: their held
+ /// state is not proved by the call's success, so a branch on its result
+ /// must not resolve them (a reentrant acquire instead deepens the fact,
+ /// keeping the origin for its still conditional top level). A try-held
+ /// fact whose paths merged different origins loses its origin and can
+ /// never be resolved; it stays try-held until the analysis loses track
+ /// of it.
+ /// (callexpr, true) : try-held state from callexpr
+ /// (callexpr, false) : held state from callexpr
+ /// (nullptr, true) : try-held state from multiple sources
+ /// (nullptr, false) : held state from definitive sources
+ llvm::PointerIntPair<const Expr *, 1, bool> TryLock;
+
protected:
~FactEntry() = default;
@@ -149,6 +219,26 @@ class FactEntry : public CapabilityExpr {
bool declared() const { return Source == Declared; }
bool managed() const { return Source == Managed; }
+ bool tryHeld() const { return TryLock.getInt(); }
+ const Expr *tryLockCall() const { return TryLock.getPointer(); }
+
+ /// Whether the capability is definitely held at least once: it is not
+ /// try-held, or only the top level of a reentrant acquisition is
+ /// conditional while the levels below it are definite.
+ virtual bool definitelyHeld() const { return !tryHeld(); }
+
+ /// The fact's reentrancy depth; only lockable facts can be reentrant.
+ virtual unsigned int getReentrancyDepth() const { return 0; }
+
+ /// Record that this fact originates from the try-acquire call \p Call.
+ /// While \p Conditional is true the fact is try-held and is resolved
+ /// (promoted to held or removed) at a branch on the call's result; a
+ /// try-held fact with a null origin (merged from different origins) can
+ /// never be resolved.
+ void setTryLock(const Expr *Call, bool Conditional) {
+ TryLock.setPointerAndInt(Call, Conditional);
+ }
+
virtual void
handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
SourceLocation JoinLoc, LockErrorKind LEK,
@@ -220,10 +310,12 @@ class FactSet {
bool isEmpty() const { return FactIDs.size() == 0; }
- // Return true if the set contains only negative facts
- bool isEmpty(FactManager &FactMan) const {
+ // Return true if the set holds no definitely-held positive capability.
+ // It may hold negative or try-held facts, unlike isEmpty, which tests
+ // the set itself.
+ bool holdsNoCapability(FactManager &FactMan) const {
for (const auto FID : *this) {
- if (!FactMan[FID].negative())
+ if (!FactMan[FID].negative() && FactMan[FID].definitelyHeld())
return false;
}
return true;
@@ -972,8 +1064,8 @@ namespace {
class LockableFactEntry final : public FactEntry {
private:
/// Reentrancy depth: incremented when a capability has been acquired
- /// reentrantly (after initial acquisition). Always 0 for non-reentrant
- /// capabilities.
+ /// again after its initial acquisition -- by a reentrant acquire, or by
+ /// a try-acquire over a definite hold.
unsigned int ReentrancyDepth = 0;
LockableFactEntry(const CapabilityExpr &CE, LockKind LK, SourceLocation Loc,
@@ -993,7 +1085,11 @@ class LockableFactEntry final : public FactEntry {
return new (Alloc) LockableFactEntry(CE, LK, Loc, Src);
}
- unsigned int getReentrancyDepth() const { return ReentrancyDepth; }
+ unsigned int getReentrancyDepth() const override { return ReentrancyDepth; }
+
+ bool definitelyHeld() const override {
+ return !tryHeld() || ReentrancyDepth > 0;
+ }
void
handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
@@ -1007,12 +1103,12 @@ class LockableFactEntry final : public FactEntry {
void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
ThreadSafetyHandler &Handler) const override {
- if (const FactEntry *RFact = tryReenter(FactMan, entry.kind())) {
+ if (const FactEntry *RFact = attemptReenter(FactMan, entry.kind())) {
// This capability has been reentrantly acquired.
FSet.replaceLock(FactMan, entry, RFact);
} else {
Handler.handleDoubleLock(entry.getKind(), entry.toString(), loc(),
- entry.loc());
+ entry.loc(), false);
}
}
@@ -1031,11 +1127,15 @@ class LockableFactEntry final : public FactEntry {
}
}
- // Return an updated FactEntry if we can acquire this capability reentrant,
- // nullptr otherwise.
- const FactEntry *tryReenter(FactManager &FactMan,
- LockKind ReenterKind) const {
- if (!reentrant())
+ // Return an updated FactEntry one level deeper, or nullptr if another
+ // acquisition cannot nest in this capability: the kinds must match, and
+ // a blocking acquire can only reacquire a reentrant capability. A
+ // conditional try-acquire can always nest -- at runtime it may simply
+ // fail. This checks only the capability, not the fact's held state:
+ // which transitions are permitted is checked by the caller.
+ const FactEntry *attemptReenter(FactManager &FactMan, LockKind ReenterKind,
+ bool Conditional = false) const {
+ if (!Conditional && !reentrant())
return nullptr;
if (kind() != ReenterKind)
return nullptr;
@@ -1045,11 +1145,10 @@ class LockableFactEntry final : public FactEntry {
}
// Return an updated FactEntry if we are releasing a capability previously
- // acquired reentrant, nullptr otherwise.
+ // acquired reentrant (or conditionally), nullptr otherwise.
const FactEntry *leaveReentrant(FactManager &FactMan) const {
if (!ReentrancyDepth)
return nullptr;
- assert(reentrant());
auto *NewFact = FactMan.createFact<LockableFactEntry>(*this);
NewFact->ReentrancyDepth--;
return NewFact;
@@ -1060,6 +1159,42 @@ class LockableFactEntry final : public FactEntry {
}
};
+static SourceLocation unmatchedUnlockNoteLoc(const FactSet &FSet,
+ FactManager &FactMan,
+ const CapabilityExpr &Cp) {
+ if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
+ return Neg->loc();
+ return SourceLocation();
+}
+
+/// Decide if this unlock unconditionally releases a capability that is only
+/// try-held; returns true if the release was handled here.
+/// Diagnose like an unmatched unlock and leave the negative fact behind:
+/// the thread provably does not hold the capability afterwards, whether the
+/// try-acquire succeeded or failed.
+/// With a null \p Handler (a scoped guard's destructor, from FullyRemove=true)
+/// the fact is kept unchanged: it may record an acquisition the guard does not
+/// own, which the destructor's conditional release cannot pair with.
+static bool handleUncheckedTryHeldUnlock(FactSet &FSet, FactManager &FactMan,
+ const FactEntry &Fact,
+ const CapabilityExpr &Cp,
+ SourceLocation UnlockLoc,
+ ThreadSafetyHandler *Handler) {
+ if (Fact.definitelyHeld())
+ return false;
+ if (Handler) {
+ Handler->handleUnmatchedUnlock(Cp.getKind(), Cp.toString(), UnlockLoc,
+ SourceLocation(), true);
+ FSet.removeLock(FactMan, Cp);
+ // A pre-existing negative fact survives a try-acquire (it is consumed
+ // only on the success edge), so do not add a duplicate over it.
+ if (!Cp.negative() && !FSet.findLock(FactMan, !Cp))
+ FSet.addLock(FactMan, FactMan.createFact<LockableFactEntry>(
+ !Cp, LK_Exclusive, UnlockLoc));
+ }
+ return true;
+}
+
enum UnderlyingCapabilityKind {
UCK_Acquired, ///< Any kind of acquired capability.
UCK_ReleasedShared, ///< Shared capability that was released.
@@ -1192,11 +1327,12 @@ class ScopedLockableFactEntry final
ThreadSafetyHandler *Handler) const {
if (const auto It = FSet.findLockIter(FactMan, Cp); It != FSet.end()) {
const auto &Fact = cast<LockableFactEntry>(FactMan[*It]);
- if (const FactEntry *RFact = Fact.tryReenter(FactMan, kind)) {
+ if (const FactEntry *RFact = Fact.attemptReenter(FactMan, kind)) {
// This capability has been reentrantly acquired.
FSet.replaceLock(FactMan, It, RFact);
} else if (Handler) {
- Handler->handleDoubleLock(Cp.getKind(), Cp.toString(), Fact.loc(), loc);
+ Handler->handleDoubleLock(Cp.getKind(), Cp.toString(), Fact.loc(), loc,
+ /*MaybeHeld=*/Fact.tryHeld());
}
} else {
FSet.removeLock(FactMan, !Cp);
@@ -1209,6 +1345,8 @@ class ScopedLockableFactEntry final
SourceLocation loc, ThreadSafetyHandler *Handler) const {
if (const auto It = FSet.findLockIter(FactMan, Cp); It != FSet.end()) {
const auto &Fact = cast<LockableFactEntry>(FactMan[*It]);
+ if (handleUncheckedTryHeldUnlock(FSet, FactMan, Fact, Cp, loc, Handler))
+ return;
if (const FactEntry *RFact = Fact.leaveReentrant(FactMan)) {
// This capability remains reentrantly acquired.
FSet.replaceLock(FactMan, It, RFact);
@@ -1219,10 +1357,9 @@ class ScopedLockableFactEntry final
FactMan, It,
FactMan.createFact<LockableFactEntry>(!Cp, LK_Exclusive, loc));
} else if (Handler) {
- SourceLocation PrevLoc;
- if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
- PrevLoc = Neg->loc();
- Handler->handleUnmatchedUnlock(Cp.getKind(), Cp.toString(), loc, PrevLoc);
+ Handler->handleUnmatchedUnlock(Cp.getKind(), Cp.toString(), loc,
+ unmatchedUnlockNoteLoc(FSet, FactMan, Cp),
+ false);
}
}
};
@@ -1241,6 +1378,16 @@ class ThreadSafetyAnalyzer {
LocalVariableMap LocalVarMap;
// Maps constructed objects to `this` placeholder prior to initialization.
llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
+ /// The capabilities named by a try-acquire call's attributes, translated
+ /// in the call's own context and grouped by the attribute's lock kind and
+ /// success value (Falsy: reported acquired when the call returns false).
+ struct TryAcquireCaps {
+ CapExprSet TruthyExclusive, TruthyShared;
+ CapExprSet FalsyExclusive, FalsyShared;
+ };
+ // Maps each try-acquire call to its attributes' capabilities, recorded
+ // before the lockset walk.
+ llvm::SmallDenseMap<const Expr *, TryAcquireCaps> TryAcquireCapsMap;
FactManager FactMan;
std::vector<CFGBlockInfo> BlockInfo;
@@ -1254,6 +1401,12 @@ class ThreadSafetyAnalyzer {
bool inCurrentScope(const CapabilityExpr &CapE);
void addLock(FactSet &FSet, const FactEntry *Entry, bool ReqAttr = false);
+ void addTryLock(FactSet &FSet, const CapabilityExpr &CE, LockKind LK,
+ SourceLocation Loc, const Expr *Call);
+ void checkAcquiredCapability(FactSet &FSet, const FactEntry &Entry,
+ bool ReqAttr);
+ const FactEntry *cloneWithTryLock(const FactEntry &FE, const Expr *Call,
+ bool Conditional);
void removeLock(FactSet &FSet, const CapabilityExpr &CapE,
SourceLocation UnlockLoc, bool FullyRemove, LockKind Kind);
@@ -1261,6 +1414,10 @@ class ThreadSafetyAnalyzer {
void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
const NamedDecl *D, til::SExpr *Self = nullptr);
+ void recordTryAcquireCall(const Expr *Exp, const NamedDecl *D,
+ til::SExpr *Self = nullptr);
+ void recordTryAcquireCalls(const PostOrderCFGView *SortedGraph);
+
/// Intermediate state for decodeTrylockBranch.
struct TrylockDecode {
/// The try-acquire call reached in the AST walk.
@@ -1312,15 +1469,13 @@ class ThreadSafetyAnalyzer {
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,
- const CapExprSet *TrylockRebranchCaps = nullptr);
+ const Expr *RebranchTryLock = nullptr);
void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet,
SourceLocation JoinLoc, LockErrorKind LEK) {
@@ -1516,32 +1671,107 @@ void ThreadSafetyAnalyzer::addLock(FactSet &FSet, const FactEntry *Entry,
if (Entry->shouldIgnore())
return;
- if (!ReqAttr && !Entry->negative()) {
+ checkAcquiredCapability(FSet, *Entry, ReqAttr);
+
+ if (const FactEntry *Cp = FSet.findLock(FactMan, *Entry)) {
+ if (Entry->tryHeld()) {
+ // Try-acquiring a capability that is already tracked deepens a
+ // definite hold, regardless of reentrancy: unlike a blocking
+ // acquire, the call cannot deadlock -- it simply fails when the
+ // capability cannot be recursively acquired (and even a reentrant
+ // capability's success is not guaranteed, e.g. by a recursion
+ // limit). The deepened fact becomes conditional at its top level,
+ // resolved by a branch on the result like any other try-held fact.
+ if (const auto *LCp = dyn_cast<LockableFactEntry>(Cp);
+ LCp && !Cp->tryHeld())
+ if (const FactEntry *RFact = LCp->attemptReenter(
+ FactMan, Entry->kind(), /*Conditional=*/true)) {
+ FSet.replaceLock(FactMan, *Cp,
+ cloneWithTryLock(*RFact, Entry->tryLockCall(),
+ /*Conditional=*/true));
+ return;
+ }
+ // Otherwise the model cannot track this call's acquisition:
+ // different kinds or repeat try-held cannot be expressed.
+ Handler.handleDoubleLock(Entry->getKind(), Entry->toString(), Cp->loc(),
+ Entry->loc(), /*MaybeHeld=*/Cp->tryHeld());
+ return;
+ }
+ if (Cp->tryHeld()) {
+ if (Entry->asserted()) {
+ // An assert directly upgrades the lock to being held, without a
+ // diagnostic: it claims exactly that knowledge.
+ FSet.replaceLock(FactMan, *Entry, Entry);
+ return;
+ }
+ // A reentrant acquire of the same kind deepens the try-held fact.
+ if (const auto *LCp = dyn_cast<LockableFactEntry>(Cp))
+ if (const FactEntry *RFact =
+ LCp->attemptReenter(FactMan, Entry->kind())) {
+ FSet.replaceLock(FactMan, *Cp, RFact);
+ return;
+ }
+ // Warn that this TryHeld -> Held transition is invalid.
+ assert(!Entry->tryLockCall() &&
+ "branch edges resolve facts in place; they do not re-acquire");
+ Handler.handleDoubleLock(Entry->getKind(), Entry->toString(), Cp->loc(),
+ Entry->loc(), /*MaybeHeld=*/true);
+ // Subsequently, if the program didn't deadlock, it is now asserted
+ // locked.
+ FSet.replaceLock(FactMan, *Entry, Entry);
+ return;
+ }
+ if (!Entry->asserted())
+ Cp->handleLock(FSet, FactMan, *Entry, Handler);
+ } else {
+ FSet.addLock(FactMan, Entry);
+ }
+}
+
+/// The checks required before an acquisition: consume (or require) the negative
+/// capability, and check acquired_before/acquired_after ordering.
+void ThreadSafetyAnalyzer::checkAcquiredCapability(FactSet &FSet,
+ const FactEntry &Entry,
+ bool ReqAttr) {
+ if (!ReqAttr && !Entry.negative()) {
// look for the negative capability, and remove it from the fact set.
- CapabilityExpr NegC = !*Entry;
+ CapabilityExpr NegC = !Entry;
const FactEntry *Nen = FSet.findLock(FactMan, NegC);
if (Nen) {
- FSet.removeLock(FactMan, NegC);
- }
- else {
- if (inCurrentScope(*Entry) && !Entry->asserted() && !Entry->reentrant())
- Handler.handleNegativeNotHeld(Entry->getKind(), Entry->toString(),
- NegC.toString(), Entry->loc());
+ if (!Entry.tryHeld())
+ FSet.removeLock(FactMan, NegC);
+ } else {
+ if (inCurrentScope(Entry) && !Entry.asserted() && !Entry.reentrant())
+ Handler.handleNegativeNotHeld(Entry.getKind(), Entry.toString(),
+ NegC.toString(), Entry.loc());
}
}
// Check before/after constraints
- if (!Entry->asserted() && !Entry->declared()) {
- GlobalBeforeSet->checkBeforeAfter(Entry->valueDecl(), FSet, *this,
- Entry->loc(), Entry->getKind());
+ if (!Entry.asserted() && !Entry.declared()) {
+ GlobalBeforeSet->checkBeforeAfter(Entry.valueDecl(), FSet, *this,
+ Entry.loc(), Entry.getKind());
}
+}
- if (const FactEntry *Cp = FSet.findLock(FactMan, *Entry)) {
- if (!Entry->asserted())
- Cp->handleLock(FSet, FactMan, *Entry, Handler);
- } else {
- FSet.addLock(FactMan, Entry);
- }
+/// Clone \p FE with its try-acquire origin and try-held flag replaced.
+const FactEntry *ThreadSafetyAnalyzer::cloneWithTryLock(const FactEntry &FE,
+ const Expr *Call,
+ bool Conditional) {
+ auto *NewFact =
+ FactMan.createFact<LockableFactEntry>(cast<LockableFactEntry>(FE));
+ NewFact->setTryLock(Call, Conditional);
+ return NewFact;
+}
+
+/// Add a try-held fact for the capability \p CE acquired by the try-acquire
+/// call \p Call at \p Loc; the fact remembers its originating call.
+void ThreadSafetyAnalyzer::addTryLock(FactSet &FSet, const CapabilityExpr &CE,
+ LockKind LK, SourceLocation Loc,
+ const Expr *Call) {
+ auto *Fact = FactMan.createFact<LockableFactEntry>(CE, LK, Loc);
+ Fact->setTryLock(Call, /*Conditional=*/true);
+ addLock(FSet, Fact);
}
/// Remove a lock from the lockset, warning if the lock is not there.
@@ -1554,14 +1784,16 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,
const FactEntry *LDat = FSet.findLock(FactMan, Cp);
if (!LDat) {
- SourceLocation PrevLoc;
- if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
- PrevLoc = Neg->loc();
Handler.handleUnmatchedUnlock(Cp.getKind(), Cp.toString(), UnlockLoc,
- PrevLoc);
+ unmatchedUnlockNoteLoc(FSet, FactMan, Cp),
+ false);
return;
}
+ if (handleUncheckedTryHeldUnlock(FSet, FactMan, *LDat, Cp, UnlockLoc,
+ &Handler))
+ return;
+
// Generic lock removal doesn't care about lock kind mismatches, but
// otherwise diagnose when the lock kinds are mismatched.
if (ReceivedKind != LK_Generic && LDat->kind() != ReceivedKind) {
@@ -1677,6 +1909,10 @@ void ThreadSafetyAnalyzer::decodeTrylockCond(const Stmt *Cond,
}
if (BOP->getOpcode() == BO_LOr)
return decodeTrylockCond(BOP->getRHS(), C, D);
+ // An assignment used as a condition (`if ((b = mu.TryLock()))`)
+ // evaluates to its right-hand side.
+ if (BOP->getOpcode() == BO_Assign)
+ return decodeTrylockCond(BOP->getRHS(), C, D);
return;
} else if (const auto *COP = dyn_cast<ConditionalOperator>(Cond)) {
bool TCond, FCond;
@@ -1697,20 +1933,18 @@ void ThreadSafetyAnalyzer::decodeTrylockCond(const Stmt *Cond,
}
}
-/// Decode a try-acquire attribute's success value.
-static bool getTrySuccessValue(const Expr *BrE) {
- if (const auto *BLE = dyn_cast_or_null<CXXBoolLiteralExpr>(BrE))
- return BLE->getValue();
- if (const auto *ILE = dyn_cast_or_null<IntegerLiteral>(BrE))
- return ILE->getValue().getBoolValue();
- return false;
+/// Decode a try-acquire attribute's success value. An expression that does
+/// not constant-evaluate reads as false.
+static bool getTrySuccessValue(ASTContext &Ctx, const Expr *BrE) {
+ bool Result;
+ return BrE && !BrE->isValueDependent() &&
+ BrE->EvaluateAsBooleanCondition(Result, Ctx) && Result;
}
/// 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 the capabilities the call's attributes name,
-/// each with the resolution every branch direction proves for it. Attributes
-/// may carry different success values; each is decoded on its own.
+/// in a local variable), return the capabilities recorded for the call, each
+/// with the resolution every branch direction proves for it.
const ThreadSafetyAnalyzer::TrylockBranch &
ThreadSafetyAnalyzer::decodeTrylockBranch(const CFGBlock *Block) {
const unsigned BlockID = Block->getBlockID();
@@ -1732,46 +1966,30 @@ ThreadSafetyAnalyzer::decodeTrylockBranch(const CFGBlock *Block) {
if (!COp->getType()->isVoidType())
return CacheMiss();
- const LocalVarContext &LVarCtx = BlockInfo[BlockID].ExitContext;
-
TrylockDecode D;
- decodeTrylockCond(Cond, LVarCtx, D);
+ decodeTrylockCond(Cond, BlockInfo[BlockID].ExitContext, D);
if (!D.TrylockCall)
return CacheMiss();
- const auto *FunDecl = cast<NamedDecl>(D.TrylockCall->getCalleeDecl());
-
- if (Handler.issueBetaWarnings()) {
- // Temporarily set the lookup context for SExprBuilder.
- SxBuilder.setLookupLocalVarExpr(
- [this, Ctx = LVarCtx](const NamedDecl *VD) mutable -> const Expr * {
- return LocalVarMap.lookupExpr(VD, Ctx);
- });
- }
- CapExprSet TruthyExclusive, TruthyShared, FalsyExclusive, FalsyShared;
- for (const auto *Attr : FunDecl->specific_attrs<TryAcquireCapabilityAttr>()) {
- const bool Success = getTrySuccessValue(Attr->getSuccessValue());
- getMutexIDs(Success ? (Attr->isShared() ? TruthyShared : TruthyExclusive)
- : (Attr->isShared() ? FalsyShared : FalsyExclusive),
- Attr, D.TrylockCall, FunDecl);
- }
- if (Handler.issueBetaWarnings())
- SxBuilder.setLookupLocalVarExpr(nullptr);
// Translate call truthiness to branch truthiness.
TrylockBranch Result;
Result.TrylockCall = D.TrylockCall;
- auto AddCaps = [&](const CapExprSet &CapSet, LockKind LK, bool Success) {
- for (const CapabilityExpr &CE : CapSet) {
- (Success != D.Negate ? Result.OnTrue : Result.OnFalse)
- .push_back({CE, LK, CapResolution::Success});
- (Success != D.Negate ? Result.OnFalse : Result.OnTrue)
- .push_back({CE, LK, CapResolution::Failure});
- }
- };
- AddCaps(TruthyExclusive, LK_Exclusive, /*Success=*/true);
- AddCaps(TruthyShared, LK_Shared, /*Success=*/true);
- AddCaps(FalsyExclusive, LK_Exclusive, /*Success=*/false);
- AddCaps(FalsyShared, LK_Shared, /*Success=*/false);
+ if (auto MapIt = TryAcquireCapsMap.find(D.TrylockCall);
+ MapIt != TryAcquireCapsMap.end()) {
+ const TryAcquireCaps &Caps = MapIt->second;
+ auto AddCaps = [&](const CapExprSet &CapSet, LockKind LK, bool Success) {
+ for (const CapabilityExpr &CE : CapSet) {
+ (Success != D.Negate ? Result.OnTrue : Result.OnFalse)
+ .push_back({CE, LK, CapResolution::Success});
+ (Success != D.Negate ? Result.OnFalse : Result.OnTrue)
+ .push_back({CE, LK, CapResolution::Failure});
+ }
+ };
+ AddCaps(Caps.TruthyExclusive, LK_Exclusive, /*Success=*/true);
+ AddCaps(Caps.TruthyShared, LK_Shared, /*Success=*/true);
+ AddCaps(Caps.FalsyExclusive, LK_Exclusive, /*Success=*/false);
+ AddCaps(Caps.FalsyShared, LK_Shared, /*Success=*/false);
+ }
return TerminatorTrylockCache[BlockID] = std::move(Result);
}
@@ -1795,11 +2013,19 @@ ThreadSafetyAnalyzer::resolveTrylockEdge(const CFGBlock *PredBlock,
SI != SE && i < 2; ++SI, ++i)
if (*SI == CurrBlock)
(i == 0 ? TrueEdge : FalseEdge) = true;
- // An edge occupying neither position, or both, has no effect.
- if (TrueEdge == FalseEdge)
+ // An edge occupying both positions (the branch reaches the same block
+ // either way) has no effect.
+ if (TrueEdge && FalseEdge)
return Edge;
Edge.TrylockCall = B.TrylockCall;
+ if (!TrueEdge && !FalseEdge) {
+ // An edge occupying neither position (e.g. a switch case) proves no
+ // acquisition.
+ for (const TrylockEdgeCap &TC : B.OnTrue)
+ Edge.Caps.push_back({TC.Cap, TC.Kind, CapResolution::Failure});
+ return Edge;
+ }
const SmallVectorImpl<TrylockEdgeCap> &Dir = TrueEdge ? B.OnTrue : B.OnFalse;
Edge.Caps.assign(Dir.begin(), Dir.end());
return Edge;
@@ -1818,22 +2044,55 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet &Result,
if (!Edge.TrylockCall)
return;
- // Add the capabilities this edge proves were acquired.
- SourceLocation Loc = Edge.TrylockCall->getExprLoc();
- for (const TrylockEdgeCap &EC : Edge.Caps)
- if (EC.Resolution == CapResolution::Success)
- addLock(Result,
- FactMan.createFact<LockableFactEntry>(EC.Cap, EC.Kind, Loc));
-}
+ // Collect the try-held facts this call created, to resolve on this edge.
+ SmallVector<const FactEntry *> ResolvedTryFacts;
+ for (const auto &Fact : Result) {
+ const FactEntry &FE = FactMan[Fact];
+ if (FE.tryHeld() && FE.tryLockCall() == Edge.TrylockCall)
+ ResolvedTryFacts.push_back(&FE);
+ }
+ if (ResolvedTryFacts.empty())
+ return;
+ assert(!Edge.Caps.empty() &&
+ "try-acquire fact without capabilities recorded at its call");
+
+ // Whether the fact's capability is acquired on this edge: the fact is
+ // re-identified by matching its capability against the capabilities
+ // recorded at the call, with the resolution this edge proves for each.
+ auto FactSucceedsHere = [&](const FactEntry &FE) {
+ if (llvm::any_of(Edge.Caps, [&](const TrylockEdgeCap &EC) {
+ return EC.Resolution == CapResolution::Success && FE.matches(EC.Cap);
+ }))
+ return true;
+ assert(llvm::any_of(
+ Edge.Caps,
+ [&](const TrylockEdgeCap &EC) { return FE.matches(EC.Cap); }) &&
+ "try-acquire fact matches neither polarity's capabilities");
+ return false;
+ };
-/// 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) {
- const TrylockBranch &B = decodeTrylockBranch(Block);
- for (const TrylockEdgeCap &TC : B.OnTrue)
- Caps.push_back_nodup(TC.Cap);
+ // Add or remove all resolved locks from this edge now.
+ for (const FactEntry *FE : ResolvedTryFacts) {
+ if (FactSucceedsHere(*FE)) {
+ // Success edge replaces try-held with held.
+ Result.replaceLock(FactMan, *FE,
+ cloneWithTryLock(*FE, FE->tryLockCall(),
+ /*Conditional=*/false));
+ if (!FE->negative())
+ Result.removeLock(FactMan, !*FE);
+ } else if (const FactEntry *Shallower =
+ isa<LockableFactEntry>(FE)
+ ? cast<LockableFactEntry>(FE)->leaveReentrant(FactMan)
+ : nullptr) {
+ // Failure edge removes try-held fact from reentrant stack.
+ Result.replaceLock(FactMan, *FE,
+ cloneWithTryLock(*Shallower, nullptr,
+ /*Conditional=*/false));
+ } else {
+ // Failure edge replaces try-held with unheld.
+ Result.removeLock(FactMan, *FE);
+ }
+ }
}
namespace {
@@ -2006,11 +2265,11 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
}
if (Cp.negative()) {
- // Negative capabilities act like locks excluded
- const FactEntry *LDat = FSet.findLock(FactMan, !Cp);
- if (LDat) {
+ // Negative capabilities act like locks excluded.
+ if (const FactEntry *LDat = FSet.findLock(FactMan, !Cp)) {
Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
- (!Cp).toString(), Loc);
+ (!Cp).toString(), Loc,
+ !LDat->definitelyHeld());
return;
}
@@ -2020,18 +2279,22 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
return;
// Otherwise the negative requirement must be propagated to the caller.
- LDat = FSet.findLock(FactMan, Cp);
- if (!LDat) {
+ if (!FSet.findLock(FactMan, Cp))
Handler.handleNegativeNotHeld(D, Cp.toString(), Loc);
- }
return;
}
const FactEntry *LDat = FSet.findLockUniv(FactMan, Cp);
+ // A try-held capability does not satisfy a requirement: it is only held on
+ // the paths where the try-acquire succeeded.
+ if (LDat && !LDat->definitelyHeld())
+ LDat = nullptr;
bool NoError = true;
if (!LDat) {
// No exact match found. Look for a partial match.
LDat = FSet.findPartialMatch(FactMan, Cp);
+ if (LDat && !LDat->definitelyHeld())
+ LDat = nullptr;
if (LDat) {
// Warn that there's no precise match.
std::string PartMatchStr = LDat->toString();
@@ -2064,7 +2327,7 @@ void ThreadSafetyAnalyzer::warnIfAnyMutexNotHeldForRead(
if (Cp.shouldIgnore())
continue;
const FactEntry *LDat = FSet.findLockUniv(FactMan, Cp);
- if (LDat && LDat->isAtLeast(LK_Shared))
+ if (LDat && LDat->definitelyHeld() && LDat->isAtLeast(LK_Shared))
return; // At least one held — read access is safe.
// FIXME: try findPartialMatch as a fallback to support
// -Wno-thread-safety-precise, as warnIfMutexNotHeld does.
@@ -2095,10 +2358,9 @@ void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet,
return;
}
- const FactEntry *LDat = FSet.findLock(FactMan, Cp);
- if (LDat) {
+ if (const FactEntry *LDat = FSet.findLock(FactMan, Cp)) {
Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
- Cp.toString(), Loc);
+ Cp.toString(), Loc, !LDat->definitelyHeld());
}
}
@@ -2164,7 +2426,7 @@ void ThreadSafetyAnalyzer::checkAccess(const FactSet &FSet, const Expr *Exp,
if (!D || !D->hasAttrs())
return;
- if (D->hasAttr<GuardedVarAttr>() && FSet.isEmpty(FactMan)) {
+ if (D->hasAttr<GuardedVarAttr>() && FSet.holdsNoCapability(FactMan)) {
Handler.handleNoMutexHeld(D, POK, AK, Loc);
}
@@ -2239,7 +2501,7 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
if (!D || !D->hasAttrs())
return;
- if (D->hasAttr<PtGuardedVarAttr>() && FSet.isEmpty(FactMan))
+ if (D->hasAttr<PtGuardedVarAttr>() && FSet.holdsNoCapability(FactMan))
Handler.handleNoMutexHeld(D, PtPOK, AK, Exp->getExprLoc());
for (auto const *I : D->specific_attrs<PtGuardedByAttr>()) {
@@ -2326,6 +2588,21 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
break;
}
+ // Try-acquired capabilities were already recorded for CallExprs, so only
+ // a constructor is recorded here, on its first try-acquire attribute,
+ // where its constructed-object placeholder is available.
+ // The conditional locks are added to our lockset below, from the recorded
+ // capabilities in TryAcquireCapsMap.
+ case attr::TryAcquireCapability: {
+ if (Exp && (!isa<CXXConstructExpr>(Exp) ||
+ Analyzer->TryAcquireCapsMap.contains(Exp)))
+ break;
+ auto PostContextForThisScope =
+ LVarCtx.switchToContextForScope(DualLocalVarContext::Post);
+ Analyzer->recordTryAcquireCall(Exp, D, Self);
+ break;
+ }
+
// An assert will add a lock to the lockset, but will not generate
// a warning if it is already there, and will not generate a warning
// if it is not removed.
@@ -2504,6 +2781,24 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
Analyzer->addLock(FSet, Analyzer->FactMan.createFact<LockableFactEntry>(
M, LK_Shared, Loc, Source));
+ // Add conditional locks.
+ // Note that scoped lockables manage their underlying mutexes themselves and
+ // are not tracked conditionally.
+ if (Exp && Scp.shouldIgnore()) {
+ if (auto It = Analyzer->TryAcquireCapsMap.find(Exp);
+ It != Analyzer->TryAcquireCapsMap.end()) {
+ const ThreadSafetyAnalyzer::TryAcquireCaps &Caps = It->second;
+ for (const auto &M : Caps.TruthyExclusive)
+ Analyzer->addTryLock(FSet, M, LK_Exclusive, Loc, Exp);
+ for (const auto &M : Caps.FalsyExclusive)
+ Analyzer->addTryLock(FSet, M, LK_Exclusive, Loc, Exp);
+ for (const auto &M : Caps.TruthyShared)
+ Analyzer->addTryLock(FSet, M, LK_Shared, Loc, Exp);
+ for (const auto &M : Caps.FalsyShared)
+ Analyzer->addTryLock(FSet, M, LK_Shared, Loc, Exp);
+ }
+ }
+
if (!Scp.shouldIgnore()) {
// Add the managing object as a dummy mutex, mapped to the underlying mutex.
auto *ScopedEntry = Analyzer->FactMan.createFact<ScopedLockableFactEntry>(
@@ -2758,21 +3053,30 @@ bool ThreadSafetyAnalyzer::join(const FactEntry &A, const FactEntry &B,
LockErrorKind EntryLEK) {
// Whether we can replace \p A by \p B.
const bool CanModify = EntryLEK != LEK_LockedSomeLoopIterations;
- unsigned int ReentrancyDepthA = 0;
- unsigned int ReentrancyDepthB = 0;
- if (const auto *LFE = dyn_cast<LockableFactEntry>(&A))
- ReentrancyDepthA = LFE->getReentrancyDepth();
- if (const auto *LFE = dyn_cast<LockableFactEntry>(&B))
- ReentrancyDepthB = LFE->getReentrancyDepth();
+ if (A.tryHeld() != B.tryHeld()) {
+ // Held joined with try-held: the merged fact must be the weaker
+ // try-held one. Under the same-origin re-branch exemption, only a
+ // try-held fact is re-resolved at the edges. An unequal reentrancy
+ // depth is diagnosed by intersectAndWarn(), which knows whether the
+ // exemption otherwise forgives this join silently.
+ return CanModify && B.tryHeld();
+ }
+
+ const unsigned int ReentrancyDepthA = A.getReentrancyDepth();
+ const unsigned int ReentrancyDepthB = B.getReentrancyDepth();
if (ReentrancyDepthA != ReentrancyDepthB) {
Handler.handleMutexHeldEndOfScope(B.getKind(), B.toString(), B.loc(),
JoinLoc, EntryLEK,
/*ReentrancyMismatch=*/true);
- // Pick the FactEntry with the greater reentrancy depth as the "good"
- // fact to reduce potential later warnings.
- return CanModify && ReentrancyDepthA < ReentrancyDepthB;
+ // The mismatch is already diagnosed; keep the fact that guarantees
+ // more, to minimize follow-on warnings in the same function: compare
+ // reentrancy depth, with a conditional (try-held) top level valued at
+ // half a level.
+ int ScoreA = 2 * (int)ReentrancyDepthA - (A.tryHeld() ? 1 : 0);
+ int ScoreB = 2 * (int)ReentrancyDepthB - (B.tryHeld() ? 1 : 0);
+ return CanModify && ScoreB > ScoreA;
} else if (A.kind() != B.kind()) {
// For managed capabilities, the destructor should unlock in the right mode
// anyway. For asserted capabilities no unlocking is needed.
@@ -2806,22 +3110,51 @@ 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.
-/// \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) {
+/// \param RebranchTryLock The try-acquire call whose result the joining
+/// block's terminator branches on, if any. A held/try-held difference
+/// between facts that both originate from that call is not diagnosed as a
+/// lost hold: the paths re-diverge at the terminator, so the merged fact is
+/// kept try-held (any reentrancy depth is diagnosed but kept) and
+/// re-resolved on the outgoing edges by getEdgeLockset(). A difference
+/// against a fact not created by that call is diagnosed normally.
+void ThreadSafetyAnalyzer::intersectAndWarn(FactSet &EntrySet,
+ const FactSet &ExitSet,
+ SourceLocation JoinLoc,
+ LockErrorKind EntryLEK,
+ LockErrorKind ExitLEK,
+ const Expr *RebranchTryLock) {
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);
- });
+ auto IsTrylockRebranched = [RebranchTryLock](const FactEntry &FE) {
+ return RebranchTryLock && FE.tryLockCall() == RebranchTryLock;
+ };
+ auto DemoteToTryHeld = [&, this](const FactEntry &FE,
+ LockErrorKind LEK) -> const FactEntry * {
+ // Replace a definite hold with a conditional hold. A mismatched
+ // reentrancy depth is diagnosed here but kept -- after the warning,
+ // the deeper fact guards more of the releases downstream than a
+ // stripped one would -- and other differences surface downstream once
+ // the edges re-resolve the demoted fact.
+ if (FE.getReentrancyDepth() != 0)
+ Handler.handleMutexHeldEndOfScope(FE.getKind(), FE.toString(), FE.loc(),
+ JoinLoc, LEK,
+ /*ReentrancyMismatch=*/true);
+ return cloneWithTryLock(FE, FE.tryLockCall(), /*Conditional=*/true);
+ };
+ // Warn about a fact the intersection removes (or weakens to try-held).
+ // However, a capability managed by a scoped object is exempt -- the
+ // scoped fact still knows to release it -- except where the scope itself
+ // ends or repeats.
+ auto WarnRemovedEntryFact = [&](const FactEntry &EntryFact) {
+ if (!EntryFact.managed() || ExitLEK == LEK_LockedSomeLoopIterations ||
+ ExitLEK == LEK_NotLockedAtEndOfFunction)
+ EntryFact.handleRemovalFromIntersection(EntrySetOrig, FactMan, JoinLoc,
+ ExitLEK, Handler);
+ };
+ auto WarnRemovedExitFact = [&](const FactEntry &ExitFact) {
+ if (!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction)
+ ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
+ EntryLEK, Handler);
};
// Find locks in ExitSet that conflict or are not in EntrySet, and warn.
@@ -2830,12 +3163,51 @@ void ThreadSafetyAnalyzer::intersectAndWarn(
FactSet::iterator EntryIt = EntrySet.findLockIter(FactMan, ExitFact);
if (EntryIt != EntrySet.end()) {
- if (join(FactMan[*EntryIt], ExitFact, JoinLoc, EntryLEK))
+ const FactEntry &EntryFact = FactMan[*EntryIt];
+ if (EntryFact.tryHeld() != ExitFact.tryHeld()) {
+ if (!(IsTrylockRebranched(EntryFact) &&
+ IsTrylockRebranched(ExitFact))) {
+ // The capability is held on one path but only try-held on the other,
+ // and the terminator does not re-branch on the try-acquire call both
+ // facts originate from. Warn about this as if the try-held path did
+ // not hold the capability at all.
+ if (ExitFact.tryHeld())
+ WarnRemovedEntryFact(EntryFact);
+ else
+ WarnRemovedExitFact(ExitFact);
+ } else {
+ if (EntryLEK != LEK_LockedSomeLoopIterations &&
+ EntryFact.getReentrancyDepth() != ExitFact.getReentrancyDepth())
+ Handler.handleMutexHeldEndOfScope(ExitFact.getKind(),
+ ExitFact.toString(),
+ ExitFact.loc(), JoinLoc, EntryLEK,
+ /*ReentrancyMismatch=*/true);
+ }
+ }
+ const Expr *EntryOrigin = EntryFact.tryLockCall();
+ if (join(EntryFact, ExitFact, JoinLoc, EntryLEK))
*EntryIt = Fact;
- } else if ((!ExitFact.managed() || EntryLEK == LEK_LockedAtEndOfFunction) &&
- !IsTrylockRebranched(ExitFact)) {
- ExitFact.handleRemovalFromIntersection(ExitSet, FactMan, JoinLoc,
- EntryLEK, Handler);
+ // If the two paths hold the capability via different origins, the
+ // merged fact is not determined by either try-acquire's result.
+ if (const FactEntry &Merged = FactMan[*EntryIt];
+ EntryLEK == LEK_LockedSomePredecessors && Merged.tryLockCall() &&
+ EntryOrigin != ExitFact.tryLockCall())
+ EntrySet.replaceLock(
+ FactMan, EntryIt,
+ cloneWithTryLock(Merged, nullptr, Merged.tryHeld()));
+ } else if (IsTrylockRebranched(ExitFact)) {
+ // Held on this predecessor only, but the terminator re-branches on
+ // the try-acquire that created the fact: demote it to try-held
+ // without warning, as getEdgeLockset will re-resolve it on the
+ // outgoing edges.
+ if (EntryLEK != LEK_LockedSomeLoopIterations)
+ EntrySet.addLock(FactMan, DemoteToTryHeld(ExitFact, EntryLEK));
+ } else if (ExitFact.tryHeld()) {
+ // The analysis loses track of the try-held fact here: this predecessor
+ // carries a try-acquire result into the join (or to the end of the
+ // function) without its result having been checked.
+ } else {
+ WarnRemovedExitFact(ExitFact);
}
}
@@ -2845,11 +3217,21 @@ void ThreadSafetyAnalyzer::intersectAndWarn(
const FactEntry *ExitFact = ExitSet.findLock(FactMan, *EntryFact);
if (!ExitFact) {
- if ((!EntryFact->managed() || ExitLEK == LEK_LockedSomeLoopIterations ||
- ExitLEK == LEK_NotLockedAtEndOfFunction) &&
- !IsTrylockRebranched(*EntryFact))
- EntryFact->handleRemovalFromIntersection(EntrySetOrig, FactMan, JoinLoc,
- ExitLEK, Handler);
+ if (IsTrylockRebranched(*EntryFact)) {
+ // As above, but here the fact is kept in the intersection in its
+ // demoted try-held form (except at a loop join, where the entry set
+ // is left unmodified).
+ if (!EntryFact->tryHeld() && EntryLEK != LEK_LockedSomeLoopIterations)
+ EntrySet.replaceLock(FactMan, *EntryFact,
+ DemoteToTryHeld(*EntryFact, ExitLEK));
+ continue;
+ }
+ if (EntryFact->tryHeld()) {
+ // As above, with the unchecked try-acquire on an earlier
+ // predecessor: it gets lost by the analysis.
+ } else {
+ WarnRemovedEntryFact(*EntryFact);
+ }
if (ExitLEK == LEK_LockedSomePredecessors)
EntrySet.removeLock(FactMan, *EntryFact);
}
@@ -2895,6 +3277,73 @@ static bool neverReturns(const CFGBlock *B) {
return false;
}
+/// Record the capabilities named by the try-acquire attributes of the call
+/// or construction \p Exp to \p D into TryAcquireCapsMap, translated in the
+/// currently installed context. Without an expression there is nothing to
+/// record or branch on; translate only for the diagnostics.
+void ThreadSafetyAnalyzer::recordTryAcquireCall(const Expr *Exp,
+ const NamedDecl *D,
+ til::SExpr *Self) {
+ TryAcquireCaps DiscardedCaps;
+ TryAcquireCaps &Caps = Exp ? TryAcquireCapsMap[Exp] : DiscardedCaps;
+ for (const Attr *At : D->attrs()) {
+ const auto *A = dyn_cast<TryAcquireCapabilityAttr>(At);
+ if (!A)
+ continue;
+ bool Success = getTrySuccessValue(D->getASTContext(), A->getSuccessValue());
+ CapExprSet &Group =
+ Success ? (A->isShared() ? Caps.TruthyShared : Caps.TruthyExclusive)
+ : (A->isShared() ? Caps.FalsyShared : Caps.FalsyExclusive);
+ CapExprSet AttrCaps;
+ getMutexIDs(AttrCaps, A, Exp, D, Self);
+ for (const auto &M : AttrCaps)
+ Group.push_back_nodup(M);
+ }
+}
+
+/// Populate TryAcquireCapsMap for every try-acquire CallExpr in the
+/// function, before the lockset walk: a branch on a stored result can
+/// precede the call in block order (a loop-top check `if (ok)` above
+/// `ok = mu.TryLock()`), and the terminator decode (decodeTrylockBranch)
+/// folds the record into its memoized per-capability resolutions. The
+/// variable map's per-statement contexts are complete by now
+/// (from traverseCFG), so each call's attributes translate in the call's own
+/// post-context by replaying the saved contexts block by block.
+/// Constructors are excluded: they record in handleCall, where the
+/// constructed-object placeholder is available.
+void ThreadSafetyAnalyzer::recordTryAcquireCalls(
+ const PostOrderCFGView *SortedGraph) {
+ for (const CFGBlock *B : *SortedGraph) {
+ const CFGBlockInfo &Info = BlockInfo[B->getBlockID()];
+ unsigned CtxIndex = Info.EntryIndex;
+ LocalVariableMap::Context Ctx = Info.EntryContext;
+ for (const auto &BI : *B) {
+ std::optional<CFGStmt> CS = BI.getAs<CFGStmt>();
+ if (!CS)
+ continue;
+ const Stmt *S = CS->getStmt();
+ // Advance to the post-context of S; a no-op for statements the
+ // variable map saved no context for.
+ Ctx = LocalVarMap.getNextContext(CtxIndex, S, Ctx);
+ const auto *CE = dyn_cast<CallExpr>(S);
+ if (!CE)
+ continue;
+ const auto *D = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl());
+ if (!D || !D->hasAttr<TryAcquireCapabilityAttr>())
+ continue;
+ // Mirror BuildLockset's post-context attribute translation.
+ if (Handler.issueBetaWarnings())
+ SxBuilder.setLookupLocalVarExpr(
+ [Ctx, this](const NamedDecl *VD) mutable -> const Expr * {
+ return LocalVarMap.lookupExpr(VD, Ctx);
+ });
+ recordTryAcquireCall(CE, D);
+ }
+ }
+ if (Handler.issueBetaWarnings())
+ SxBuilder.setLookupLocalVarExpr(nullptr);
+}
+
/// Check a function's CFG for thread-safety violations.
///
/// We traverse the blocks in the CFG, compute the set of mutexes that are held
@@ -3045,6 +3494,10 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
}
}
+ // Record the capabilities of every try-acquire call, recorded in the exact
+ // context of that call.
+ recordTryAcquireCalls(SortedGraph);
+
// Compute the expected exit set.
// By default, we expect all locks held on entry to be held on exit.
FactSet ExpectedFunctionExitSet = Initial.EntrySet;
@@ -3085,10 +3538,18 @@ 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;
+ // The try-acquire call whose result this block's terminator branches
+ // on, if any. Computed lazily on the first join of sets that carry a
+ // try-acquire fact at all.
+ const CallExpr *RebranchTryLock = nullptr;
+ bool RebranchTryLockComputed = false;
+ auto HasTryLockFact = [this](const FactSet &FS) {
+ // TryAcquireCapsMap is empty in functions without try-acquires (the
+ // common case): skip scanning the fact sets entirely.
+ return !TryAcquireCapsMap.empty() && llvm::any_of(FS, [this](FactID ID) {
+ return FactMan[ID].tryLockCall();
+ });
+ };
for (CFGBlock::const_pred_iterator PI = CurrBlock->pred_begin(),
PE = CurrBlock->pred_end(); PI != PE; ++PI) {
// if *PI -> CurrBlock is a back edge
@@ -3122,16 +3583,21 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
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;
+ // Branch join: a difference in the facts created by a try-acquire
+ // is demoted to try-held and re-resolved on the outgoing edges if
+ // the terminator branches on that call's result.
+ if (!RebranchTryLockComputed &&
+ (HasTryLockFact(CurrBlockInfo->EntrySet) ||
+ HasTryLockFact(PrevLockset))) {
+ // Compute once; the result depends only on CurrBlock, not on
+ // *PI. Skipped entirely (the common case) until some fact at
+ // this join originates from a try-acquire.
+ RebranchTryLock = decodeTrylockBranch(CurrBlock).TrylockCall;
+ RebranchTryLockComputed = true;
}
intersectAndWarn(CurrBlockInfo->EntrySet, PrevLockset,
CurrBlockInfo->EntryLoc, LEK_LockedSomePredecessors,
- LEK_LockedSomePredecessors, &TerminatorTrylockCaps);
+ LEK_LockedSomePredecessors, RebranchTryLock);
}
}
}
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index d0500a6defd64..696b63c19a8fb 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2042,11 +2042,12 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
}
void handleUnmatchedUnlock(StringRef Kind, Name LockName, SourceLocation Loc,
- SourceLocation LocPreviousUnlock) override {
+ SourceLocation LocPreviousUnlock,
+ bool MaybeHeld) override {
if (Loc.isInvalid())
Loc = FunLocation;
PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_unlock_but_no_lock)
- << Kind << LockName);
+ << Kind << LockName << MaybeHeld);
Warnings.emplace_back(std::move(Warning),
makeUnlockedHereNote(LocPreviousUnlock, Kind));
}
@@ -2065,11 +2066,12 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
}
void handleDoubleLock(StringRef Kind, Name LockName, SourceLocation LocLocked,
- SourceLocation LocDoubleLock) override {
+ SourceLocation LocDoubleLock, bool MaybeHeld) override {
if (LocDoubleLock.isInvalid())
LocDoubleLock = FunLocation;
PartialDiagnosticAt Warning(LocDoubleLock, S.PDiag(diag::warn_double_lock)
- << Kind << LockName);
+ << Kind << LockName
+ << MaybeHeld);
Warnings.emplace_back(std::move(Warning),
makeLockedHereNote(LocLocked, Kind));
}
@@ -2292,9 +2294,10 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
}
void handleFunExcludesLock(StringRef Kind, Name FunName, Name LockName,
- SourceLocation Loc) override {
+ SourceLocation Loc, bool MaybeHeld) override {
PartialDiagnosticAt Warning(Loc, S.PDiag(diag::warn_fun_excludes_mutex)
- << Kind << FunName << LockName);
+ << Kind << FunName << LockName
+ << MaybeHeld);
Warnings.emplace_back(std::move(Warning), getNotes());
}
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index a58b0d24e4239..8c337e784567c 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1979,6 +1979,7 @@ struct TestTryLock {
Mutex mu;
Mutex mu2;
int a GUARDED_BY(mu);
+ int a2 GUARDED_BY(mu2);
bool cond;
void foo1() {
@@ -2220,9 +2221,332 @@ struct TestTryLock {
mu.Unlock(); // expected-warning{{releasing mutex 'mu' that was not held}}
}
+ // An unconditional acquire upgrades a try-held capability to held.
+ void tryheld_upgrade() {
+ mu.TryLock(); // expected-note {{mutex acquired here}}
+ mu.Lock(); // expected-warning {{acquiring mutex 'mu' that may already be held}}
+ a = 1;
+ mu.Unlock();
+ }
+
+ // The same upgrade with the kinds swapped.
+ void tryheld_upgrade_kind_mismatch() {
+ mu.ReaderTryLock(); // expected-note {{mutex acquired here}}
+ mu.Lock(); // expected-warning {{acquiring mutex 'mu' that may already be held}}
+ a = 1;
+ mu.Unlock();
+ }
+
+ // No warning when asserting held (which upgrades the try silently).
+ void tryheld_assert_upgrade() {
+ mu.TryLock();
+ mu.AssertHeld();
+ a = 1;
+ mu.Unlock();
+ }
+
+ // Releasing a capability whose try-acquire result was never checked may
+ // release a capability that is not held.
+ void tryheld_unlock_unchecked() {
+ mu.TryLock();
+ mu.Unlock(); // expected-warning {{releasing mutex 'mu' that may not be held}}
+ }
+
+ // After a mismatched unlock, the state still changes from try-lock to
+ // unlocked.
+ void tryheld_unlock_unchecked_then_again() {
+ mu.TryLock();
+ mu.Unlock(); // expected-warning {{releasing mutex 'mu' that may not be held}} \
+ // expected-note {{mutex released here}}
+ mu.Unlock(); // expected-warning {{releasing mutex 'mu' that was not held}}
+ }
+
+ // A spin-acquire resolves the state on both loop edges.
+ void tryheld_spin() {
+ while (!mu.TryLock());
+ a = 1;
+ mu.Unlock();
+ }
+
+ // The join suppression is keyed to the specific try-acquire call whose
+ // result the terminator re-branches on, not to the capability it names:
+ // leaking the lock from an earlier try-acquire of the same mutex must
+ // still warn at a join whose terminator tests a later call's result.
+ void tryheld_rebranch_identity() {
+ if (mu.TryLock()) // expected-note {{mutex acquired here}}
+ cond = true; // leaks the successfully acquired lock
+ bool b = mu.TryLock(); // expected-warning {{mutex 'mu' is not held on every path through here}}
+ if (b) // re-branches on the second call, not the first
+ mu.Unlock();
+ }
+
+ // A try-held capability may be held: uses that require it to be excluded are
+ // rejected in the try region, with "may be held" wording.
+ void needsNotMu() EXCLUSIVE_LOCKS_REQUIRED(!mu);
+ void excludesMu() LOCKS_EXCLUDED(mu);
+
+ void tryheld_negative_requires() {
+ bool b = mu.TryLock();
+ needsNotMu(); // expected-warning {{cannot call function 'needsNotMu' while mutex 'mu' may be held}}
+ if (b)
+ mu.Unlock();
+ }
+
+ void tryheld_excludes() {
+ bool b = mu.TryLock();
+ excludesMu(); // expected-warning {{cannot call function 'excludesMu' while mutex 'mu' may be held}}
+ if (b)
+ mu.Unlock();
+ }
+
+ // On the resolved branches the state is definite again.
+ void tryheld_excludes_on_failure_path() {
+ bool b = mu.TryLock();
+ if (b) {
+ a = 1;
+ mu.Unlock();
+ } else {
+ excludesMu();
+ }
+ }
+
+ // A second try-acquire of the same capability while it is try-held cannot
+ // be tracked separately, so it generates warnings and the fact is dropped.
+ void tryheld_nested_try() {
+ bool b1 = mu.TryLock(); // expected-note {{mutex acquired here}}
+ bool b2 = mu.TryLock(); // expected-warning {{acquiring mutex 'mu' that may already be held}}
+ if (b2) {
+ a = 1; // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+ mu.Unlock(); // expected-warning {{releasing mutex 'mu' that may not be held}}
+ }
+ if (b1) {
+ a = 2;
+ mu.Unlock();
+ }
+ }
+
+ // Held on one path but only try-held on the other: without a branch on
+ // the try-acquire result the definite state is lost at the join, which is
+ // diagnosed like a path that does not hold the capability at all. The
+ // weaker try-held fact survives the join, so the release is also
+ // diagnosed.
+ void tryheld_join_with_held() {
+ if (cond)
+ mu.Lock(); // expected-note {{mutex acquired here}}
+ else
+ mu.TryLock();
+ a = 1; // 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 may not be held}}
+ }
+
+ // A try-acquire on the loop back edge does not restore the held state the
+ // loop was entered with.
+ void tryheld_loop_back_edge() {
+ mu.Lock(); // expected-note {{mutex acquired here}}
+ while (cond) { // expected-warning {{expecting mutex 'mu' to be held at start of each loop}}
+ mu.Unlock();
+ mu.TryLock();
+ }
+ mu.Unlock();
+ }
+
+ // A try-held capability does not fulfill an acquire-capability promise.
+ void tryheld_exit_expected() EXCLUSIVE_LOCK_FUNCTION(mu) { // expected-note {{mutex acquired here}}
+ mu.TryLock();
+ } // expected-warning {{expecting mutex 'mu' to be held at the end of function}}
+
+ // An assert (or acquire) upgrade does not keep the try-acquire origin on the
+ // promoted fact: the upgraded hold is not proved by the call's success, so a
+ // branch on the (now stale) result must not resolve it. Branching on it
+ // afterwards leaves the assert-claimed hold untouched on both edges -- the
+ // success edge is not a second acquisition -- and the conditional release is
+ // absorbed by the asserted fact's join exemptions.
+ void tryheld_assert_then_branch() {
+ bool b = mu.TryLock();
+ mu.AssertHeld();
+ a = 1;
+ if (b)
+ mu.Unlock();
+ }
+
+ // A try-acquire of a capability that is provably held by an earlier acquire
+ // deepens the hold instead of colliding, even though the capability is not
+ // reentrant: at runtime the call simply fails (it cannot deadlock). The
+ // extra level is conditionally held until the branch on the result, to
+ // still enforce that the shape of the (likely dead) code is correct.
+ void tryheld_trylock_over_locked() {
+ mu.Lock();
+ if (mu.TryLock())
+ mu.Unlock();
+ mu.Unlock();
+ }
+
+ // As above without a release inside the success branch: that branch leaks
+ // the extra level, which the join diagnoses as a reentrancy-depth mismatch;
+ // the deeper fact wins the join, so the final release unwinds only the extra
+ // level and the function exits still holding the base.
+ void tryheld_trylock_over_locked_no_release() {
+ mu.Lock(); // expected-note 2 {{mutex acquired here}}
+ if (mu.TryLock()) {
+ }
+ mu.Unlock(); // expected-warning {{mutex 'mu' is not held on every path through here with equal reentrancy depth}}
+ } // expected-warning {{mutex 'mu' is still held at the end of function}}
+
+ // A shared try-acquire over a shared hold deepens the same way.
+ void tryheld_reader_trylock_over_reader_locked() {
+ mu.ReaderLock();
+ if (mu.ReaderTryLock())
+ mu.ReaderUnlock();
+ mu.ReaderUnlock();
+ }
+
+ // A try-acquire of a different kind over a definite hold cannot deepen the
+ // single fact (it carries one lock kind), so the conflict is still diagnosed
+ // at the call.
+ void tryheld_trylock_kind_mismatch_over_locked() {
+ mu.ReaderLock(); // expected-note {{mutex acquired here}}
+ if (mu.TryLock()) { // expected-warning {{acquiring mutex 'mu' that is already held}}
+ }
+ mu.ReaderUnlock();
+ }
+
+ // Check that joins hold the intended facts in both directions.
+ void tryheld_merged_hold_double_release() {
+ bool b = mu.TryLock();
+ if (!b)
+ mu.Lock(); // expected-note {{mutex acquired here}}
+ if (b)
+ mu.Unlock();
+ mu.Unlock(); // expected-warning {{mutex 'mu' is not held on every path through here}} \
+ // expected-warning {{releasing mutex 'mu' that was not held}}
+ }
+
+ // A try-acquire attempts the acquisition, so acquired_before/after
+ // ordering is checked at the call like an unconditional acquire.
+ Mutex mu_after ACQUIRED_AFTER(mu2);
+ void tryheld_acquired_after() {
+ mu_after.Lock();
+ if (mu2.TryLock()) { // expected-warning {{mutex 'mu2' must be acquired before 'mu_after'}}
+ mu2.Unlock();
+ }
+ mu_after.Unlock();
+ }
+
+ // A function may carry several try-acquire attributes with different
+ // success values; each capability resolves with its own attribute's
+ // polarity: mu is held only on the true branch, mu2 only on the false
+ // branch.
+ bool TryLockSplit() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu)
+ EXCLUSIVE_TRYLOCK_FUNCTION(false, mu2);
+ void tryheld_mixed_success_values() {
+ if (TryLockSplit()) {
+ a = 1;
+ a2 = 1; // expected-warning {{writing variable 'a2' requires holding mutex 'mu2' exclusively}}
+ mu.Unlock();
+ } else {
+ a2 = 1;
+ a = 1; // expected-warning {{writing variable 'a' requires holding mutex 'mu' exclusively}}
+ mu2.Unlock();
+ }
+ }
+
+ void tryheld_assign_as_condition() {
+ bool b;
+ if ((b = mu.TryLock())) {
+ a = 1;
+ mu.Unlock();
+ }
+ }
+
+ void tryheld_assign_as_condition_short_circuit() {
+ bool ok;
+ while (cond && (ok = mu.TryLock())) {
+ a = 1;
+ mu.Unlock();
+ }
+ }
+
static void fail() __attribute__((noreturn));
}; // end TestTrylock
+// A try-acquire's facts record the capabilities as translated at the call;
+// re-translating the attribute arguments at the branch on its result can
+// name a different capability (here: through a reassigned pointer).
+namespace TryLockPointerRetranslation {
+
+Mutex pmu1, pmu2;
+int pdata GUARDED_BY(pmu1);
+bool try_lock_ptr(Mutex *m) EXCLUSIVE_TRYLOCK_FUNCTION(true, m);
+
+void ok() {
+ Mutex *p = &pmu1;
+ bool b = try_lock_ptr(p);
+ p = &pmu2;
+ if (b) {
+ pdata = 1;
+ pmu1.Unlock();
+ }
+}
+
+// The success path releases the wrong mutex and leaks the acquired one.
+void leak() {
+ Mutex *p = &pmu1;
+ bool b = try_lock_ptr(p); // expected-note {{mutex acquired here}}
+ p = &pmu2;
+ if (b)
+ p->Unlock(); // expected-warning {{releasing mutex 'pmu2' that was not held}}
+} // expected-warning {{mutex 'pmu1' is not held on every path through here}}
+
+// With mixed success values each fact is re-identified by matching its
+// capability against the ones recorded at the call, so the reassignment
+// does not keep the facts from resolving, nor swap their polarities.
+bool try_lock_ptr_split(Mutex *m1, Mutex *m2)
+ EXCLUSIVE_TRYLOCK_FUNCTION(true, m1) EXCLUSIVE_TRYLOCK_FUNCTION(false, m2);
+
+void mixed_success_after_reassignment() {
+ Mutex *p = &pmu1;
+ bool b = try_lock_ptr_split(p, &pmu2);
+ p = &pmu2;
+ if (b) {
+ pdata = 1;
+ pmu1.Unlock();
+ } else {
+ pmu2.Unlock();
+ }
+}
+
+} // end namespace TryLockPointerRetranslation
+
+// A try-acquire attribute's success value is decoded by constant evaluation,
+// which handles template parameters for example.
+namespace TrySuccessValueConstant {
+
+Mutex smu;
+
+template <bool S>
+bool try_lock_templ() EXCLUSIVE_TRYLOCK_FUNCTION(S, smu);
+
+void success_true() {
+ if (try_lock_templ<true>())
+ smu.Unlock();
+}
+
+void success_false() {
+ if (!try_lock_templ<false>())
+ smu.Unlock();
+}
+
+void failure_edge() {
+ if (try_lock_templ<false>()) {
+ smu.Unlock(); // expected-warning {{releasing mutex 'smu' that was not held}}
+ } else {
+ smu.Unlock();
+ }
+}
+
+} // end namespace TrySuccessValueConstant
+
} // end namespace TrylockTest
@@ -3077,6 +3401,23 @@ void deferLock() {
x = 3;
}
+// Check for warnings even through managed mutexes.
+bool tryLockMu() EXCLUSIVE_TRYLOCK_FUNCTION(true, mu);
+
+void deferredUnlockOverTryHeld() {
+ RelockableExclusiveMutexLock scope(&mu, DeferTraits{});
+ bool ok = tryLockMu();
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that may not be held}}
+}
+
+void deferredRelockOverTryHeld() {
+ RelockableExclusiveMutexLock scope(&mu, DeferTraits{});
+ bool ok = tryLockMu(); // expected-note {{mutex acquired here}}
+ scope.Lock(); // expected-warning {{acquiring mutex 'mu' that may already be held}}
+ x = 1; // expected-warning {{writing variable 'x' requires holding mutex 'mu' exclusively}}
+ scope.Unlock(); // expected-warning {{releasing mutex 'mu' that may not be held}}
+}
+
void relockExclusive() {
RelockableMutexLock scope(&mu, SharedTraits{});
print(x);
@@ -7346,6 +7687,85 @@ void testReentrantTryLock2() {
guardby_var = 1; // expected-warning{{writing variable 'guardby_var' requires holding mutex 'rmu' exclusively}}
}
+// Analysis can only track one try-lock at a time. A second one, while still
+// tracking the first, should get a warning.
+void testReentrantTryLockSequential() {
+ bool b1 = rmu.TryLock(); // expected-note {{mutex acquired here}}
+ bool b2 = rmu.TryLock(); // expected-warning {{acquiring mutex 'rmu' that may already be held}}
+ if (b2) {
+ guardby_var = 1; // expected-warning {{writing variable 'guardby_var' requires holding mutex 'rmu' exclusively}}
+ rmu.Unlock(); // expected-warning {{releasing mutex 'rmu' that may not be held}}
+ }
+ if (b1) {
+ guardby_var = 1;
+ rmu.Unlock();
+ }
+}
+
+// Analysis can only track one kind at a time. A second attempt to try-acquire
+// a different kind conflicts with the ability to track it, requiring a warning.
+void testReentrantTryLockKindMismatch() {
+ rmu.Lock(); // expected-note {{mutex acquired here}}
+ if (rmu.ReaderTryLock()) { // expected-warning {{acquiring mutex 'rmu' that is already held}}
+ }
+ rmu.Unlock();
+}
+
+void testReentrantTryLockUpgradeKindMismatch() {
+ rmu.ReaderTryLock(); // expected-note {{mutex acquired here}}
+ rmu.Lock(); // expected-warning {{acquiring mutex 'rmu' that may already be held}}
+ guardby_var = 1;
+ rmu.Unlock();
+}
+
+// A join of facts with unequal reentrancy depths warns once, then keeps the
+// fact that guarantees more, to minimize follow-on warnings: the guarded
+// write under !b is covered by the definite level the failure edge leaves,
+// while the depth imbalance is re-diagnosed at the later joins.
+void testReentrantTryLockConditionalDeepen(bool c) {
+ bool b = rmu.TryLock(); // expected-note 4 {{mutex acquired here}}
+ if (c)
+ rmu.Lock();
+ if (!b) // expected-warning {{mutex 'rmu' is not held on every path through here with equal reentrancy depth}}
+ guardby_var = 1;
+ if (b) // expected-warning {{mutex 'rmu' is not held on every path through here with equal reentrancy depth}}
+ rmu.Unlock();
+} // expected-warning {{mutex 'rmu' is not held on every path through here with equal reentrancy depth}} \
+ // expected-warning {{mutex 'rmu' is still held at the end of function}}
+
+// As above; the kept deeper fact absorbs both releases that the deepening
+// path had balanced, and the imbalance against the shallow path surfaces
+// at the joins instead.
+void testReentrantTryLockBranchDeepen() {
+ bool b = rmu.TryLock(); // expected-note 3 {{mutex acquired here}}
+ if (b)
+ rmu.Lock();
+ if (!b) // expected-warning {{mutex 'rmu' is not held on every path through here with equal reentrancy depth}}
+ guardby_var = 1;
+ if (b) { // expected-warning {{mutex 'rmu' is not held on every path through here with equal reentrancy depth}}
+ rmu.Unlock();
+ rmu.Unlock();
+ }
+} // expected-warning {{mutex 'rmu' is not held on every path through here}}
+
+// A held fact joining a try-held fact of the same origin merges to
+// try-held (the re-branch below re-resolves it); the depth mismatch is
+// diagnosed once, and the extra level the deepening path acquired
+// surfaces at its extra release.
+void testReentrantTryLockMixedDepthJoin(bool c) {
+ bool b = rmu.TryLock(); // expected-note {{mutex acquired here}}
+ if (c) {
+ if (!b)
+ return;
+ rmu.Lock();
+ guardby_var = 1;
+ }
+ if (b) { // expected-warning {{mutex 'rmu' is not held on every path through here with equal reentrancy depth}}
+ rmu.Unlock(); // expected-note {{mutex released here}}
+ rmu.Unlock(); // expected-warning {{releasing mutex 'rmu' that was not held}}
+ }
+}
+
void testReentrantNotHeld() {
rmu.Unlock(); // \
// expected-warning{{releasing mutex 'rmu' that was not held}}
diff --git a/clang/test/SemaCXX/warn-thread-safety-negative.cpp b/clang/test/SemaCXX/warn-thread-safety-negative.cpp
index 0caf6d6139e58..555afa23a64eb 100644
--- a/clang/test/SemaCXX/warn-thread-safety-negative.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-negative.cpp
@@ -121,6 +121,47 @@ class Reentrant {
}
};
+class TryLockTest {
+ Mutex mu;
+ int a GUARDED_BY(mu);
+
+public:
+ // The analysis of a TryLock expects to have !mu declared at the boundaries,
+ // even though inside a function this recursion pattern is permitted without
+ // a warning.
+ void tryLockNegativeWarn() {
+ if (mu.TryLock()) { // expected-warning{{acquiring mutex 'mu' requires negative capability '!mu'}}
+ a = 0;
+ mu.Unlock();
+ }
+ }
+
+ // Inside a REQUIRES(!mu) region the declared negative fact satisfies the
+ // attempt; the success edge consumes it (no duplicate '!mu' facts, no
+ // spurious diagnostics), and the failure path retains it.
+ void tryLockNegativeSatisfied() EXCLUSIVE_LOCKS_REQUIRED(!mu) {
+ if (mu.TryLock()) {
+ a = 0;
+ mu.Unlock();
+ } else {
+ needsNegative();
+ }
+ }
+
+ // Releasing an unchecked try-acquire is diagnosed at the release, and the
+ // thread provably does not hold the capability afterwards: the release
+ // establishes the negative fact, so re-acquiring does not also warn.
+ void tryLockUncheckedReleaseThenLock() {
+ mu.TryLock(); // expected-warning{{acquiring mutex 'mu' requires negative capability '!mu'}}
+ mu.Unlock(); // expected-warning{{releasing mutex 'mu' that may not be held}}
+ mu.Lock(); // no '!mu' warning: the release above proves it
+ a = 0;
+ mu.Unlock();
+ }
+
+ void needsNegative() EXCLUSIVE_LOCKS_REQUIRED(!mu);
+};
+
} // end namespace SimpleTest
Mutex globalMutex;
More information about the llvm-branch-commits
mailing list