Thread safety analysis: Support copy-elided production of scoped capabilities through arbitrary calls

When support for copy elision was initially added in e97654b2f2807, it
was taking attributes from a constructor call, although that constructor
call is actually not involved. It seems more natural to use attributes
on the function returning the scoped capability, which is where it's
actually coming from. This would also support a number of interesting
use cases, like producing different scope kinds without the need for tag
types, or producing scopes from a private mutex.

Changing the behavior was surprisingly difficult: we were not handling
CXXConstructorExpr calls like regular calls but instead handled them
through the DeclStmt they're contained in. This was based on the
assumption that constructors are basically only called in variable
declarations (not true because of temporaries), and that variable
declarations necessitate constructors (not true with C++17 anymore).

Untangling this required separating construction from assigning a
variable name. When a call produces an object, we use a placeholder
til::LiteralPtr for `this`, and we collect the call expression and
placeholder in a map. Later when going through a DeclStmt, we look up
the call expression and set the placeholder to the new VarDecl.

The change has a couple of nice side effects:
* We don't miss constructor calls not contained in DeclStmts anymore,
  allowing patterns like
    MutexLock{&mu}, requiresMutex();
  The scoped lock temporary will be destructed at the end of the full
  statement, so it protects the following call without the need for a
  scope, but with the ability to unlock in case of an exception.
* We support lifetime extension of temporaries. While unusual, one can
  now write
    const MutexLock &scope = MutexLock(&mu);
  and have it behave as expected.
* Destructors used to be handled in a weird way: since there is no
  expression in the AST for implicit destructor calls, we instead
  provided a made-up DeclRefExpr to the variable being destructed, and
  passed that instead of a CallExpr. Then later in translateAttrExpr
  there was special code that knew that destructor expressions worked a
  bit different.
* We were producing dummy DeclRefExprs in a number of places, this has
  been eliminated. We now use til::SExprs instead.

Technically this could break existing code, but the current handling
seems unexpected enough to justify this change.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D129755

1 files changed, 104 insertions, 98 deletions

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index a29134c6..7674756 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1029,7 +1029,7 @@ public:
 
   template <typename AttrType>
   void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
-                   const NamedDecl *D, VarDecl *SelfDecl = nullptr);
+                   const NamedDecl *D, til::SExpr *Self = nullptr);
 
   template <class AttrType>
   void getMutexIDs(CapExprSet &Mtxs, AttrType *Attr, const Expr *Exp,
@@ -1220,7 +1220,7 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
   if (const auto *LP = dyn_cast<til::LiteralPtr>(SExp)) {
     const ValueDecl *VD = LP->clangDecl();
     // Variables defined in a function are always inaccessible.
-    if (!VD->isDefinedOutsideFunctionOrMethod())
+    if (!VD || !VD->isDefinedOutsideFunctionOrMethod())
       return false;
     // For now we consider static class members to be inaccessible.
     if (isa<CXXRecordDecl>(VD->getDeclContext()))
@@ -1311,10 +1311,10 @@ void ThreadSafetyAnalyzer::removeLock(FactSet &FSet, const CapabilityExpr &Cp,
 template <typename AttrType>
 void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
                                        const Expr *Exp, const NamedDecl *D,
-                                       VarDecl *SelfDecl) {
+                                       til::SExpr *Self) {
   if (Attr->args_size() == 0) {
     // The mutex held is the "this" object.
-    CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, SelfDecl);
+    CapabilityExpr Cp = SxBuilder.translateAttrExpr(nullptr, D, Exp, Self);
     if (Cp.isInvalid()) {
       warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind());
       return;
@@ -1326,7 +1326,7 @@ void ThreadSafetyAnalyzer::getMutexIDs(CapExprSet &Mtxs, AttrType *Attr,
   }
 
   for (const auto *Arg : Attr->args()) {
-    CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, SelfDecl);
+    CapabilityExpr Cp = SxBuilder.translateAttrExpr(Arg, D, Exp, Self);
     if (Cp.isInvalid()) {
       warnInvalidLock(Handler, nullptr, D, Exp, Cp.getKind());
       continue;
@@ -1529,21 +1529,26 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  /// Maps constructed objects to `this` placeholder prior to initialization.
+  llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
   // helper functions
   void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
                           Expr *MutexExp, ProtectedOperationKind POK,
-                          SourceLocation Loc);
-  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp);
+                          til::LiteralPtr *Self, SourceLocation Loc);
+  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
+                       til::LiteralPtr *Self, SourceLocation Loc);
 
   void checkAccess(const Expr *Exp, AccessKind AK,
                    ProtectedOperationKind POK = POK_VarAccess);
   void checkPtAccess(const Expr *Exp, AccessKind AK,
                      ProtectedOperationKind POK = POK_VarAccess);
 
-  void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr);
+  void handleCall(const Expr *Exp, const NamedDecl *D,
+                  til::LiteralPtr *Self = nullptr,
+                  SourceLocation Loc = SourceLocation());
   void examineArguments(const FunctionDecl *FD,
                         CallExpr::const_arg_iterator ArgBegin,
                         CallExpr::const_arg_iterator ArgEnd,
@@ -1560,6 +1565,7 @@ public:
   void VisitCallExpr(const CallExpr *Exp);
   void VisitCXXConstructExpr(const CXXConstructExpr *Exp);
   void VisitDeclStmt(const DeclStmt *S);
+  void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp);
 };
 
 } // namespace
@@ -1569,10 +1575,12 @@ public:
 void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
                                       AccessKind AK, Expr *MutexExp,
                                       ProtectedOperationKind POK,
+                                      til::LiteralPtr *Self,
                                       SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
 
-  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
+  CapabilityExpr Cp =
+      Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
     warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
     return;
@@ -1629,8 +1637,10 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
 
 /// Warn if the LSet contains the given lock.
 void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
-                                   Expr *MutexExp) {
-  CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp);
+                                   Expr *MutexExp, til::LiteralPtr *Self,
+                                   SourceLocation Loc) {
+  CapabilityExpr Cp =
+      Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
     warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
     return;
@@ -1641,7 +1651,7 @@ void BuildLockset::warnIfMutexHeld(const NamedDecl *D, const Expr *Exp,
   const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, Cp);
   if (LDat) {
     Analyzer->Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
-                                            Cp.toString(), Exp->getExprLoc());
+                                            Cp.toString(), Loc);
   }
 }
 
@@ -1711,7 +1721,7 @@ void BuildLockset::checkAccess(const Expr *Exp, AccessKind AK,
   }
 
   for (const auto *I : D->specific_attrs<GuardedByAttr>())
-    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, Loc);
+    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), POK, nullptr, Loc);
 }
 
 /// Checks pt_guarded_by and pt_guarded_var attributes.
@@ -1748,7 +1758,8 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK,
     Analyzer->Handler.handleNoMutexHeld(D, PtPOK, AK, Exp->getExprLoc());
 
   for (auto const *I : D->specific_attrs<PtGuardedByAttr>())
-    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, Exp->getExprLoc());
+    warnIfMutexNotHeld(D, Exp, AK, I->getArg(), PtPOK, nullptr,
+                       Exp->getExprLoc());
 }
 
 /// Process a function call, method call, constructor call,
@@ -1761,21 +1772,35 @@ void BuildLockset::checkPtAccess(const Expr *Exp, AccessKind AK,
 /// and check that the appropriate locks are held. Non-const method calls with
 /// the same signature as const method calls can be also treated as reads.
 ///
+/// \param Exp   The call expression.
+/// \param D     The callee declaration.
+/// \param Self  If \p Exp = nullptr, the implicit this argument.
+/// \param Loc   If \p Exp = nullptr, the location.
 void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
-                              VarDecl *VD) {
-  SourceLocation Loc = Exp->getExprLoc();
+                              til::LiteralPtr *Self, SourceLocation Loc) {
   CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
   CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
   CapExprSet ScopedReqsAndExcludes;
 
   // Figure out if we're constructing an object of scoped lockable class
-  bool isScopedVar = false;
-  if (VD) {
-    if (const auto *CD = dyn_cast<const CXXConstructorDecl>(D)) {
-      const CXXRecordDecl* PD = CD->getParent();
-      if (PD && PD->hasAttr<ScopedLockableAttr>())
-        isScopedVar = true;
+  CapabilityExpr Scp;
+  if (Exp) {
+    assert(!Self);
+    const auto *TagT = Exp->getType()->getAs<TagType>();
+    if (TagT && Exp->isPRValue()) {
+      std::pair<til::LiteralPtr *, StringRef> Placeholder =
+          Analyzer->SxBuilder.createThisPlaceholder(Exp);
+      [[maybe_unused]] auto inserted =
+          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);
     }
+
+    assert(Loc.isInvalid());
+    Loc = Exp->getExprLoc();
   }
 
   for(const Attr *At : D->attrs()) {
@@ -1786,7 +1811,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
         const auto *A = cast<AcquireCapabilityAttr>(At);
         Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
                                             : ExclusiveLocksToAdd,
-                              A, Exp, D, VD);
+                              A, Exp, D, Self);
         break;
       }
 
@@ -1797,7 +1822,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
         const auto *A = cast<AssertExclusiveLockAttr>(At);
 
         CapExprSet AssertLocks;
-        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(
               FSet, std::make_unique<LockableFactEntry>(
@@ -1808,7 +1833,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
         const auto *A = cast<AssertSharedLockAttr>(At);
 
         CapExprSet AssertLocks;
-        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(
               FSet, std::make_unique<LockableFactEntry>(
@@ -1819,7 +1844,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
       case attr::AssertCapability: {
         const auto *A = cast<AssertCapabilityAttr>(At);
         CapExprSet AssertLocks;
-        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, VD);
+        Analyzer->getMutexIDs(AssertLocks, A, Exp, D, Self);
         for (const auto &AssertLock : AssertLocks)
           Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(
                                       AssertLock,
@@ -1833,11 +1858,11 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
       case attr::ReleaseCapability: {
         const auto *A = cast<ReleaseCapabilityAttr>(At);
         if (A->isGeneric())
-          Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, VD);
+          Analyzer->getMutexIDs(GenericLocksToRemove, A, Exp, D, Self);
         else if (A->isShared())
-          Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, VD);
+          Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
         else
-          Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, VD);
+          Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
         break;
       }
 
@@ -1845,10 +1870,10 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
         const auto *A = cast<RequiresCapabilityAttr>(At);
         for (auto *Arg : A->args()) {
           warnIfMutexNotHeld(D, Exp, A->isShared() ? AK_Read : AK_Written, Arg,
-                             POK_FunctionCall, Exp->getExprLoc());
+                             POK_FunctionCall, Self, Loc);
           // use for adopting a lock
-          if (isScopedVar)
-            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD);
+          if (!Scp.shouldIgnore())
+            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self);
         }
         break;
       }
@@ -1856,10 +1881,10 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
       case attr::LocksExcluded: {
         const auto *A = cast<LocksExcludedAttr>(At);
         for (auto *Arg : A->args()) {
-          warnIfMutexHeld(D, Exp, Arg);
+          warnIfMutexHeld(D, Exp, Arg, Self, Loc);
           // use for deferring a lock
-          if (isScopedVar)
-            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, VD);
+          if (!Scp.shouldIgnore())
+            Analyzer->getMutexIDs(ScopedReqsAndExcludes, A, Exp, D, Self);
         }
         break;
       }
@@ -1882,7 +1907,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
 
   // Add locks.
   FactEntry::SourceKind Source =
-      isScopedVar ? FactEntry::Managed : FactEntry::Acquired;
+      !Scp.shouldIgnore() ? FactEntry::Managed : FactEntry::Acquired;
   for (const auto &M : ExclusiveLocksToAdd)
     Analyzer->addLock(FSet, std::make_unique<LockableFactEntry>(M, LK_Exclusive,
                                                                 Loc, Source));
@@ -1890,15 +1915,9 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
     Analyzer->addLock(
         FSet, std::make_unique<LockableFactEntry>(M, LK_Shared, Loc, Source));
 
-  if (isScopedVar) {
+  if (!Scp.shouldIgnore()) {
     // 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);
-
-    auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, MLoc);
+    auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(Scp, Loc);
     for (const auto &M : ExclusiveLocksToAdd)
       ScopedEntry->addLock(M);
     for (const auto &M : SharedLocksToAdd)
@@ -2058,36 +2077,11 @@ void BuildLockset::VisitCXXConstructExpr(const CXXConstructExpr *Exp) {
   } else {
     examineArguments(D, Exp->arg_begin(), Exp->arg_end());
   }
+  if (D && D->hasAttrs())
+    handleCall(Exp, D);
 }
 
-static CXXConstructorDecl *
-findConstructorForByValueReturn(const CXXRecordDecl *RD) {
-  // Prefer a move constructor over a copy constructor. If there's more than
-  // one copy constructor or more than one move constructor, we arbitrarily
-  // pick the first declared such constructor rather than trying to guess which
-  // one is more appropriate.
-  CXXConstructorDecl *CopyCtor = nullptr;
-  for (auto *Ctor : RD->ctors()) {
-    if (Ctor->isDeleted())
-      continue;
-    if (Ctor->isMoveConstructor())
-      return Ctor;
-    if (!CopyCtor && Ctor->isCopyConstructor())
-      CopyCtor = Ctor;
-  }
-  return CopyCtor;
-}
-
-static Expr *buildFakeCtorCall(CXXConstructorDecl *CD, ArrayRef<Expr *> Args,
-                               SourceLocation Loc) {
-  ASTContext &Ctx = CD->getASTContext();
-  return CXXConstructExpr::Create(Ctx, Ctx.getRecordType(CD->getParent()), Loc,
-                                  CD, true, Args, false, false, false, false,
-                                  CXXConstructExpr::CK_Complete,
-                                  SourceRange(Loc, Loc));
-}
-
-static Expr *UnpackConstruction(Expr *E) {
+static const Expr *UnpackConstruction(const Expr *E) {
   if (auto *CE = dyn_cast<CastExpr>(E))
     if (CE->getCastKind() == CK_NoOp)
       E = CE->getSubExpr()->IgnoreParens();
@@ -2106,7 +2100,7 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
 
   for (auto *D : S->getDeclGroup()) {
     if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
-      Expr *E = VD->getInit();
+      const Expr *E = VD->getInit();
       if (!E)
         continue;
       E = E->IgnoreParens();
@@ -2116,29 +2110,27 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
         E = EWC->getSubExpr()->IgnoreParens();
       E = UnpackConstruction(E);
 
-      if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
-        const auto *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
-        if (!CtorD || !CtorD->hasAttrs())
-          continue;
-        handleCall(E, CtorD, VD);
-      } else if (isa<CallExpr>(E) && E->isPRValue()) {
-        // If the object is initialized by a function call that returns a
-        // scoped lockable by value, use the attributes on the copy or move
-        // constructor to figure out what effect that should have on the
-        // lockset.
-        // FIXME: Is this really the best way to handle this situation?
-        auto *RD = E->getType()->getAsCXXRecordDecl();
-        if (!RD || !RD->hasAttr<ScopedLockableAttr>())
-          continue;
-        CXXConstructorDecl *CtorD = findConstructorForByValueReturn(RD);
-        if (!CtorD || !CtorD->hasAttrs())
-          continue;
-        handleCall(buildFakeCtorCall(CtorD, {E}, E->getBeginLoc()), CtorD, VD);
+      if (auto Object = ConstructedObjects.find(E);
+          Object != ConstructedObjects.end()) {
+        Object->second->setClangDecl(VD);
+        ConstructedObjects.erase(Object);
       }
     }
   }
 }
 
+void BuildLockset::VisitMaterializeTemporaryExpr(
+    const MaterializeTemporaryExpr *Exp) {
+  if (const ValueDecl *ExtD = Exp->getExtendingDecl()) {
+    if (auto Object =
+            ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr()));
+        Object != ConstructedObjects.end()) {
+      Object->second->setClangDecl(ExtD);
+      ConstructedObjects.erase(Object);
+    }
+  }
+}
+
 /// Given two facts merging on a join point, possibly warn and decide whether to
 /// keep or replace.
 ///
@@ -2411,19 +2403,33 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
           LocksetBuilder.Visit(CS.getStmt());
           break;
         }
-        // Ignore BaseDtor, MemberDtor, and TemporaryDtor for now.
+        // Ignore BaseDtor and MemberDtor for now.
         case CFGElement::AutomaticObjectDtor: {
           CFGAutomaticObjDtor AD = BI.castAs<CFGAutomaticObjDtor>();
           const auto *DD = AD.getDestructorDecl(AC.getASTContext());
           if (!DD->hasAttrs())
             break;
 
-          // Create a dummy expression,
-          auto *VD = const_cast<VarDecl *>(AD.getVarDecl());
-          DeclRefExpr DRE(VD->getASTContext(), VD, false,
-                          VD->getType().getNonReferenceType(), VK_LValue,
-                          AD.getTriggerStmt()->getEndLoc());
-          LocksetBuilder.handleCall(&DRE, DD);
+          LocksetBuilder.handleCall(nullptr, DD,
+                                    SxBuilder.createVariable(AD.getVarDecl()),
+                                    AD.getTriggerStmt()->getEndLoc());
+          break;
+        }
+        case CFGElement::TemporaryDtor: {
+          auto TD = BI.castAs<CFGTemporaryDtor>();
+
+          // Clean up constructed object even if there are no attributes to
+          // keep the number of objects in limbo as small as possible.
+          if (auto Object = LocksetBuilder.ConstructedObjects.find(
+                  TD.getBindTemporaryExpr()->getSubExpr());
+              Object != LocksetBuilder.ConstructedObjects.end()) {
+            const auto *DD = TD.getDestructorDecl(AC.getASTContext());
+            if (DD->hasAttrs())
+              // TODO: the location here isn't quite correct.
+              LocksetBuilder.handleCall(nullptr, DD, Object->second,
+                                        TD.getBindTemporaryExpr()->getEndLoc());
+            LocksetBuilder.ConstructedObjects.erase(Object);
+          }
           break;
         }
         default: