[clang] Thread Safety Analysis: Support reentrant capabilities (PR #137133)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 24 01:10:00 PDT 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
@llvm/pr-subscribers-clang
Author: Marco Elver (melver)
<details>
<summary>Changes</summary>
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<const til::SExpr *, 1, bool> CapExpr;
+ /// The capability expression and flags.
+ llvm::PointerIntPair<const til::SExpr *, 2, unsigned> 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 <typename T> CapabilityExpr(const til::SExpr *, T, bool) = delete;
+ template <typename T>
+ 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; }
+ bool reentrant() const { return CapExpr.getInt() & FlagReentrant; }
CapabilityExpr operator!() const {
- return CapabilityExpr(CapExpr.getPointer(), CapKind, !CapExpr.getInt());
+ return CapabilityExpr(CapExpr.getPointer(), CapKind,
+ CapExpr.getInt() ^ FlagNegative);
}
bool equals(const CapabilityExpr &other) const {
@@ -388,7 +394,7 @@ class SExprBuilder {
til::LiteralPtr *createVariable(const VarDecl *VD);
// Create placeholder for this: we don't know the VarDecl on construction yet.
- std::pair<til::LiteralPtr *, StringRef>
+ std::tuple<til::LiteralPtr *, StringRef, unsigned>
createThisPlaceholder(const Expr *Exp);
// Translate a clang statement or expression to a TIL expression.
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index d48aed5b73cf5..88be9d3d13629 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -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];
+ let SimpleHandler = 1;
+}
+
// C/C++ consumed attributes.
def Consumable : InheritableAttr {
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 42fb0fe7dcdaa..5b1883a0a4b15 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -99,8 +99,6 @@ class FactSet;
/// particular point in program execution. Currently, a fact is a capability,
/// along with additional information, such as where it was acquired, whether
/// it is exclusive or shared, etc.
-///
-/// FIXME: this analysis does not currently support re-entrant locking.
class FactEntry : public CapabilityExpr {
public:
enum FactEntryKind { Lockable, ScopedLockable };
@@ -114,21 +112,25 @@ 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 count.
+ unsigned int ReentrancyCount : 20;
/// 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),
+ ReentrancyCount(0), AcquireLoc(Loc) {}
virtual ~FactEntry() = default;
LockKind kind() const { return LKind; }
@@ -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");
+ return true;
+ }
+
+ // Return true if we are releasing a capability previously acquired reentrant.
+ [[nodiscard]] bool leaveReentrant() {
+ if (!ReentrancyCount)
+ return false;
+ ReentrancyCount--;
+ return true;
+ }
};
using FactID = unsigned short;
@@ -163,7 +184,7 @@ using FactID = unsigned short;
/// the analysis of a single routine.
class FactManager {
private:
- std::vector<std::unique_ptr<const FactEntry>> Facts;
+ std::vector<std::unique_ptr<FactEntry>> Facts;
public:
FactID newFact(std::unique_ptr<FactEntry> Entry) {
@@ -171,7 +192,7 @@ class FactManager {
return static_cast<unsigned short>(Facts.size() - 1);
}
- const FactEntry &operator[](FactID F) const { return *Facts[F]; }
+ FactEntry &operator[](FactID F) { return *Facts[F]; }
};
/// A FactSet is the set of facts that are known to be true at a
@@ -241,23 +262,21 @@ class FactSet {
});
}
- const FactEntry *findLock(FactManager &FM, const CapabilityExpr &CapE) const {
+ FactEntry *findLock(FactManager &FM, const CapabilityExpr &CapE) {
auto I = std::find_if(begin(), end(), [&](FactID ID) {
return FM[ID].matches(CapE);
});
return I != end() ? &FM[*I] : nullptr;
}
- const FactEntry *findLockUniv(FactManager &FM,
- const CapabilityExpr &CapE) const {
+ FactEntry *findLockUniv(FactManager &FM, const CapabilityExpr &CapE) {
auto I = std::find_if(begin(), end(), [&](FactID ID) -> bool {
return FM[ID].matchesUniv(CapE);
});
return I != end() ? &FM[*I] : nullptr;
}
- const FactEntry *findPartialMatch(FactManager &FM,
- const CapabilityExpr &CapE) const {
+ FactEntry *findPartialMatch(FactManager &FM, const CapabilityExpr &CapE) {
auto I = std::find_if(begin(), end(), [&](FactID ID) -> bool {
return FM[ID].partiallyMatches(CapE);
});
@@ -864,10 +883,9 @@ class LockableFactEntry : public FactEntry {
SourceKind Src = Acquired)
: FactEntry(Lockable, CE, LK, Loc, Src) {}
- void
- handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
- SourceLocation JoinLoc, LockErrorKind LEK,
- ThreadSafetyHandler &Handler) const override {
+ void handleRemovalFromIntersection(FactSet &FSet, FactManager &FactMan,
+ SourceLocation JoinLoc, LockErrorKind LEK,
+ ThreadSafetyHandler &Handler) override {
if (!asserted() && !negative() && !isUniversal()) {
Handler.handleMutexHeldEndOfScope(getKind(), toString(), loc(), JoinLoc,
LEK);
@@ -875,15 +893,18 @@ class LockableFactEntry : public FactEntry {
}
void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
- ThreadSafetyHandler &Handler) const override {
+ ThreadSafetyHandler &Handler) override {
+ if (tryReenter(entry.kind()))
+ return;
Handler.handleDoubleLock(entry.getKind(), entry.toString(), loc(),
entry.loc());
}
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
- bool FullyRemove,
- ThreadSafetyHandler &Handler) const override {
+ bool FullyRemove, ThreadSafetyHandler &Handler) override {
+ if (leaveReentrant())
+ return;
FSet.removeLock(FactMan, Cp);
if (!Cp.negative()) {
FSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
@@ -935,10 +956,9 @@ class ScopedLockableFactEntry : public FactEntry {
UnderlyingMutexes.push_back(UnderlyingCapability{M, UCK_ReleasedShared});
}
- void
- handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
- SourceLocation JoinLoc, LockErrorKind LEK,
- ThreadSafetyHandler &Handler) const override {
+ void handleRemovalFromIntersection(FactSet &FSet, FactManager &FactMan,
+ SourceLocation JoinLoc, LockErrorKind LEK,
+ ThreadSafetyHandler &Handler) override {
if (LEK == LEK_LockedAtEndOfFunction || LEK == LEK_NotLockedAtEndOfFunction)
return;
@@ -956,7 +976,7 @@ class ScopedLockableFactEntry : public FactEntry {
}
void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
- ThreadSafetyHandler &Handler) const override {
+ ThreadSafetyHandler &Handler) override {
for (const auto &UnderlyingMutex : UnderlyingMutexes) {
if (UnderlyingMutex.Kind == UCK_Acquired)
lock(FSet, FactMan, UnderlyingMutex.Cap, entry.kind(), entry.loc(),
@@ -968,8 +988,7 @@ class ScopedLockableFactEntry : public FactEntry {
void handleUnlock(FactSet &FSet, FactManager &FactMan,
const CapabilityExpr &Cp, SourceLocation UnlockLoc,
- bool FullyRemove,
- ThreadSafetyHandler &Handler) const override {
+ bool FullyRemove, ThreadSafetyHandler &Handler) override {
assert(!Cp.negative() && "Managing object cannot be negative.");
for (const auto &UnderlyingMutex : UnderlyingMutexes) {
// Remove/lock the underlying mutex if it exists/is still unlocked; warn
@@ -996,8 +1015,8 @@ class ScopedLockableFactEntry : public FactEntry {
void lock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
LockKind kind, SourceLocation loc,
ThreadSafetyHandler *Handler) const {
- if (const FactEntry *Fact = FSet.findLock(FactMan, Cp)) {
- if (Handler)
+ if (FactEntry *Fact = FSet.findLock(FactMan, Cp)) {
+ if (!Fact->tryReenter(kind) && Handler)
Handler->handleDoubleLock(Cp.getKind(), Cp.toString(), Fact->loc(),
loc);
} else {
@@ -1009,7 +1028,9 @@ class ScopedLockableFactEntry : public FactEntry {
void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
SourceLocation loc, ThreadSafetyHandler *Handler) const {
- if (FSet.findLock(FactMan, Cp)) {
+ if (FactEntry *Fact = FSet.findLock(FactMan, Cp)) {
+ if (Fact->leaveReentrant())
+ return;
FSet.removeLock(FactMan, Cp);
FSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
!Cp, LK_Exclusive, loc));
@@ -1071,28 +1092,28 @@ class ThreadSafetyAnalyzer {
bool join(const FactEntry &a, const FactEntry &b, bool CanModify);
- void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet,
+ void intersectAndWarn(FactSet &EntrySet, FactSet &ExitSet,
SourceLocation JoinLoc, LockErrorKind EntryLEK,
LockErrorKind ExitLEK);
- void intersectAndWarn(FactSet &EntrySet, const FactSet &ExitSet,
+ void intersectAndWarn(FactSet &EntrySet, FactSet &ExitSet,
SourceLocation JoinLoc, LockErrorKind LEK) {
intersectAndWarn(EntrySet, ExitSet, JoinLoc, LEK, LEK);
}
void runAnalysis(AnalysisDeclContext &AC);
- void warnIfMutexNotHeld(const FactSet &FSet, const NamedDecl *D,
- const Expr *Exp, AccessKind AK, Expr *MutexExp,
+ void warnIfMutexNotHeld(FactSet &FSet, const NamedDecl *D, const Expr *Exp,
+ AccessKind AK, Expr *MutexExp,
ProtectedOperationKind POK, til::LiteralPtr *Self,
SourceLocation Loc);
- void warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp,
+ void warnIfMutexHeld(FactSet &FSet, const NamedDecl *D, const Expr *Exp,
Expr *MutexExp, til::LiteralPtr *Self,
SourceLocation Loc);
- void checkAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK,
+ void checkAccess(FactSet &FSet, const Expr *Exp, AccessKind AK,
ProtectedOperationKind POK);
- void checkPtAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK,
+ void checkPtAccess(FactSet &FSet, const Expr *Exp, AccessKind AK,
ProtectedOperationKind POK);
};
@@ -1306,8 +1327,7 @@ void ThreadSafetyAnalyzer::addLock(FactSet &FSet,
Entry->loc(), Entry->getKind());
}
- // FIXME: Don't always warn when we have support for reentrant locks.
- if (const FactEntry *Cp = FSet.findLock(FactMan, *Entry)) {
+ if (FactEntry *Cp = FSet.findLock(FactMan, *Entry)) {
if (!Entry->asserted())
Cp->handleLock(FSet, FactMan, *Entry, Handler);
} else {
@@ -1323,7 +1343,7 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,
if (Cp.shouldIgnore())
return;
- const FactEntry *LDat = FSet.findLock(FactMan, Cp);
+ FactEntry *LDat = FSet.findLock(FactMan, Cp);
if (!LDat) {
SourceLocation PrevLoc;
if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
@@ -1546,7 +1566,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
ThreadSafetyAnalyzer *Analyzer;
FactSet FSet;
// The fact set for the function on exit.
- const FactSet &FunctionExitFSet;
+ FactSet &FunctionExitFSet;
LocalVariableMap::Context LVarCtx;
unsigned CtxIndex;
@@ -1571,7 +1591,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
public:
BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
- const FactSet &FunctionExitFSet)
+ FactSet &FunctionExitFSet)
: ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
CtxIndex(Info.EntryIndex) {}
@@ -1590,10 +1610,12 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
/// Warn if the LSet does not contain a lock sufficient to protect access
/// of at least the passed in AccessKind.
-void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
- const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK,
- Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self,
- SourceLocation Loc) {
+void ThreadSafetyAnalyzer::warnIfMutexNotHeld(FactSet &FSet, const NamedDecl *D,
+ const Expr *Exp, AccessKind AK,
+ Expr *MutexExp,
+ ProtectedOperationKind POK,
+ til::LiteralPtr *Self,
+ SourceLocation Loc) {
LockKind LK = getLockKindFromAccessKind(AK);
CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
if (Cp.isInvalid()) {
@@ -1649,9 +1671,8 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
}
/// Warn if the LSet contains the given lock.
-void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet,
- const NamedDecl *D, const Expr *Exp,
- Expr *MutexExp,
+void ThreadSafetyAnalyzer::warnIfMutexHeld(FactSet &FSet, const NamedDecl *D,
+ const Expr *Exp, Expr *MutexExp,
til::LiteralPtr *Self,
SourceLocation Loc) {
CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
@@ -1674,7 +1695,7 @@ void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet,
/// marked with guarded_by, we must ensure the appropriate mutexes are held.
/// Similarly, we check if the access is to an expression that dereferences
/// a pointer marked with pt_guarded_by.
-void ThreadSafetyAnalyzer::checkAccess(const FactSet &FSet, const Expr *Exp,
+void ThreadSafetyAnalyzer::checkAccess(FactSet &FSet, const Expr *Exp,
AccessKind AK,
ProtectedOperationKind POK) {
Exp = Exp->IgnoreImplicit()->IgnoreParenCasts();
@@ -1741,7 +1762...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/137133
More information about the cfe-commits
mailing list