From: Richard Smith <richard-llvm@metafoo.co.uk>
Date: Thu, 11 Jan 2018 22:13:57 +0000 (+0000)
Subject: Handle scoped_lockable objects being returned by value in C++17.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=568cb88091f86b41872e912622694a5a3f2632fa;p=clang

Handle scoped_lockable objects being returned by value in C++17.

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. In order to model the effects of
initializing a local variable with a function call returning a scoped_lockable,
pretend that the move constructor was invoked within the caller at the point of
return.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@322316 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp
index 6a9c9a04c5..f81d916bcb 100644
--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -1689,7 +1689,7 @@ void BuildLockset::handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD) {
   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)) {
@@ -1991,6 +1991,33 @@ void BuildLockset::VisitCXXConstructExpr(CXXConstructExpr *Exp) {
   // 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);
@@ -1998,15 +2025,34 @@ void BuildLockset::VisitDeclStmt(DeclStmt *S) {
   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);
       }
     }
   }
diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp
index fc1ea5e9e9..829e2e4916 100644
--- a/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -1,5 +1,6 @@
 // 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
@@ -5248,3 +5249,28 @@ struct C {
 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();
+  }
+}