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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
