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

This caused false positives, see comment on the code review.

> 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

This reverts commit 0041a69495f828f6732803cfb0f1e3fddd7fbf2a and the follow-up
warning fix in 83d93d3c11ac9727bf3d4c5c956de44233cc7f87.

1 files changed, 17 insertions, 29 deletions

diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp
index a771149..06b61b4 100644
--- a/clang/lib/Analysis/ThreadSafetyCommon.cpp
+++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp
@@ -115,22 +115,19 @@ static StringRef ClassifyDiagnostic(QualType VDT) {
 /// \param D       The declaration to which the attribute is attached.
 /// \param DeclExp An expression involving the Decl to which the attribute
 ///                is attached.  E.g. the call to a function.
-/// \param Self    S-expression to substitute for a \ref CXXThisExpr.
 CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
                                                const NamedDecl *D,
                                                const Expr *DeclExp,
-                                               til::SExpr *Self) {
+                                               VarDecl *SelfDecl) {
   // If we are processing a raw attribute expression, with no substitutions.
-  if (!DeclExp && !Self)
+  if (!DeclExp)
     return translateAttrExpr(AttrExp, nullptr);
 
   CallingContext Ctx(nullptr, D);
 
   // Examine DeclExp to find SelfArg and FunArgs, which are used to substitute
   // for formal parameters when we call buildMutexID later.
-  if (!DeclExp)
-    /* We'll use Self. */;
-  else if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) {
+  if (const auto *ME = dyn_cast<MemberExpr>(DeclExp)) {
     Ctx.SelfArg   = ME->getBase();
     Ctx.SelfArrow = ME->isArrow();
   } else if (const auto *CE = dyn_cast<CXXMemberCallExpr>(DeclExp)) {
@@ -145,24 +142,29 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
     Ctx.SelfArg = nullptr;  // Will be set below
     Ctx.NumArgs = CE->getNumArgs();
     Ctx.FunArgs = CE->getArgs();
+  } else if (D && isa<CXXDestructorDecl>(D)) {
+    // There's no such thing as a "destructor call" in the AST.
+    Ctx.SelfArg = DeclExp;
   }
 
-  if (Self) {
-    assert(!Ctx.SelfArg && "Ambiguous self argument");
-    Ctx.SelfArg = Self;
+  // Hack to handle constructors, where self cannot be recovered from
+  // the expression.
+  if (SelfDecl && !Ctx.SelfArg) {
+    DeclRefExpr SelfDRE(SelfDecl->getASTContext(), SelfDecl, false,
+                        SelfDecl->getType(), VK_LValue,
+                        SelfDecl->getLocation());
+    Ctx.SelfArg = &SelfDRE;
 
     // If the attribute has no arguments, then assume the argument is "this".
     if (!AttrExp)
-      return CapabilityExpr(
-          Self, ClassifyDiagnostic(cast<CXXMethodDecl>(D)->getThisObjectType()),
-          false);
+      return translateAttrExpr(Ctx.SelfArg, nullptr);
     else  // For most attributes.
       return translateAttrExpr(AttrExp, &Ctx);
   }
 
   // If the attribute has no arguments, then assume the argument is "this".
   if (!AttrExp)
-    return translateAttrExpr(cast<const Expr *>(Ctx.SelfArg), nullptr);
+    return translateAttrExpr(Ctx.SelfArg, nullptr);
   else  // For most attributes.
     return translateAttrExpr(AttrExp, &Ctx);
 }
@@ -216,16 +218,6 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
   return CapabilityExpr(E, Kind, Neg);
 }
 
-til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
-  return new (Arena) til::LiteralPtr(VD);
-}
-
-std::pair<til::LiteralPtr *, StringRef>
-SExprBuilder::createThisPlaceholder(const Expr *Exp) {
-  return {new (Arena) til::LiteralPtr(nullptr),
-          ClassifyDiagnostic(Exp->getType())};
-}
-
 // Translate a clang statement or expression to a TIL expression.
 // Also performs substitution of variables; Ctx provides the context.
 // Dispatches on the type of S.
@@ -335,12 +327,8 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
 til::SExpr *SExprBuilder::translateCXXThisExpr(const CXXThisExpr *TE,
                                                CallingContext *Ctx) {
   // Substitute for 'this'
-  if (Ctx && Ctx->SelfArg) {
-    if (const auto *SelfArg = dyn_cast<const Expr *>(Ctx->SelfArg))
-      return translate(SelfArg, Ctx->Prev);
-    else
-      return cast<til::SExpr *>(Ctx->SelfArg);
-  }
+  if (Ctx && Ctx->SelfArg)
+    return translate(Ctx->SelfArg, Ctx->Prev);
   assert(SelfVar && "We have no variable for 'this'!");
   return SelfVar;
 }