Thread safety analysis: Replace flags in FactEntry by SourceKind [NFC]

[aaronpuchert]

Summary

The motivation here is to make it available in the base class whether a
fact is managed or not. That would have meant three flags on the base
class, so I had a look whether we really have 8 possible combinations.

It turns out we don't: asserted and declared are obviously mutually
exclusive. Managed facts are only created when we acquire a capability
through a scoped capability. Adopting an asserted or declared lock will
not (in fact can not, because Facts are immutable) make them managed.

We probably don't want to allow adopting an asserted lock (because then
the function should probably have a release attribute, and then the
assertion is pointless), but we might at some point decide to replace a
declared fact on adoption.

[aaron.ballman]

LGTM aside from a formatting nit. Thanks, this is more clear than a set of booleans.

clang/lib/Analysis/ThreadSafety.cpp

138

Might as well fix the clang-tidy diagnostic (we don't usually try to line up function qualifiers like that).

clang/lib/Analysis/ThreadSafety.cpp

 
 /// This is a helper class that stores a fact that is known at a
 /// particular point in program execution.  Currently, a fact is a capability,
 /// 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:
+  /// Where a fact comes from.
+  enum SourceKind {
+    Acquired, ///< The fact has been directly acquired.
+    Asserted, ///< The fact has been asserted to be held.
+    Declared, ///< The fact is assumed to be held by callers.
+    Managed,  ///< The fact has been acquired through a scoped capability.
+  };
+
 private:
   /// Exclusive or shared.
-  LockKind LKind;
+  LockKind LKind : 8;
+
+  // How it was acquired.
+  SourceKind Source : 8;
 
   /// Where it was acquired.
   SourceLocation AcquireLoc;
 
-  /// True if the lock was asserted.
-  bool Asserted;
-
-  /// True if the lock was declared.
-  bool Declared;
-
 public:
   FactEntry(const CapabilityExpr &CE, LockKind LK, SourceLocation Loc,
-            bool Asrt, bool Declrd = false)
-      : CapabilityExpr(CE), LKind(LK), AcquireLoc(Loc), Asserted(Asrt),
-        Declared(Declrd) {}
+            SourceKind Src)
+      : CapabilityExpr(CE), LKind(LK), Source(Src), AcquireLoc(Loc) {}
   virtual ~FactEntry() = default;
 
   LockKind kind() const { return LKind;      }
   SourceLocation loc() const { return AcquireLoc; }
-  bool asserted() const { return Asserted; }
-  bool declared() const { return Declared; }
 
-  void setDeclared(bool D) { Declared = D; }
+  bool asserted() const { return Source == Asserted; }
+  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 handleLock(FactSet &FSet, FactManager &FactMan,
                           const FactEntry &entry, ThreadSafetyHandler &Handler,
                           StringRef DiagKind) const = 0;
if (CurrBlockInfo->ExitLoc.isValid()) {
           BlockInfo[(*CurrBlock->pred_begin())->getBlockID()].ExitLoc;
     }
   }
 }
 
 namespace {
 
 class LockableFactEntry : public FactEntry {
-private:
-  /// managed by ScopedLockable object
-  bool Managed;
-
 public:
   LockableFactEntry(const CapabilityExpr &CE, LockKind LK, SourceLocation Loc,
-                    bool Mng = false, bool Asrt = false)
-      : FactEntry(CE, LK, Loc, Asrt), Managed(Mng) {}
+                    SourceKind Src = Acquired)
+      : FactEntry(CE, LK, Loc, Src) {}
 
   void
   handleRemovalFromIntersection(const FactSet &FSet, FactManager &FactMan,
                                 SourceLocation JoinLoc, LockErrorKind LEK,
                                 ThreadSafetyHandler &Handler) const override {
-    if (!Managed && !asserted() && !negative() && !isUniversal()) {
+    if (!managed() && !asserted() && !negative() && !isUniversal()) {
       Handler.handleMutexHeldEndOfScope("mutex", toString(), loc(), JoinLoc,
                                         LEK);
     }
   }
 
   void handleLock(FactSet &FSet, FactManager &FactMan, const FactEntry &entry,
                   ThreadSafetyHandler &Handler,
                   StringRef DiagKind) const override {
private:
 
   using UnderlyingCapability =
       llvm::PointerIntPair<const til::SExpr *, 2, UnderlyingCapabilityKind>;
 
   SmallVector<UnderlyingCapability, 4> UnderlyingMutexes;
 
 public:
   ScopedLockableFactEntry(const CapabilityExpr &CE, SourceLocation Loc)
-      : FactEntry(CE, LK_Exclusive, Loc, false) {}
+      : FactEntry(CE, LK_Exclusive, Loc, Acquired) {}
 
   void addLock(const CapabilityExpr &M) {
     UnderlyingMutexes.emplace_back(M.sexpr(), UCK_Acquired);
   }
 
   void addExclusiveUnlock(const CapabilityExpr &M) {
     UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }
void lock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
             LockKind kind, SourceLocation loc, ThreadSafetyHandler *Handler,
             StringRef DiagKind) const {
     if (const FactEntry *Fact = FSet.findLock(FactMan, Cp)) {
       if (Handler)
         Handler->handleDoubleLock(DiagKind, Cp.toString(), Fact->loc(), loc);
     } else {
       FSet.removeLock(FactMan, !Cp);
       FSet.addLock(FactMan,
-                   std::make_unique<LockableFactEntry>(Cp, kind, loc, true));
+                   std::make_unique<LockableFactEntry>(Cp, kind, loc, Managed));
     }
   }
 
   void unlock(FactSet &FSet, FactManager &FactMan, const CapabilityExpr &Cp,
               SourceLocation loc, ThreadSafetyHandler *Handler,
               StringRef DiagKind) const {
     if (FSet.findLock(FactMan, Cp)) {
       FSet.removeLock(FactMan, Cp);
switch (At->getKind()) {
       // a warning if it is already there, and will not generate a warning
       // if it is not removed.
       case attr::AssertExclusiveLock: {
         const auto *A = cast<AssertExclusiveLockAttr>(At);
 
         CapExprSet AssertLocks;
         Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
         for (const auto &AssertLock : AssertLocks)
-          Analyzer->addLock(FSet,
-                            std::make_unique<LockableFactEntry>(
-                                AssertLock, LK_Exclusive, Loc, false, true),
-                            ClassifyDiagnostic(A));
+          Analyzer->addLock(
+              FSet,
+              std::make_unique<LockableFactEntry>(AssertLock, LK_Exclusive, Loc,
+                                                  FactEntry::Asserted),
         break;
       }
       case attr::AssertSharedLock: {
         const auto *A = cast<AssertSharedLockAttr>(At);
 
         CapExprSet AssertLocks;
         Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
         for (const auto &AssertLock : AssertLocks)
-          Analyzer->addLock(FSet,
-                            std::make_unique<LockableFactEntry>(
-                                AssertLock, LK_Shared, Loc, false, true),
-                            ClassifyDiagnostic(A));
+          Analyzer->addLock(
+              FSet,
+              std::make_unique<LockableFactEntry>(AssertLock, LK_Shared, Loc,
+                                                  FactEntry::Asserted),
         break;
       }
 
       case attr::AssertCapability: {
         const auto *A = cast<AssertCapabilityAttr>(At);
         CapExprSet AssertLocks;
         Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(FSet,
                             std::make_unique<LockableFactEntry>(
                                 AssertLock,
                                 A->isShared() ? LK_Shared : LK_Exclusive, Loc,
-                                false, true),
+                                FactEntry::Asserted),
                             ClassifyDiagnostic(A));
         break;
       }
 
       // When we encounter an unlock function, we need to remove unlocked
       // mutexes from the lockset, and flag a warning if they are not there.
       case attr::ReleaseCapability: {
         const auto *A = cast<ReleaseCapabilityAttr>(At);
void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
   for (const auto &M : ExclusiveLocksToRemove)
     Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Exclusive, CapDiagKind);
   for (const auto &M : SharedLocksToRemove)
     Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Shared, CapDiagKind);
   for (const auto &M : GenericLocksToRemove)
     Analyzer->removeLock(FSet, M, Loc, Dtor, LK_Generic, CapDiagKind);
 
   // Add locks.
+  FactEntry::SourceKind Source =
+      isScopedVar ? FactEntry::Managed : FactEntry::Acquired;
   for (const auto &M : ExclusiveLocksToAdd)
-    Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
-                                M, LK_Exclusive, Loc, isScopedVar),
-                      CapDiagKind);
+    Analyzer->addLock(
+        FSet, std::make_unique<LockableFactEntry>(M, LK_Exclusive, Loc, Source),
   for (const auto &M : SharedLocksToAdd)
-    Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
-                                M, LK_Shared, Loc, isScopedVar),
-                      CapDiagKind);
+    Analyzer->addLock(
+        FSet, std::make_unique<LockableFactEntry>(M, LK_Shared, Loc, Source),
 
   if (isScopedVar) {
     // Add the managing object as a dummy mutex, mapped to the underlying mutex.
     SourceLocation MLoc = VD->getLocation();
     DeclRefExpr DRE(VD->getASTContext(), VD, false, VD->getType(), VK_LValue,
                     VD->getLocation());
     // FIXME: does this store a pointer to DRE?
     CapabilityExpr Scp = Analyzer->SxBuilder.translateAttrExpr(&DRE, nullptr);
for (const auto *Attr : D->attrs()) {
       } else if (isa<TryAcquireCapabilityAttr>(Attr)) {
         // Don't try to check trylock functions for now.
         return;
       }
     }
 
     // FIXME -- Loc can be wrong here.
     for (const auto &Mu : ExclusiveLocksToAdd) {
-      auto Entry = std::make_unique<LockableFactEntry>(Mu, LK_Exclusive, Loc);
-      Entry->setDeclared(true);
+      auto Entry = std::make_unique<LockableFactEntry>(Mu, LK_Exclusive, Loc,
+                                                       FactEntry::Declared);
       addLock(InitialLockset, std::move(Entry), CapDiagKind, true);
     }
     for (const auto &Mu : SharedLocksToAdd) {
-      auto Entry = std::make_unique<LockableFactEntry>(Mu, LK_Shared, Loc);
-      Entry->setDeclared(true);
+      auto Entry = std::make_unique<LockableFactEntry>(Mu, LK_Shared, Loc,
+                                                       FactEntry::Declared);
       addLock(InitialLockset, std::move(Entry), CapDiagKind, true);
     }
   }
 
   for (const auto *CurrBlock : *SortedGraph) {
     unsigned CurrBlockID = CurrBlock->getBlockID();
     CFGBlockInfo *CurrBlockInfo = &BlockInfo[CurrBlockID];