[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -3990,6 +3990,13 @@ def LocksExcluded : InheritableAttr {
let Documentation = [Undocumented];
}
+def ReentrantCapability : InheritableAttr {
+ let Spellings = [Clang<"reentrant_capability">];
+ let Subjects = SubjectList<[Record, TypedefName]>;
+ let Documentation = [Undocumented];
philnik777 wrote:
IMO we could at least add a link to the other documentation. Just saying that
it's undocumented in the place where all other attributes are documented is
really not helpful.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
melver wrote:
I think both angles have merit. Let's try to introduce a new "pedantic" warning
class, which we'll enable by default:
https://github.com/llvm/llvm-project/pull/141599 -- those who know what they
are doing are free to disable the warning.
I'm willing to compromise on removing the "pedantic" warning class altogether,
although I still think the combination can be helpful, hence the "pedantic"
warning. ;-)
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
aaronpuchert wrote:
While I see your point, this is all a bit vague. The purpose of negative
capabilities is
[documented](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) as
helping to prevent double locking, which is not an issue for reentrant mutexes.
> I've been on the receiving end of such forced restrictions, and it's rather
> annoying
The other side of the coin is that it is very hard to make the analysis more
strict after we've released a feature, while making it more permissive is not a
problem. Having done a few breaking changes myself, it's something that I'd
like to avoid as much as possible.
That's why I tend to err on the side of making the analysis as strict as it can
be while still being useful, and if that causes too much friction relax it
later.
In this case, the restriction doesn't seem arbitrary at all. The combination
simply doesn't make sense by the documented purpose of negative capabilities.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver closed https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From b8754a894e8822c43dfce62b7d13d5169ea4a215 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +-
.../Analysis/Analyses/ThreadSafetyCommon.h| 22 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 7 +-
clang/lib/Analysis/ThreadSafety.cpp | 133 +--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 76 ++--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 +-
clang/lib/Sema/SemaDeclAttr.cpp | 18 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 +
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 364 ++
.../SemaCXX/warn-thread-safety-parsing.cpp| 12 +
clang/unittests/AST/ASTImporterTest.cpp | 9 +
16 files changed, 624 insertions(+), 77 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 75f529db7260e..780716b089e41 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -447,6 +447,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..4fc7ff28e9931 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -434,6 +434,21 @@ class can be used as a capability. The string argument
specifies the kind of
capability in error messages, e.g. ``"mutex"``. See the ``Container`` example
given above, or the ``Mutex`` class in :ref:`mutexheader`.
+REENTRANT_CAPABILITY
+
+
+``REENTRANT_CAPABILITY`` is an attribute on capability classes, denoting that
+they are reentrant. Marking a capability as reentrant means that acquiring the
+same capability multiple times is safe. Acquiring the same capability with
+different access privileges (exclusive vs. shared) again is not considered
+reentrant by the analysis.
+
+Note: In many cases this attribute is only required where a capability is
+acquired reentrant within the same function, such as via macros or other
+helpers. Otherwise, best practice is to avoid explicitly acquiring a capability
+multiple times within the same function, and letting the analysis produce
+warnings on double-acquisition attempts.
+
.. _scoped_capability:
SCOPED_CAPABILITY
@@ -846,6 +861,9 @@ implementation.
#define CAPABILITY(x) \
THREAD_ANNOTATION_ATTRIBUTE__(capability(x))
+ #define REENTRANT_CAPABILITY \
+THREAD_ANNOTATION_ATTRIBUTE__(reentrant_capability)
+
#define SCOPED_CAPABILITY \
THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 65a91483562e0..9fb23212aaf97 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -166,10 +166,12 @@ class ThreadSafetyHandler {
/// \param LocEndOfScope -- The location of the end of the scope where the
/// mutex is no longer held
/// \param LEK -- which of the three above cases we should warn for
+ /// \param ReentrancyMismatch -- mismatching reentrancy depth
virtual void handleMutexHeldEndOfScope(StringRef Kind, Name LockName,
SourceLocation LocLocked,
SourceLocation LocEndOfScope,
-
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From bce9df281e5ea7c2efd9c880f791f6572732c31d Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From c4d362a060123eb77b1c684819b7ca02b1979a13 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +-
.../Analysis/Analyses/ThreadSafetyCommon.h| 22 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 7 +-
clang/lib/Analysis/ThreadSafety.cpp | 133 +--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 76 ++--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 +-
clang/lib/Sema/SemaDeclAttr.cpp | 18 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 +
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 364 ++
.../SemaCXX/warn-thread-safety-parsing.cpp| 12 +
clang/unittests/AST/ASTImporterTest.cpp | 9 +
16 files changed, 624 insertions(+), 77 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 75f529db7260e..780716b089e41 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -447,6 +447,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..4fc7ff28e9931 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/aaronpuchert edited https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -235,6 +266,20 @@ class FactSet {
return false;
}
+ std::optional replaceLock(FactManager &FM, iterator It,
+std::unique_ptr Entry) {
+if (It == end())
+ return std::nullopt;
+FactID F = FM.newFact(std::move(Entry));
+*It = F;
+return F;
+ }
+
+ std::optional replaceLock(FactManager &FM, const CapabilityExpr
&CapE,
+std::unique_ptr Entry) {
+return replaceLock(FM, findLockIter(FM, CapE), std::move(Entry));
+ }
+
aaronpuchert wrote:
Well, I guess we can clean this up later.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/aaronpuchert approved this pull request. I think this looks good, thanks for your contribution! https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
melver wrote: As additional motivation - quote from a kernel maintainer: > But I think we should get the infrastructure in once your reentrancy > support has landed in a release, because with that we can start > annotation some code and show uses, while also helping to driver more > requirements. So there's hope we don't have to also get alias analysis before the Linux kernel use can proceed. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From b264872c3f28f6cf172b0123087adda9d53dc1b9 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From 08b170299096ed4c49a7d52d169510a6ecb567fe Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +-
.../Analysis/Analyses/ThreadSafetyCommon.h| 22 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 7 +-
clang/lib/Analysis/ThreadSafety.cpp | 133 +--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 76 ++--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 +-
clang/lib/Sema/SemaDeclAttr.cpp | 18 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 +
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 364 ++
.../SemaCXX/warn-thread-safety-parsing.cpp| 12 +
clang/unittests/AST/ASTImporterTest.cpp | 9 +
16 files changed, 624 insertions(+), 77 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f04cb7b91788c..e87f04fe1cdd0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -443,6 +443,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..4fc7ff28e9931 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
melver wrote: Gentle ping - PTAL. Many thanks! https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
melver wrote: Thanks for the feedback. Addressed comments as best as I could. Most notable changes: - Also warns properly for loops with mismatching reentrancy depth. - Devirtualized new helpers. - Require ordering `reentrant_capability` after `capability`. - Stylistic improvements. PTAL. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -235,6 +266,20 @@ class FactSet {
return false;
}
+ std::optional replaceLock(FactManager &FM, iterator It,
+std::unique_ptr Entry) {
+if (It == end())
+ return std::nullopt;
+FactID F = FM.newFact(std::move(Entry));
+*It = F;
+return F;
+ }
+
+ std::optional replaceLock(FactManager &FM, const CapabilityExpr
&CapE,
+std::unique_ptr Entry) {
+return replaceLock(FM, findLockIter(FM, CapE), std::move(Entry));
+ }
+
melver wrote:
replaceLock without caller-iterator still saves a copy+pop_back+push_back. Not
much, but not nothing either.
This PR should include the bare minimum use of replaceLock - I have one patch
queued that is not part of this PR to do an unrelated substitution with
replaceLock.
If anything I'd move replaceLock introduction before this PR, but I think we're
just splitting hairs at that point.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +272,34 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
+ CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg, bool Reentrant)
+ : CapExpr(E, (Neg ? FlagNegative : 0) | (Reentrant ? FlagReentrant : 0)),
+CapKind(Kind) {}
+ CapabilityExpr(const til::SExpr *E, QualType QT, bool Neg);
melver wrote:
It's a bit of a maze, but I felt this is a decent trade-off. Less/simpler code
traded off against (minimal) loss of abstraction.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -1011,6 +979,30 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
IncompleteArgs.clear();
}
+static CapabilityExpr makeCapabilityExpr(const til::SExpr *E, QualType VDT,
+ bool Neg) {
+ // We need to look at the declaration of the type of the value to determine
+ // which it is. The type should either be a record or a typedef, or a pointer
+ // or reference thereof.
+ if (const auto *RT = VDT->getAs()) {
+if (const auto *RD = RT->getDecl())
+ if (const auto *CA = RD->getAttr())
+return CapabilityExpr(E, CA->getName(), Neg,
+ RD->hasAttr());
+ } else if (const auto *TT = VDT->getAs()) {
+if (const auto *TD = TT->getDecl())
+ if (const auto *CA = TD->getAttr())
+return CapabilityExpr(E, CA->getName(), Neg,
+ TD->hasAttr());
+ } else if (VDT->isPointerOrReferenceType())
+return makeCapabilityExpr(E, VDT->getPointeeType(), Neg);
+
+ return CapabilityExpr(E, StringRef("mutex"), Neg, false);
+}
melver wrote:
I'll rework this, trying to improve readability here as well.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
melver wrote:
There might be valid uses for that, even if it's not something we'd expect to
be frequent. Developer creativity how to use a feature can sometimes lead to
new uses we never expected, and I wouldn't want to prohibit them.
If it's *not wrong semantically*, I tend to not want to impose a certain
pattern and imply to the user "you're holding it wrong" even though it's not
strictly wrong and they might actually know what they're doing. I've been on
the receiving end of such forced restrictions, and it's rather annoying -
though quite often it's because the restriction allowed for a simpler
implementation, I'd argue in this case that's a moot point.
I think if we were talking about synchronization only, I'd be more inclined to
say "this never makes sense", but the feature is now about arbitrary
capabilities.
For example, we might want to express that some capability is never held when
entering a function for performance reasons, yet that capability itself can be
reentrantly acquired on slow paths. I don't see a reason to prohibit uses like
that.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -4048,6 +4048,9 @@ def warn_thread_attribute_not_on_scoped_lockable_param :
Warning<
"%0 attribute applies to function parameters only if their type is a "
"reference to a 'scoped_lockable'-annotated type">,
InGroup, DefaultIgnore;
+def warn_reentrant_capability_without_capability : Warning<
+ "ignoring %0 attribute on %1 without 'capability' attribute">,
melver wrote:
I'll generalize the diagnostic and make both parameters, so it can be reused if
necessary. I think "requires" is not the right word here, because of the new
ordering constraint, so I'll choose "must be preceded".
```
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4048,8 +4048,8 @@ def warn_thread_attribute_not_on_scoped_lockable_param :
Warning<
"%0 attribute applies to function parameters only if their type is a "
"reference to a 'scoped_lockable'-annotated type">,
InGroup, DefaultIgnore;
-def warn_reentrant_capability_without_capability : Warning<
- "%0 attribute on %1 must be preceded by 'capability' attribute">,
+def warn_thread_attribute_requires_preceded : Warning<
+ "%0 attribute on %1 must be preceded by %2 attribute">,
InGroup, DefaultIgnore;
def err_attribute_argument_out_of_bounds_extra_info : Error<
"%0 attribute parameter %1 is out of bounds: "
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 68ddd286bce5..93d919102a9c 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -6249,8 +6249,8 @@ static void handleReentrantCapabilityAttr(Sema &S, Decl
*D,
// This helps enforce a canonical style. Also avoids placing an additional
// branch into ProcessDeclAttributeList().
if (!D->hasAttr()) {
-S.Diag(AL.getLoc(), diag::warn_reentrant_capability_without_capability)
-<< AL << cast(D);
+S.Diag(AL.getLoc(), diag::warn_thread_attribute_requires_preceded)
+<< AL << cast(D) << "'capability'";
return;
}
```
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -1011,6 +979,30 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
IncompleteArgs.clear();
}
+static CapabilityExpr makeCapabilityExpr(const til::SExpr *E, QualType VDT,
+ bool Neg) {
+ // We need to look at the declaration of the type of the value to determine
+ // which it is. The type should either be a record or a typedef, or a pointer
+ // or reference thereof.
+ if (const auto *RT = VDT->getAs()) {
+if (const auto *RD = RT->getDecl())
+ if (const auto *CA = RD->getAttr())
+return CapabilityExpr(E, CA->getName(), Neg,
+ RD->hasAttr());
+ } else if (const auto *TT = VDT->getAs()) {
+if (const auto *TD = TT->getDecl())
+ if (const auto *CA = TD->getAttr())
+return CapabilityExpr(E, CA->getName(), Neg,
+ TD->hasAttr());
+ } else if (VDT->isPointerOrReferenceType())
+return makeCapabilityExpr(E, VDT->getPointeeType(), Neg);
+
+ return CapabilityExpr(E, StringRef("mutex"), Neg, false);
+}
+
+CapabilityExpr::CapabilityExpr(const til::SExpr *E, QualType QT, bool Neg)
+: CapabilityExpr(makeCapabilityExpr(E, QT, Neg)) {}
aaronpuchert wrote:
In the header, `CapabilityExpr` seems to come before `SExprBuilder`, so this
should probably also come before `SExprBuilder` functions. Or at least before
`translateAttrExpr`, so that we can leave the classification in place.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver edited https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver edited https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver edited https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From a8319028f08192ca6140beed7f27a83a829c6d84 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From 427dcd2c396fbd454129abb3d3f2d572f87eadbc Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 13 +-
.../Analysis/Analyses/ThreadSafetyCommon.h| 21 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 6 +
clang/lib/Analysis/ThreadSafety.cpp | 134 +--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 66 ++--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 3 +
clang/lib/Sema/SemaDeclAttr.cpp | 10 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 +
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 356 ++
.../SemaCXX/warn-thread-safety-parsing.cpp| 7 +
clang/unittests/AST/ASTImporterTest.cpp | 9 +
16 files changed, 598 insertions(+), 75 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 80399e458aec3..2a6565e37e256 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -369,6 +369,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --g
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -388,7 +395,7 @@ class SExprBuilder {
til::LiteralPtr *createVariable(const VarDecl *VD);
// Create placeholder for this: we don't know the VarDecl on construction
yet.
- std::pair
+ std::pair
melver wrote:
I think this code is just more complex than it needs to be. I do not understand
the need for createThisPlaceholder():
- Placeholder.second is only used if there's a ScopedLockableAttr;
- Placeholder.first is equivalent to a createVariable(nullptr);
Therefore, we can just defer the second step (getting the string+bool out) for
when we determine it's a ScopedLockableAttr, and use createVariable(nullptr)
instead of createThisPlaceholder(). The main problem was that the function that
extracts the string (and now also the reentrant bool) wasn't available here.
So it'll just be much clearer if we have a constructor that does that for us,
and just use that rather than through this complex indirection of "prefetching"
the string+bool.
Also saves a few cycles if we simplify it. Let me try that.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -81,26 +81,25 @@ static bool isCalleeArrow(const Expr *E) {
return ME ? ME->isArrow() : false;
}
-static StringRef ClassifyDiagnostic(const CapabilityAttr *A) {
- return A->getName();
-}
-
-static StringRef ClassifyDiagnostic(QualType VDT) {
+static CapabilityExpr makeCapabilityExpr(const til::SExpr *E, QualType VDT,
melver wrote:
I'm simplifying this. The only place where it's not yet know if it's a
capability is the ScopedLockableAttr case. If we solve that, then we can have
makeCapabilityExpr() - or rather, just a constructor that takes QualType.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -235,6 +266,20 @@ class FactSet {
return false;
}
+ std::optional replaceLock(FactManager &FM, iterator It,
+std::unique_ptr Entry) {
+if (It == end())
+ return std::nullopt;
+FactID F = FM.newFact(std::move(Entry));
+*It = F;
+return F;
+ }
+
+ std::optional replaceLock(FactManager &FM, const CapabilityExpr
&CapE,
+std::unique_ptr Entry) {
+return replaceLock(FM, findLockIter(FM, CapE), std::move(Entry));
+ }
+
melver wrote:
We could, but it's less efficient, esp. if we used findLockIter():
1. without findLockIter: we save a pop_back() and push_back().
2. with preceding findLockIter: like (1), but also saves a search.
Sure, maybe we're just shaving off some constant overheads, but I also find it
makes it less verbose and more readable.
I have this later patch I would send out as a separate PR later:
```
commit 9a41f2b68a28e6a2239b45a11a0cd360dce320af
Author: Marco Elver
Date: Fri Apr 25 18:51:16 2025 +0200
Thread Safety Analysis: Use replaceLock instead of removeLock+addLock
In ScopedLockableFactEntry::unlock(), we can avoid a second search,
pop_back(), and push_back() if we use the already obtained iterator into
the FactSet to replace the old FactEntry and take its position in the
vector.
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index cb62f11d6f15..429f22715886 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1090,9 +1090,9 @@ private:
return;
}
- FSet.removeLock(FactMan, Cp);
- FSet.addLock(FactMan,
- std::make_unique(!Cp, LK_Exclusive,
loc));
+ FSet.replaceLock(
+ FactMan, It,
+ std::make_unique(!Cp, LK_Exclusive, loc));
} else if (Handler) {
SourceLocation PrevLoc;
if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
```
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
melver wrote:
We need 4 states.
I'm adding this test:
```
+class TestNegativeWithReentrantMutex {
+ ReentrantMutex rmu;
+ int a GUARDED_BY(rmu);
+
+public:
+ void baz() EXCLUSIVE_LOCKS_REQUIRED(!rmu) {
+rmu.Lock();
+a = 0;
+rmu.Unlock();
+ }
+};
```
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -114,31 +112,39 @@ class FactEntry : public CapabilityExpr {
};
private:
- const FactEntryKind Kind : 8;
+ const FactEntryKind Kind : 4;
/// Exclusive or shared.
- LockKind LKind : 8;
+ const LockKind LKind : 4;
+
+ /// How it was acquired.
+ const SourceKind Source : 4;
- // How it was acquired.
- SourceKind Source : 8;
+ /// Reentrancy depth; 16 bits should be enough given that FactID is a short,
+ /// and thus we can't store more than 65536 facts anyway.
+ unsigned int ReentrancyDepth : 16;
/// Where it was acquired.
- SourceLocation AcquireLoc;
+ const SourceLocation AcquireLoc;
public:
FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK,
SourceLocation Loc, SourceKind Src)
- : CapabilityExpr(CE), Kind(FK), LKind(LK), Source(Src), AcquireLoc(Loc)
{}
+ : CapabilityExpr(CE), Kind(FK), LKind(LK), Source(Src),
+ReentrancyDepth(0), AcquireLoc(Loc) {}
virtual ~FactEntry() = default;
LockKind kind() const { return LKind; }
SourceLocation loc() const { return AcquireLoc; }
FactEntryKind getFactEntryKind() const { return Kind; }
+ unsigned int getReentrancyDepth() const { return ReentrancyDepth; }
bool asserted() const { return Source == Asserted; }
bool declared() const { return Source == Declared; }
bool managed() const { return Source == Managed; }
+ virtual std::unique_ptr clone() const = 0;
aaronpuchert wrote:
Not sure if this makes sense. When we clone facts (to modify them), we should
always know which kind of fact we're dealing with. In our case it should always
be a `LockableFactEntry`. So you can just use `cast` and the copy constructor.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -168,6 +197,8 @@ class FactManager {
public:
FactID newFact(std::unique_ptr Entry) {
Facts.push_back(std::move(Entry));
+assert(Facts.size() - 1 <= std::numeric_limits::max() &&
aaronpuchert wrote:
```suggestion
assert(Facts.size() - 1 <= std::numeric_limits::max() &&
```
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -235,6 +266,20 @@ class FactSet {
return false;
}
+ std::optional replaceLock(FactManager &FM, iterator It,
+std::unique_ptr Entry) {
+if (It == end())
+ return std::nullopt;
+FactID F = FM.newFact(std::move(Entry));
+*It = F;
+return F;
+ }
+
+ std::optional replaceLock(FactManager &FM, const CapabilityExpr
&CapE,
+std::unique_ptr Entry) {
+return replaceLock(FM, findLockIter(FM, CapE), std::move(Entry));
+ }
+
aaronpuchert wrote:
Can't we just use `removeLock` and then `addLock`?
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/aaronpuchert commented: Two more things come to mind: * Consider adding a check to `SemaDeclAttr.cpp` that the new attribute is always accompanied by `capability` or `lockable`. Although I wonder whether that's too early. I'm not sure if we can see the other attributes already. * You should probably do something in `ThreadSafetyAnalyzer::intersectAndWarn`: if two branches join with different counts, we should warn and drop the fact with higher count. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -114,31 +112,39 @@ class FactEntry : public CapabilityExpr {
};
private:
- const FactEntryKind Kind : 8;
+ const FactEntryKind Kind : 4;
/// Exclusive or shared.
- LockKind LKind : 8;
+ const LockKind LKind : 4;
+
+ /// How it was acquired.
+ const SourceKind Source : 4;
aaronpuchert wrote:
We probably don't need these to be `const`. The `FactEntryKind` is `const`
because it encodes the actual dynamic type of the fact, which shouldn't change
after construction. The other fields can freely be changed, at least before we
move the fact into the `FactManager`, which applies `const` to the entire fact.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -114,31 +112,39 @@ class FactEntry : public CapabilityExpr {
};
private:
- const FactEntryKind Kind : 8;
+ const FactEntryKind Kind : 4;
/// Exclusive or shared.
- LockKind LKind : 8;
+ const LockKind LKind : 4;
+
+ /// How it was acquired.
+ const SourceKind Source : 4;
- // How it was acquired.
- SourceKind Source : 8;
+ /// Reentrancy depth; 16 bits should be enough given that FactID is a short,
+ /// and thus we can't store more than 65536 facts anyway.
+ unsigned int ReentrancyDepth : 16;
aaronpuchert wrote:
This should probably be in `LockableFactEntry`. I don't think we want to
support reentrant scoped locks, and they don't really make sense to me.
I understand that you want to pack these, but that's probably not so important:
we're looking at one function at a time, and will clean up everything before we
go check out the next function. The existing packing is just to make it not too
wasteful. (Also, computers have byte-addressable memory, so there is no need to
reserve an entire word if we only need a byte.)
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -388,7 +395,7 @@ class SExprBuilder {
til::LiteralPtr *createVariable(const VarDecl *VD);
// Create placeholder for this: we don't know the VarDecl on construction
yet.
- std::pair
+ std::pair
aaronpuchert wrote:
Nice idea, but I'm not sure if it's conceptually correct. We'll create these
also for objects that are not a capability, we only create the `CapabilityExpr`
when we see it has a `ScopedLockableAttr`.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
aaronpuchert wrote:
We do, but the question is whether we have 4 possible states or just 3: can a
mutex be both
[negative](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) and
reentrant? If yes, we have 2×`bool`, otherwise we have something like `enum
{Regular, Negative, Reentrant}`.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -163,15 +184,15 @@ using FactID = unsigned short;
/// the analysis of a single routine.
class FactManager {
private:
- std::vector> Facts;
+ std::vector> Facts;
aaronpuchert wrote:
The FactEntries themselves should never be replaced, they are immutable. What
you're doing now (correctly) is to replace the `FactID` indices in a `FactSet`.
But maybe that is what you meant.
They should probably not be stored in a `std::unique_ptr` but be allocated via
`BumpPtrAllocator`, but that's unrelated to your change. I've also thought
about putting them in a `std::variant`, but I think an arena might be even
better.
Yes, copying them is a bit inefficient, but I expect reentrancy within a single
function to be pretty rare. So we can probably pay the price. What makes the
analysis costly is mostly looking at things that have nothing to do with thread
safety.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -81,26 +81,25 @@ static bool isCalleeArrow(const Expr *E) {
return ME ? ME->isArrow() : false;
}
-static StringRef ClassifyDiagnostic(const CapabilityAttr *A) {
- return A->getName();
-}
-
-static StringRef ClassifyDiagnostic(QualType VDT) {
+static CapabilityExpr makeCapabilityExpr(const til::SExpr *E, QualType VDT,
aaronpuchert wrote:
Same as for `createThisPlaceholder`: at this point we don't know if we have a
capability yet. Probably better to produce a `pair`.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/aaronpuchert edited https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From d3324c1023533bf784a3c3c3ef095d07c865e6f9 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From 89ceb0d45d7fb30885f793a19cfae7ee26dae3fc Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +
.../Analysis/Analyses/ThreadSafetyCommon.h| 17 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 3 +
clang/lib/Analysis/ThreadSafety.cpp | 127 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 39 +--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 ++
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 308 ++
clang/unittests/AST/ASTImporterTest.cpp | 7 +
14 files changed, 509 insertions(+), 51 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1f24fb23d44d..7afa2ef359de9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -343,6 +343,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..4fc7ff28e
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From d3324c1023533bf784a3c3c3ef095d07c865e6f9 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From 1a2594caf99b5693e5e804b8c25c7a52b35f28b4 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +
.../Analysis/Analyses/ThreadSafetyCommon.h| 17 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 3 +
clang/lib/Analysis/ThreadSafety.cpp | 127 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 39 +--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 ++
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 308 ++
clang/unittests/AST/ASTImporterTest.cpp | 7 +
14 files changed, 509 insertions(+), 51 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1f24fb23d44d..7afa2ef359de9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -343,6 +343,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..4fc7ff28e
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
melver wrote: > I think the biggest issue is that removing `const` from `FactEntry` does not > work. You'll have to undo all those changes and instead create a new > `FactEntry` for every lock/unlock. Good catch, reworked this. PTAL. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -434,6 +434,16 @@ class can be used as a capability. The string argument specifies the kind of capability in error messages, e.g. ``"mutex"``. See the ``Container`` example given above, or the ``Mutex`` class in :ref:`mutexheader`. +REENTRANT +- + +``REENTRANT`` is an attribute on capability classes, denoting that they are +reentrant. Marking a capability as reentrant means that acquiring the same +capability multiple times is safe. + +Note that acquiring the same capability with different access privileges +(exclusive vs. shared) again is not considered reentrant by the analysis. melver wrote: Added a Note. PTAL. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From d3324c1023533bf784a3c3c3ef095d07c865e6f9 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From cadff841b5ddc812e1ae44bcd4f87960173ef7c8 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 18 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +
.../Analysis/Analyses/ThreadSafetyCommon.h| 17 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 3 +
clang/lib/Analysis/ThreadSafety.cpp | 127 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 39 +--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 ++
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 308 ++
clang/unittests/AST/ASTImporterTest.cpp | 7 +
14 files changed, 509 insertions(+), 51 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1f24fb23d44d..7afa2ef359de9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -343,6 +343,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..4fc7ff28e
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From d3324c1023533bf784a3c3c3ef095d07c865e6f9 Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../clang/Analysis/Analyses/ThreadSafetyCommon.h | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..6e46a2d721463 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,28 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
template CapabilityExpr(const til::SExpr *, T, bool) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
}
bool equals(const CapabilityExpr &other) const {
>From 1e01f552b4f3e53d1f42a7af7962e15a1fa46ecd Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Thu, 24 Apr 2025 09:02:08 +0200
Subject: [PATCH 2/2] Thread Safety Analysis: Support reentrant capabilities
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
---
clang/docs/ReleaseNotes.rst | 1 +
clang/docs/ThreadSafetyAnalysis.rst | 13 +
.../clang/Analysis/Analyses/ThreadSafety.h| 4 +
.../Analysis/Analyses/ThreadSafetyCommon.h| 17 +-
clang/include/clang/Basic/Attr.td | 7 +
.../clang/Basic/DiagnosticSemaKinds.td| 3 +
clang/lib/Analysis/ThreadSafety.cpp | 127 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 39 +--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 7 +
...a-attribute-supported-attributes-list.test | 1 +
clang/test/Sema/warn-thread-safety-analysis.c | 20 ++
.../test/SemaCXX/thread-safety-annotations.h | 1 +
.../SemaCXX/warn-thread-safety-analysis.cpp | 308 ++
clang/unittests/AST/ASTImporterTest.cpp | 7 +
14 files changed, 504 insertions(+), 51 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index d1f24fb23d44d..7afa2ef359de9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -343,6 +343,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..beecf09b9
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -3990,6 +3990,13 @@ def LocksExcluded : InheritableAttr {
let Documentation = [Undocumented];
}
+def ReentrantCapability : InheritableAttr {
+ let Spellings = [Clang<"reentrant_capability">];
+ let Subjects = SubjectList<[Record, TypedefName]>;
+ let Documentation = [Undocumented];
melver wrote:
Ack. Yes, style follows the rest of these attributes.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -1831,15 +1852,15 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
assert(!Self);
const auto *TagT = Exp->getType()->getAs();
if (D->hasAttrs() && TagT && Exp->isPRValue()) {
- std::pair Placeholder =
- Analyzer->SxBuilder.createThisPlaceholder(Exp);
+ auto Placeholder = Analyzer->SxBuilder.createThisPlaceholder(Exp);
melver wrote:
Growing this pair/tuple become a little too complex, so I just made
createThisPlaceholder return a CapabilityExpr struct right away.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
melver wrote:
I don't follow. We need a way to query both, so we need at least 2 bits to
store that info.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -163,15 +184,15 @@ using FactID = unsigned short;
/// the analysis of a single routine.
class FactManager {
private:
- std::vector> Facts;
+ std::vector> Facts;
melver wrote:
Fixed.
I guess we have to do clone-mutate-replace of the FactEntries - a little less
efficient, but given these structs are relatively small (+ vtables) shouldn't
be too bad.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/aaronpuchert edited https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
+
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
+ CapabilityExpr(const til::SExpr *E, StringRef Kind, unsigned Flags)
aaronpuchert wrote:
I'd rather have two `bool` parameters and don't expose the data layout to the
user.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -103,6 +103,23 @@ static StringRef ClassifyDiagnostic(QualType VDT) {
return "mutex";
}
+static unsigned getCapabilityExprFlags(QualType VDT) {
+ unsigned Flags = 0;
+
+ if (const auto *RT = VDT->getAs()) {
+if (const auto *RD = RT->getDecl())
+ if (RD->hasAttr())
+Flags |= CapabilityExpr::FlagReentrant;
+ } else if (const auto *TT = VDT->getAs()) {
+if (const auto *TD = TT->getDecl())
+ if (TD->hasAttr())
+Flags |= CapabilityExpr::FlagReentrant;
+ } else if (VDT->isPointerOrReferenceType())
+return getCapabilityExprFlags(VDT->getPointeeType());
+
+ return Flags;
+}
aaronpuchert wrote:
I suppose this could be merged with `ClassifyDiagnostic` as we're doing the
same cases, but maybe I'm missing something?
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
aaronpuchert wrote:
I wonder if there is a use case for having both. If the capability can be
acquired an arbitrary number of times, does it make sense to require it not
being held? At least the negative capability should not be required when
acquiring a reentrant mutex.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -6708,15 +6708,15 @@ int testAdoptShared() {
} // namespace ReturnScopedLockable
-#endif
+#endif // __cpp_guaranteed_copy_elision
aaronpuchert wrote:
These changes are fine, but please just commit them separately. (No review
required.)
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -163,15 +184,15 @@ using FactID = unsigned short;
/// the analysis of a single routine.
class FactManager {
private:
- std::vector> Facts;
+ std::vector> Facts;
aaronpuchert wrote:
This does not work, `FactEntry` has to remain `const`. See Delesley's comment
in https://reviews.llvm.org/D51187:
> It's been a while since I last looked at this code, but I don't think you can
> use mutable fields in a FactEntry. The analysis creates a FactSet for each
> program point, but each FactSet simply has pointers (FactIDs) for the
> underlying FactEntries. If you change the definition of a FactEntry, it will
> change that definition for every program point.
I think the same applies here. You need to create new facts. (As a follow-up to
my mistake, I added this `const`. Perhaps I should add a comment that it's
there for a reason.)
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -434,6 +434,16 @@ class can be used as a capability. The string argument specifies the kind of capability in error messages, e.g. ``"mutex"``. See the ``Container`` example given above, or the ``Mutex`` class in :ref:`mutexheader`. +REENTRANT aaronpuchert wrote: I would suggest `REENTRANT_CAPABILITY` like the attribute because otherwise this could have a different meaning. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -434,6 +434,16 @@ class can be used as a capability. The string argument specifies the kind of capability in error messages, e.g. ``"mutex"``. See the ``Container`` example given above, or the ``Mutex`` class in :ref:`mutexheader`. +REENTRANT +- + +``REENTRANT`` is an attribute on capability classes, denoting that they are +reentrant. Marking a capability as reentrant means that acquiring the same +capability multiple times is safe. + +Note that acquiring the same capability with different access privileges +(exclusive vs. shared) again is not considered reentrant by the analysis. aaronpuchert wrote: Perhaps we could add a sentence that this is usually not needed, because the analysis will only complain about double locking that's provably unnecessary. It's only in cases where locking is happening through abstractions that double locking might be hard to prevent. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -1831,15 +1852,15 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
assert(!Self);
const auto *TagT = Exp->getType()->getAs();
if (D->hasAttrs() && TagT && Exp->isPRValue()) {
- std::pair Placeholder =
- Analyzer->SxBuilder.createThisPlaceholder(Exp);
+ auto Placeholder = Analyzer->SxBuilder.createThisPlaceholder(Exp);
aaronpuchert wrote:
I'd like to keep an explicit type here for readability. As an alternative, use
the typed `std::get`, i.e. `std::get`.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/aaronpuchert commented: I think the biggest issue is that removing `const` from `FactEntry` does not work. You'll have to undo all those changes and instead create a new `FactEntry` for every lock/unlock. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -139,22 +141,41 @@ class FactEntry : public CapabilityExpr {
bool declared() const { return Source == Declared; }
bool managed() const { return Source == Managed; }
- virtual void
- handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
-SourceLocation JoinLoc, LockErrorKind LEK,
-ThreadSafetyHandler &Handler) const = 0;
+ virtual void handleRemovalFromIntersection(FactSet &FSet,
+ FactManager &FactMan,
+ SourceLocation JoinLoc,
+ LockErrorKind LEK,
+ ThreadSafetyHandler &Handler) = 0;
virtual void handleLock(FactSet &FSet, FactManager &FactMan,
const FactEntry &entry,
- ThreadSafetyHandler &Handler) const = 0;
+ ThreadSafetyHandler &Handler) = 0;
virtual void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
-bool FullyRemove,
-ThreadSafetyHandler &Handler) const = 0;
+bool FullyRemove, ThreadSafetyHandler &Handler) =
0;
// Return true if LKind >= LK, where exclusive > shared
bool isAtLeast(LockKind LK) const {
return (LKind == LK_Exclusive) || (LK == LK_Shared);
}
+
+ // Return true if we can acquire a capability reentrant.
+ [[nodiscard]] bool tryReenter(LockKind ReenterKind) {
+if (!reentrant())
+ return false;
+if (kind() != ReenterKind)
+ return false;
+if (++ReentrancyCount == 0)
+ llvm::report_fatal_error("Maximum reentrancy reached");
AaronBallman wrote:
This should use a real diagnostic rather than reporting a fatal error, though I
expect no user will ever run into the diagnostic in practice.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
@@ -3990,6 +3990,13 @@ def LocksExcluded : InheritableAttr {
let Documentation = [Undocumented];
}
+def ReentrantCapability : InheritableAttr {
+ let Spellings = [Clang<"reentrant_capability">];
+ let Subjects = SubjectList<[Record, TypedefName]>;
+ let Documentation = [Undocumented];
AaronBallman wrote:
We generally don't want new, undocumented attributes. However, I think this is
fine because we have dedicated documentation for this elsewhere which is being
updated, and this matches the same lack of documentation as the rest of the
thread safety attributes.
https://github.com/llvm/llvm-project/pull/137133
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From c60ccbc31de8e81e6a4af915a83b8271f58f8e7e Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../Analysis/Analyses/ThreadSafetyCommon.h| 20 +++
clang/lib/Analysis/ThreadSafety.cpp | 4 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 14 ++---
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..f328d4c7f481a 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,30 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
+ CapabilityExpr(const til::SExpr *E, StringRef Kind, unsigned Flags)
+ : CapExpr(E, Flags), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
- template CapabilityExpr(const til::SExpr *, T, bool) = delete;
+ template
+ CapabilityExpr(const til::SExpr *, T, unsigned) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind,
+ CapExpr.getInt() ^ FlagNegative);
}
bool equals(const CapabilityExpr &other) const {
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index 42fb0fe7dcdaa..96e79bc4dcfcc 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1839,7 +1839,7 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
if (isa(Exp))
Self = Placeholder.first;
if (TagT->getDecl()->hasAttr())
-Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
+Scp = CapabilityExpr(Placeholder.first, Placeholder.second, 0);
}
assert(Loc.isInvalid());
@@ -1982,7 +1982,7 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
Cp.isInvalid() && CBTE) {
if (auto Object =
Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
Object != Analyzer->ConstructedObjects.end())
- Cp = CapabilityExpr(Object->second, StringRef("mutex"), false);
+ Cp = CapabilityExpr(Object->second, StringRef("mutex"), 0);
}
const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
if (!Fact) {
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp
b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index 13cd7e26dc16f..255e6413344da 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -173,7 +173,7 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr
*AttrExp,
Self,
ClassifyDiagnostic(
cast(D)->getFunctionObjectParameterType()),
- false);
+ 0);
else // For most attributes.
return translateAttrExpr(AttrExp, &Ctx);
}
@@ -197,22 +197,22 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr
*AttrExp,
// The "*" expr is a universal lock, which essentially turns off
// checks until it is removed from the lockset.
return CapabilityExpr(new (Arena) til::Wildcard(), StringRef("wildcard"),
-false);
+0);
else
// Ignore other string literals for now.
return CapabilityExpr();
}
- bool Neg = false;
+ unsigned ExprFlags = 0;
if (const auto *OE = dyn_cast(AttrExp)) {
if (OE->getOperator() == OO_Exclaim) {
- Neg = true;
+ ExprFlags |= CapabilityExpr::FlagNegative;
AttrExp = OE->getArg(0);
}
}
else if (const auto *UO = dyn_cast(AttrExp)) {
i
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver updated
https://github.com/llvm/llvm-project/pull/137133
>From c60ccbc31de8e81e6a4af915a83b8271f58f8e7e Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../Analysis/Analyses/ThreadSafetyCommon.h| 20 +++
clang/lib/Analysis/ThreadSafety.cpp | 4 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 14 ++---
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..f328d4c7f481a 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,30 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
+ CapabilityExpr(const til::SExpr *E, StringRef Kind, unsigned Flags)
+ : CapExpr(E, Flags), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
- template CapabilityExpr(const til::SExpr *, T, bool) = delete;
+ template
+ CapabilityExpr(const til::SExpr *, T, unsigned) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind,
+ CapExpr.getInt() ^ FlagNegative);
}
bool equals(const CapabilityExpr &other) const {
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index 42fb0fe7dcdaa..96e79bc4dcfcc 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1839,7 +1839,7 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
if (isa(Exp))
Self = Placeholder.first;
if (TagT->getDecl()->hasAttr())
-Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
+Scp = CapabilityExpr(Placeholder.first, Placeholder.second, 0);
}
assert(Loc.isInvalid());
@@ -1982,7 +1982,7 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
Cp.isInvalid() && CBTE) {
if (auto Object =
Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
Object != Analyzer->ConstructedObjects.end())
- Cp = CapabilityExpr(Object->second, StringRef("mutex"), false);
+ Cp = CapabilityExpr(Object->second, StringRef("mutex"), 0);
}
const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
if (!Fact) {
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp
b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index 13cd7e26dc16f..255e6413344da 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -173,7 +173,7 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr
*AttrExp,
Self,
ClassifyDiagnostic(
cast(D)->getFunctionObjectParameterType()),
- false);
+ 0);
else // For most attributes.
return translateAttrExpr(AttrExp, &Ctx);
}
@@ -197,22 +197,22 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr
*AttrExp,
// The "*" expr is a universal lock, which essentially turns off
// checks until it is removed from the lockset.
return CapabilityExpr(new (Arena) til::Wildcard(), StringRef("wildcard"),
-false);
+0);
else
// Ignore other string literals for now.
return CapabilityExpr();
}
- bool Neg = false;
+ unsigned ExprFlags = 0;
if (const auto *OE = dyn_cast(AttrExp)) {
if (OE->getOperator() == OO_Exclaim) {
- Neg = true;
+ ExprFlags |= CapabilityExpr::FlagNegative;
AttrExp = OE->getArg(0);
}
}
else if (const auto *UO = dyn_cast(AttrExp)) {
i
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
melver wrote: In https://lore.kernel.org/all/CANpmjNPquO=W1JAh1FNQb8pMQjgeZAKCPQUAd7qUg=5pjJ6x=q...@mail.gmail.com/ I wrote: > 1. Re-entrant acquires: rcu_read_lock(), preempt_disable(), etc. are > all re-entrant locks. My proposal is to introduce an attribute that > can be added to "ACQUIRE(..)" annotated functions to indicate they are > re-entrant. Release-count needs to then match acquire-count to fully > release a capability. But thinking about this more, this is unnecessarily complex and introduces other headaches in the analysis. The simpler option is to just declare the whole capability/lock as reentrant, as done in this PR. My conclusion is that the more complex version would be a case of YAGNI, and in the odd cases where it might need to be more fine-grained, marking the whole capability as reentrant will be traded off against potential false negatives. For user space locking primitives I've never seen this kind of mixed interface -- only in the kernel I could imaging this to be something we could cook up, but I'm happy to take the false negatives for having a simpler-to-use and saner feature in Clang. https://github.com/llvm/llvm-project/pull/137133 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
llvmbot wrote:
@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-clang
Author: Marco Elver (melver)
Changes
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
---
Patch is 39.45 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/137133.diff
11 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+1)
- (modified) clang/docs/ThreadSafetyAnalysis.rst (+10)
- (modified) clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h (+15-9)
- (modified) clang/include/clang/Basic/Attr.td (+7)
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+90-70)
- (modified) clang/lib/Analysis/ThreadSafetyCommon.cpp (+33-15)
- (modified) clang/test/Misc/pragma-attribute-supported-attributes-list.test
(+1)
- (modified) clang/test/Sema/warn-thread-safety-analysis.c (+20)
- (modified) clang/test/SemaCXX/thread-safety-annotations.h (+1)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+310-2)
- (modified) clang/unittests/AST/ASTImporterTest.cpp (+7)
``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index cf90218c562e2..6d2b3b288d506 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -340,6 +340,7 @@ Improvements to Clang's diagnostics
as function arguments or return value respectively. Note that
:doc:`ThreadSafetyAnalysis` still does not perform alias analysis. The
feature will be default-enabled with ``-Wthread-safety`` in a future release.
+- The :doc:`ThreadSafetyAnalysis` now supports reentrant capabilities.
- Clang will now do a better job producing common nested names, when producing
common types for ternary operator, template argument deduction and multiple
return auto deduction.
- The ``-Wsign-compare`` warning now treats expressions with bitwise not(~)
and minus(-) as signed integers
diff --git a/clang/docs/ThreadSafetyAnalysis.rst
b/clang/docs/ThreadSafetyAnalysis.rst
index 130069c5659d6..368826aebc689 100644
--- a/clang/docs/ThreadSafetyAnalysis.rst
+++ b/clang/docs/ThreadSafetyAnalysis.rst
@@ -434,6 +434,13 @@ class can be used as a capability. The string argument
specifies the kind of
capability in error messages, e.g. ``"mutex"``. See the ``Container`` example
given above, or the ``Mutex`` class in :ref:`mutexheader`.
+REENTRANT
+-
+
+``REENTRANT`` is an attribute on capability classes, denoting that they are
+reentrant. Marking a capability as reentrant means that acquiring the same
+capability multiple times is safe.
+
.. _scoped_capability:
SCOPED_CAPABILITY
@@ -846,6 +853,9 @@ implementation.
#define CAPABILITY(x) \
THREAD_ANNOTATION_ATTRIBUTE__(capability(x))
+ #define REENTRANT \
+THREAD_ANNOTATION_ATTRIBUTE__(reentrant_capability)
+
#define SCOPED_CAPABILITY \
THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..29d464f867367 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,32 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+ static constexpr unsigned FlagReentrant = 1u << 1;
+
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
+ CapabilityExpr(const til::SExpr *E, StringRef Kind, unsigned Flags)
+ : CapExpr(E, Flags), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
- template CapabilityExpr(const til::SExpr *, T, bool) = delete;
+ template
+ CapabilityExpr(const til::SExpr *, T, unsigned) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef ge
[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
https://github.com/melver created
https://github.com/llvm/llvm-project/pull/137133
Introduce the `reentrant_capability` attribute, which may be specified
alongside the `capability(..)` attribute to denote that the defined
capability type is reentrant. Marking a capability as reentrant means
that acquiring the same capability multiple times is safe, and does not
produce warnings on attempted re-acquisition.
The most significant changes required are plumbing to propagate if the
attribute is present to a CapabilityExpr, and then introducing a
ReentrancyCount to FactEntry that can be incremented while a fact
remains in the FactSet.
Care was taken to avoid increasing the size of both CapabilityExpr and
FactEntry by carefully allocating free bits of CapabilityExpr::CapExpr
and the bitset respectively.
>From c60ccbc31de8e81e6a4af915a83b8271f58f8e7e Mon Sep 17 00:00:00 2001
From: Marco Elver
Date: Wed, 23 Apr 2025 11:31:25 +0200
Subject: [PATCH 1/2] Thread Safety Analysis: Convert CapabilityExpr::CapExpr
to hold flags
Rather than holding a single bool, switch it to contain flags, which is
both more descriptive and simplifies adding more flags in subsequent
changes.
NFC.
---
.../Analysis/Analyses/ThreadSafetyCommon.h| 20 +++
clang/lib/Analysis/ThreadSafety.cpp | 4 ++--
clang/lib/Analysis/ThreadSafetyCommon.cpp | 14 ++---
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
index e99c5b2466334..f328d4c7f481a 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
@@ -271,26 +271,30 @@ class CFGWalker {
// translateAttrExpr needs it, but that should be moved too.
class CapabilityExpr {
private:
- /// The capability expression and whether it's negated.
- llvm::PointerIntPair CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair CapExpr;
/// The kind of capability as specified by @ref CapabilityAttr::getName.
StringRef CapKind;
public:
- CapabilityExpr() : CapExpr(nullptr, false) {}
- CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
- : CapExpr(E, Neg), CapKind(Kind) {}
+ static constexpr unsigned FlagNegative = 1u << 0;
+
+ CapabilityExpr() : CapExpr(nullptr, 0) {}
+ CapabilityExpr(const til::SExpr *E, StringRef Kind, unsigned Flags)
+ : CapExpr(E, Flags), CapKind(Kind) {}
// Don't allow implicitly-constructed StringRefs since we'll capture them.
- template CapabilityExpr(const til::SExpr *, T, bool) = delete;
+ template
+ CapabilityExpr(const til::SExpr *, T, unsigned) = delete;
const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
StringRef getKind() const { return CapKind; }
- bool negative() const { return CapExpr.getInt(); }
+ bool negative() const { return CapExpr.getInt() & FlagNegative; }
CapabilityExpr operator!() const {
-return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+return CapabilityExpr(CapExpr.getPointer(), CapKind,
+ CapExpr.getInt() ^ FlagNegative);
}
bool equals(const CapabilityExpr &other) const {
diff --git a/clang/lib/Analysis/ThreadSafety.cpp
b/clang/lib/Analysis/ThreadSafety.cpp
index 42fb0fe7dcdaa..96e79bc4dcfcc 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1839,7 +1839,7 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
if (isa(Exp))
Self = Placeholder.first;
if (TagT->getDecl()->hasAttr())
-Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
+Scp = CapabilityExpr(Placeholder.first, Placeholder.second, 0);
}
assert(Loc.isInvalid());
@@ -1982,7 +1982,7 @@ void BuildLockset::handleCall(const Expr *Exp, const
NamedDecl *D,
Cp.isInvalid() && CBTE) {
if (auto Object =
Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
Object != Analyzer->ConstructedObjects.end())
- Cp = CapabilityExpr(Object->second, StringRef("mutex"), false);
+ Cp = CapabilityExpr(Object->second, StringRef("mutex"), 0);
}
const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
if (!Fact) {
diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp
b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index 13cd7e26dc16f..255e6413344da 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -173,7 +173,7 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr
*AttrExp,
Self,
ClassifyDiagnostic(
cast(D)->getFunctionObjectParameterType()),
- false);
+ 0);
else // For most attributes.
return translateAttrExpr(AttrExp, &Ctx);
}
@@ -197,22 +197,22 @@ CapabilityExpr SExprBui
