handle scoped_lockable objects being returned by value in C++17

[rsmith]

Summary

In C++17, guaranteed copy elision means that there isn't necessarily a constructor call when a local variable is initialized by a function call that returns a scoped_lockable by value. That causes code such as:

// see test for definition of ReadLockedPtr
ReadLockedPtr<X> get();
void f() {
  auto my_ptr = get();
  my_ptr->do_something_requiring_lock();
}

... to fail for two reasons: the lock my_ptr->mutex is not held in the do_something_requiring_lock() call, and the lock my_ptr is not held at the point of the implicit destructor call at the end of the function.

There isn't really a good way to work around this: we don't have a list of "these are the locks you should expect to be held when an object is returned by value". But we can approximate that by pretending that the local my_ptr variable was actually initialized by running the move constructor of ReadLockedPtr, which is what this patch does.

[Note that this doesn't address the more problematic case where the returned type has no copy or move constructor. If that comes up, it can be hacked around with something like:

struct SCOPED_LOCKABLE MyImmovableButReturnableScopedLock {
  MyImmovableButReturnableScopedLock(MyImmovableButReturnableScopedLock &&) = delete;
  MyImmovableButReturnableScopedLock(volatile MyImmovableButReturnableScopedLock&&) ATTRIBUTES_HERE; // not defined
};

... on the basis that no sane program will ever actually call the other move constructor.]

Ideas for alternative approaches welcome. Ideally I'd like something that allows existing C++<=14 code to continue to work in C++17 mode unchanged, which I think this approach mostly provides.

[aaron.ballman]

I think this approach is a reasonable approximation, but @delesley has the final word.

lib/Analysis/ThreadSafety.cpp

1994

Can you sprinkle some const correctness around this declaration?

2001

Should we care about access checking here (protected ctor within a reasonable context, or friendship)?

[rsmith]

lib/Analysis/ThreadSafety.cpp

1994

I can't make the CXXConstructorDecl const without adding a const_cast somewhere or making very large-scale changes to Clang :) The CXXConstructExpr constructor needs a pointer to a non-const CXXConstructorDecl.

I can make the CXXRecordDecl const. I think that's actually a const-correctness bug -- if const on an AST node means anything, it should mean that the AST is potentially immutable, and you shouldn't be able to get from a const AST node to a non-const one -- but it's probably closer to the ideal than no const, so I'll add a const there.

2001

I'm not completely sure. My thinking was that if someone uses the "private copy constructor with no definition" approach rather than the "deleted copy constructor" approach, we should ignore that copy constructor. But this would only really matter if they do that to their *move* constructor and they have an accessible copy constructor, which seems like a sufficiently bizarre situation to not have a special rule for it. (Well, it'd also matter if the private copy ctor with no definition had thread safety attributes, which also seems like a very strange case.)

Happy to go either way here. On balance I think I agree that it's more consistent with the general language rules to ignore access here.

[aaron.ballman]

LGTM, but you should wait for a bit to see if DeLesley has concerns.

lib/Analysis/ThreadSafety.cpp

1994

Thanks! I agree with you that const on an AST node should mean that the AST is immutable, but I think getting to that point would be a pretty large change to Clang in general, unfortunately.

2001

I think ignoring access is a good first approximation. We can refine later if we find the analysis requires it.

[delesley]

LGTM. Thanks, Richard!

[rsmith]

Committed as r322316.

lib/Analysis/ThreadSafety.cpp

 ///
 void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) {
   SourceLocation Loc = Exp->getExprLoc();
   CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
   CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
   CapExprSet ScopedExclusiveReqs, ScopedSharedReqs;
   StringRef CapDiagKind = "mutex";
 
-  // Figure out if we're calling the constructor of scoped lockable class
+  // Figure out if we're constructing an object of scoped lockable class
   bool isScopedVar = false;
   if (VD) {
     if (const CXXConstructorDecl *CD = dyn_cast<const CXXConstructorDecl>(D)) {
       const CXXRecordDecl* PD = CD->getParent();
       if (PD && PD->hasAttr<ScopedLockableAttr>())
         isScopedVar = true;
     }
   }
void BuildLockset::VisitCXXConstructExpr(CXXConstructExpr *Exp) {
   const CXXConstructorDecl *D = Exp->getConstructor();
   if (D && D->isCopyConstructor()) {
     const Expr* Source = Exp->getArg(0);
     checkAccess(Source, AK_Read);
   }
   // FIXME -- only handles constructors in DeclStmt below.
 }
 
+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 (CXXConstructorDecl *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));
+}
+
 void BuildLockset::VisitDeclStmt(DeclStmt *S) {
   // adjust the context
   LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
 
   for (auto *D : S->getDeclGroup()) {
     if (VarDecl *VD = dyn_cast_or_null<VarDecl>(D)) {
       Expr *E = VD->getInit();
+      if (!E)
+        continue;
+      E = E->IgnoreParens();
+
       // handle constructors that involve temporaries
-      if (ExprWithCleanups *EWC = dyn_cast_or_null<ExprWithCleanups>(E))
+      if (auto *EWC = dyn_cast<ExprWithCleanups>(E))
         E = EWC->getSubExpr();
+      if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
+        E = BTE->getSubExpr();
 
-      if (CXXConstructExpr *CE = dyn_cast_or_null<CXXConstructExpr>(E)) {
+      if (CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(E)) {
         NamedDecl *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
         if (!CtorD || !CtorD->hasAttrs())
-          return;
-        handleCall(CE, CtorD, VD);
+          continue;
+        handleCall(E, CtorD, VD);
+      } else if (isa<CallExpr>(E) && E->isRValue()) {
+        // 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->getLocStart()), CtorD, VD);
       }
     }
   }
 }
 
 
 
 /// \brief Compute the intersection of two locksets and issue warnings for any

test/SemaCXX/warn-thread-safety-analysis.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=0 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=1 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_ASSERT_CAPABILITY=0 %s
 
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s
 // FIXME: should also run  %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
 
 #define LOCKABLE             __attribute__((lockable))
 #define SCOPED_LOCKABLE      __attribute__((scoped_lockable))
 #define GUARDED_BY(x)        __attribute__((guarded_by(x)))
 #define GUARDED_VAR          __attribute__((guarded_var))
struct B {
   int h;
 };
 struct C {
   B *operator[](int);
 };
 C c;
 void f() { c[A()]->g(); }
 } // namespace PR34800
+
+namespace ReturnScopedLockable {
+  template<typename Object> class SCOPED_LOCKABLE ReadLockedPtr {
+  public:
+    ReadLockedPtr(Object *ptr) SHARED_LOCK_FUNCTION((*this)->mutex);
+    ReadLockedPtr(ReadLockedPtr &&) SHARED_LOCK_FUNCTION((*this)->mutex);
+    ~ReadLockedPtr() UNLOCK_FUNCTION();
+
+    Object *operator->() const { return object; }
+
+  private:
+    Object *object;
+  };
+
+  struct Object {
+    int f() SHARED_LOCKS_REQUIRED(mutex);
+    Mutex mutex;
+  };
+
+  ReadLockedPtr<Object> get();
+  int use() {
+    auto ptr = get();
+    return ptr->f();
+  }
+}