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.

Author: melver

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

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)
 

clang/include/clang/Analysis/Analyses/ThreadSafety.h

@@ -278,6 +278,10 @@ class ThreadSafetyHandler {
   /// Warn that there is a cycle in acquired_before/after dependencies.
   virtual void handleBeforeAfterCycle(Name L1Name, SourceLocation Loc) {}
 
+  /// Warn that the maximum supported reentrancy depth has been reached.
+  virtual void handleReentrancyDepthLimit(StringRef Kind, Name LockName,
+                                          SourceLocation Loc) {}
+
   /// Called by the analysis when starting analysis of a function.
   /// Used to issue suggestions for changes to annotations.
   virtual void enterFunction(const FunctionDecl *FD) {}

clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h

@@ -272,27 +272,32 @@ class CFGWalker {
 class CapabilityExpr {
 private:
   static constexpr unsigned FlagNegative = 1u << 0;
+  static constexpr unsigned FlagReentrant = 1u << 1;
 
   /// The capability expression and flags.
-  llvm::PointerIntPair<const til::SExpr *, 1, unsigned> CapExpr;
+  llvm::PointerIntPair<const til::SExpr *, 2, unsigned> CapExpr;
 
   /// The kind of capability as specified by @ref CapabilityAttr::getName.
   StringRef CapKind;
 
 public:
   CapabilityExpr() : CapExpr(nullptr, 0) {}
-  CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg)
-      : CapExpr(E, Neg ? FlagNegative : 0), CapKind(Kind) {}
+  CapabilityExpr(const til::SExpr *E, StringRef Kind, bool Neg, bool Reentrant)
+      : CapExpr(E, (Neg ? FlagNegative : 0) | (Reentrant ? FlagReentrant : 0)),
+        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, bool, bool) = delete;
 
   const til::SExpr *sexpr() const { return CapExpr.getPointer(); }
   StringRef getKind() const { return CapKind; }
   bool negative() const { return CapExpr.getInt() & FlagNegative; }
+  bool reentrant() const { return CapExpr.getInt() & FlagReentrant; }
 
   CapabilityExpr operator!() const {
-    return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative());
+    return CapabilityExpr(CapExpr.getPointer(), CapKind, !negative(),
+                          reentrant());
   }
 
   bool equals(const CapabilityExpr &other) const {
@@ -390,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<til::LiteralPtr *, StringRef>
+  std::pair<til::LiteralPtr *, CapabilityExpr>
   createThisPlaceholder(const Expr *Exp);
 
   // Translate a clang statement or expression to a TIL expression.

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 {

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -4105,6 +4105,9 @@ def warn_expect_more_underlying_mutexes : Warning<
 def warn_expect_fewer_underlying_mutexes : Warning<
   "did not expect %0 '%2' to be managed by '%1'">,
   InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
+def warn_reentrancy_depth_limit : Warning<
+  "reentrancy depth limit reached for %0 '%1'">,
+  InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
 def note_managed_mismatch_here_for_param : Note<
   "see attribute on parameter here">;
 

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,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<FactEntry> clone() const = 0;
+
   virtual void
   handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
                                 SourceLocation JoinLoc, LockErrorKind LEK,
@@ -155,6 +161,29 @@ class FactEntry : public CapabilityExpr {
   bool isAtLeast(LockKind LK) const {
     return  (LKind == LK_Exclusive) || (LK == LK_Shared);
   }
+
+  // Return an updated FactEntry if we can acquire this capability reentrant,
+  // nullptr otherwise. Returns nullopt if reentrancy depth limit was reached.
+  [[nodiscard]] std::unique_ptr<FactEntry>
+  tryReenter(LockKind ReenterKind) const {
+    if (!reentrant())
+      return nullptr;
+    if (kind() != ReenterKind)
+      return nullptr;
+    auto NewFact = clone();
+    NewFact->ReentrancyDepth++;
+    return NewFact;
+  }
+
+  // Return an updated FactEntry if we are releasing a capability previously
+  // acquired reentrant, nullptr otherwise.
+  [[nodiscard]] std::unique_ptr<FactEntry> leaveReentrant() const {
+    if (!ReentrancyDepth)
+      return nullptr;
+    auto NewFact = clone();
+    NewFact->ReentrancyDepth--;
+    return NewFact;
+  }
 };
 
 using FactID = unsigned short;
@@ -168,6 +197,8 @@ class FactManager {
 public:
   FactID newFact(std::unique_ptr<FactEntry> Entry) {
     Facts.push_back(std::move(Entry));
+    assert(Facts.size() - 1 <= std::numeric_limits<unsigned short>::max() &&
+           "FactID space exhausted");
     return static_cast<unsigned short>(Facts.size() - 1);
   }
 
@@ -235,6 +266,20 @@ class FactSet {
     return false;
   }
 
+  std::optional<FactID> replaceLock(FactManager &FM, iterator It,
+                                    std::unique_ptr<FactEntry> Entry) {
+    if (It == end())
+      return std::nullopt;
+    FactID F = FM.newFact(std::move(Entry));
+    *It = F;
+    return F;
+  }
+
+  std::optional<FactID> replaceLock(FactManager &FM, const CapabilityExpr &CapE,
+                                    std::unique_ptr<FactEntry> Entry) {
+    return replaceLock(FM, findLockIter(FM, CapE), std::move(Entry));
+  }
+
   iterator findLockIter(FactManager &FM, const CapabilityExpr &CapE) {
     return std::find_if(begin(), end(), [&](FactID ID) {
       return FM[ID].matches(CapE);
@@ -864,6 +909,10 @@ class LockableFactEntry : public FactEntry {
                     SourceKind Src = Acquired)
       : FactEntry(Lockable, CE, LK, Loc, Src) {}
 
+  std::unique_ptr<FactEntry> clone() const override {
+    return std::make_unique<LockableFactEntry>(*this);
+  }
+
   void
   handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
                                 SourceLocation JoinLoc, LockErrorKind LEK,
@@ -876,14 +925,28 @@ class LockableFactEntry : public FactEntry {
 
   void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
                   ThreadSafetyHandler &Handler) const override {
-    Handler.handleDoubleLock(entry.getKind(), entry.toString(), loc(),
-                             entry.loc());
+    if (std::unique_ptr<FactEntry> ReentrantFact = tryReenter(entry.kind())) {
+      if (ReentrantFact->getReentrancyDepth() == 0)
+        Handler.handleReentrancyDepthLimit(entry.getKind(), entry.toString(),
+                                           entry.loc());
+      // This capability has been reentrantly acquired.
+      FSet.replaceLock(FactMan, entry, std::move(ReentrantFact));
+    } else {
+      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 {
+    if (std::unique_ptr<FactEntry> ReentrantFact = leaveReentrant()) {
+      // This capability remains reentrantly acquired.
+      FSet.replaceLock(FactMan, Cp, std::move(ReentrantFact));
+      return;
+    }
+
     FSet.removeLock(FactMan, Cp);
     if (!Cp.negative()) {
       FSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
@@ -916,6 +979,10 @@ class ScopedLockableFactEntry : public FactEntry {
                           SourceKind Src)
       : FactEntry(ScopedLockable, CE, LK_Exclusive, Loc, Src) {}
 
+  std::unique_ptr<FactEntry> clone() const override {
+    return std::make_unique<ScopedLockableFactEntry>(*this);
+  }
+
   CapExprSet getUnderlyingMutexes() const {
     CapExprSet UnderlyingMutexesSet;
     for (const UnderlyingCapability &UnderlyingMutex : UnderlyingMutexes)
@@ -996,10 +1063,16 @@ 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)
-        Handler->handleDoubleLock(Cp.getKind(), Cp.toString(), Fact->loc(),
-                                  loc);
+    if (const auto It = FSet.findLockIter(FactMan, Cp); It != FSet.end()) {
+      const FactEntry &Fact = FactMan[*It];
+      if (std::unique_ptr<FactEntry> ReentrantFact = Fact.tryReenter(kind)) {
+        if (ReentrantFact->getReentrancyDepth() == 0 && Handler)
+          Handler->handleReentrancyDepthLimit(Cp.getKind(), Cp.toString(), loc);
+        // This capability has been reentrantly acquired.
+        FSet.replaceLock(FactMan, It, std::move(ReentrantFact));
+      } else if (Handler) {
+        Handler->handleDoubleLock(Cp.getKind(), Cp.toString(), Fact.loc(), loc);
+      }
     } else {
       FSet.removeLock(FactMan, !Cp);
       FSet.addLock(FactMan,
@@ -1009,10 +1082,17 @@ class ScopedLockableFactEntry : public FactEntry {
 
   void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
               SourceLocation loc, ThreadSafetyHandler *Handler) const {
-    if (FSet.findLock(FactMan, Cp)) {
+    if (const auto It = FSet.findLockIter(FactMan, Cp); It != FSet.end()) {
+      const FactEntry &Fact = FactMan[*It];
+      if (std::unique_ptr<FactEntry> ReentrantFact = Fact.leaveReentrant()) {
+        // This capability remains reentrantly acquired.
+        FSet.replaceLock(FactMan, It, std::move(ReentrantFact));
+        return;
+      }
+
       FSet.removeLock(FactMan, Cp);
-      FSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
-                                !Cp, LK_Exclusive, loc));
+      FSet.addLock(FactMan,
+                   std::make_unique<LockableFactEntry>(!Cp, LK_Exclusive, loc));
     } else if (Handler) {
       SourceLocation PrevLoc;
       if (const FactEntry *Neg = FSet.findLock(FactMan, !Cp))
@@ -1306,7 +1386,6 @@ 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 (!Entry->asserted())
       Cp->handleLock(FSet, FactMan, *Entry, Handler);
@@ -1831,15 +1910,15 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
     assert(!Self);
     const auto *TagT = Exp->getType()->getAs<TagType>();
     if (D->hasAttrs() && TagT && Exp->isPRValue()) {
-      std::pair<til::LiteralPtr *, StringRef> Placeholder =
+      std::pair<til::LiteralPtr *, CapabilityExpr> Placeholder =
           Analyzer->SxBuilder.createThisPlaceholder(Exp);
       [[maybe_unused]] auto inserted =
           Analyzer->ConstructedObjects.insert({Exp, Placeholder.first});
       assert(inserted.second && "Are we visiting the same expression again?");
       if (isa<CXXConstructExpr>(Exp))
         Self = Placeholder.first;
       if (TagT->getDecl()->hasAttr<ScopedLockableAttr>())
-        Scp = CapabilityExpr(Placeholder.first, Placeholder.second, false);
+        Scp = Placeholder.second;
     }
 
     assert(Loc.isInvalid());
@@ -1977,12 +2056,13 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
       if (DeclaredLocks.empty())
         continue;
       CapabilityExpr Cp(Analyzer->SxBuilder.translate(Arg, nullptr),
-                        StringRef("mutex"), false);
+                        StringRef("mutex"), /*Neg=*/false, /*Reentrant=*/false);
       if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(Arg->IgnoreCasts());
           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"), /*Neg=*/false,
+                              /*Reentrant=*/false);
       }
       const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
       if (!Fact) {
@@ -2491,7 +2571,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
       }
       if (UnderlyingLocks.empty())
         continue;
-      CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(), false);
+      CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(),
+                        /*Neg=*/false, /*Reentrant=*/false);
       auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(
           Cp, Param->getLocation(), FactEntry::Declared);
       for (const CapabilityExpr &M : UnderlyingLocks)