Thread safety analysis: Support copy-elided production of scoped capa…

…bilities through arbitrary calls

When support for copy elision was initially added in e97654b, 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

clang/docs/ReleaseNotes.rst

@@ -307,6 +307,13 @@ Improvements to Clang's diagnostics
   function definition without a prototype which is preceded by a static
   declaration of the function with a prototype. Fixes
   `Issue 58181 <https://github.com/llvm/llvm-project/issues/58181>`_.
+- Copy-elided initialization of lock scopes is now handled differently in
+  ``-Wthread-safety-analysis``: annotations on the move constructor are no
+  longer taken into account, in favor of annotations on the function returning
+  the lock scope by value. This could result in new warnings if code depended
+  on the previous undocumented behavior. As a side effect of this change,
+  constructor calls outside of initializer expressions are no longer ignored,
+  which can result in new warnings (or make existing warnings disappear).
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

clang/docs/ThreadSafetyAnalysis.rst

@@ -408,7 +408,8 @@ and destructor refer to the capability via different names; see the
 Scoped capabilities are treated as capabilities that are implicitly acquired
 on construction and released on destruction. They are associated with
 the set of (regular) capabilities named in thread safety attributes on the
-constructor. Acquire-type attributes on other member functions are treated as
+constructor or function returning them by value (using C++17 guaranteed copy
+elision). Acquire-type attributes on other member functions are treated as
 applying to that set of associated capabilities, while ``RELEASE`` implies that
 a function releases all associated capabilities in whatever mode they're held.
 
@@ -930,6 +931,13 @@ implementation.
     // Assume mu is not held, implicitly acquire *this and associate it with mu.
     MutexLocker(Mutex *mu, defer_lock_t) EXCLUDES(mu) : mut(mu), locked(false) {}
 
+    // Same as constructors, but without tag types. (Requires C++17 copy elision.)
+    static MutexLocker Lock(Mutex *mu) ACQUIRE(mu);
+    static MutexLocker Adopt(Mutex *mu) REQUIRES(mu);
+    static MutexLocker ReaderLock(Mutex *mu) ACQUIRE_SHARED(mu);
+    static MutexLocker AdoptReaderLock(Mutex *mu) REQUIRES_SHARED(mu);
+    static MutexLocker DeferLock(Mutex *mu) EXCLUDES(mu);
+
     // Release *this and all associated mutexes, if they are still held.
     // There is no warning if the scope was already unlocked before.
     ~MutexLocker() RELEASE() {

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

@@ -31,6 +31,7 @@
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/PointerIntPair.h"
+#include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include <sstream>
@@ -354,7 +355,7 @@ class SExprBuilder {
     const NamedDecl *AttrDecl;
 
     // Implicit object argument -- e.g. 'this'
-    const Expr *SelfArg = nullptr;
+    llvm::PointerUnion<const Expr *, til::SExpr *> SelfArg = nullptr;
 
     // Number of funArgs
     unsigned NumArgs = 0;
@@ -378,10 +379,18 @@ class SExprBuilder {
   // Translate a clang expression in an attribute to a til::SExpr.
   // Constructs the context from D, DeclExp, and SelfDecl.
   CapabilityExpr translateAttrExpr(const Expr *AttrExp, const NamedDecl *D,
-                                   const Expr *DeclExp, VarDecl *SelfD=nullptr);
+                                   const Expr *DeclExp,
+                                   til::SExpr *Self = nullptr);
 
   CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx);
 
+  // Translate a variable reference.
+  til::LiteralPtr *createVariable(const VarDecl *VD);
+
+  // Create placeholder for this: we don't know the VarDecl on construction yet.
+  std::pair<til::LiteralPtr *, StringRef>
+  createThisPlaceholder(const Expr *Exp);
+
   // 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.

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

@@ -634,15 +634,14 @@ typename V::R_SExpr Literal::traverse(V &Vs, typename V::R_Ctx Ctx) {
 /// At compile time, pointer literals are represented by symbolic names.
 class LiteralPtr : public SExpr {
 public:
-  LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {
-    assert(D && "ValueDecl must not be null");
-  }
+  LiteralPtr(const ValueDecl *D) : SExpr(COP_LiteralPtr), Cvdecl(D) {}
   LiteralPtr(const LiteralPtr &) = default;
 
   static bool classof(const SExpr *E) { return E->opcode() == COP_LiteralPtr; }
 
   // The clang declaration for the value that this pointer points to.
   const ValueDecl *clangDecl() const { return Cvdecl; }
+  void setClangDecl(const ValueDecl *VD) { Cvdecl = VD; }
 
   template <class V>
   typename V::R_SExpr traverse(V &Vs, typename V::R_Ctx Ctx) {
@@ -651,6 +650,8 @@ class LiteralPtr : public SExpr {
 
   template <class C>
   typename C::CType compare(const LiteralPtr* E, C& Cmp) const {
+    if (!Cvdecl || !E->Cvdecl)
+      return Cmp.comparePointers(this, E);
     return Cmp.comparePointers(Cvdecl, E->Cvdecl);
   }
 

clang/include/clang/Analysis/Analyses/ThreadSafetyTraverse.h

@@ -623,7 +623,10 @@ class PrettyPrinter {
   }
 
   void printLiteralPtr(const LiteralPtr *E, StreamType &SS) {
-    SS << E->clangDecl()->getNameAsString();
+    if (const NamedDecl *D = E->clangDecl())
+      SS << D->getNameAsString();
+    else
+      SS << "<temporary>";
   }
 
   void printVariable(const Variable *V, StreamType &SS, bool IsVarDecl=false) {