Thread safety analysis: Examine constructor arguments

Summary:
Instead of only examining call arguments, we also examine constructor
arguments applying the same rules.

That was an opportunity for refactoring the examination procedure to
work with iterators instead of integer indices. For the case of
CallExprs no functional change is intended.

Reviewers: aaron.ballman, delesley

Reviewed By: delesley

Subscribers: JonasToth, cfe-commits

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

2 files changed, 75 insertions, 51 deletions

diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index be1f8b8eebe..846dc80a4d0 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1538,6 +1538,10 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
                      ProtectedOperationKind POK = POK_VarAccess);
 
   void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr);
+  void examineArguments(const FunctionDecl *FD,
+                        CallExpr::const_arg_iterator ArgBegin,
+                        CallExpr::const_arg_iterator ArgEnd,
+                        bool SkipFirstParam = false);
 
 public:
   BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
@@ -1938,10 +1942,37 @@ void BuildLockset::VisitCastExpr(const CastExpr *CE) {
   checkAccess(CE->getSubExpr(), AK_Read);
 }
 
-void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
-  bool ExamineArgs = true;
-  bool OperatorFun = false;
+void BuildLockset::examineArguments(const FunctionDecl *FD,
+                                    CallExpr::const_arg_iterator ArgBegin,
+                                    CallExpr::const_arg_iterator ArgEnd,
+                                    bool SkipFirstParam) {
+  // Currently we can't do anything if we don't know the function declaration.
+  if (!FD)
+    return;
+
+  // NO_THREAD_SAFETY_ANALYSIS does double duty here.  Normally it
+  // only turns off checking within the body of a function, but we also
+  // use it to turn off checking in arguments to the function.  This
+  // could result in some false negatives, but the alternative is to
+  // create yet another attribute.
+  if (FD->hasAttr<NoThreadSafetyAnalysisAttr>())
+    return;
+
+  const ArrayRef<ParmVarDecl *> Params = FD->parameters();
+  auto Param = Params.begin();
+  if (SkipFirstParam)
+    ++Param;
+
+  // There can be default arguments, so we stop when one iterator is at end().
+  for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd;
+       ++Param, ++Arg) {
+    QualType Qt = (*Param)->getType();
+    if (Qt->isReferenceType())
+      checkAccess(*Arg, AK_Read, POK_PassByRef);
+  }
+}
 
+void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
   if (const auto *CE = dyn_cast<CXXMemberCallExpr>(Exp)) {
     const auto *ME = dyn_cast<MemberExpr>(CE->getCallee());
     // ME can be null when calling a method pointer
@@ -1960,13 +1991,12 @@ void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
           checkAccess(CE->getImplicitObjectArgument(), AK_Read);
       }
     }
-  } else if (const auto *OE = dyn_cast<CXXOperatorCallExpr>(Exp)) {
-    OperatorFun = true;
 
+    examineArguments(CE->getDirectCallee(), CE->arg_begin(), CE->arg_end());
+  } else if (const auto *OE = dyn_cast<CXXOperatorCallExpr>(Exp)) {
     auto OEop = OE->getOperator();
     switch (OEop) {
       case OO_Equal: {
-        ExamineArgs = false;
         const Expr *Target = OE->getArg(0);
         const Expr *Source = OE->getArg(1);
         checkAccess(Target, AK_Written);
@@ -1975,60 +2005,27 @@ void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
       }
       case OO_Star:
       case OO_Arrow:
-      case OO_Subscript: {
-        const Expr *Obj = OE->getArg(0);
-        checkAccess(Obj, AK_Read);
+      case OO_Subscript:
         if (!(OEop == OO_Star && OE->getNumArgs() > 1)) {
           // Grrr.  operator* can be multiplication...
-          checkPtAccess(Obj, AK_Read);
+          checkPtAccess(OE->getArg(0), AK_Read);
         }
-        break;
-      }
+        LLVM_FALLTHROUGH;
       default: {
         // TODO: get rid of this, and rely on pass-by-ref instead.
         const Expr *Obj = OE->getArg(0);
         checkAccess(Obj, AK_Read);
+        // Check the remaining arguments. For method operators, the first
+        // argument is the implicit self argument, and doesn't appear in the
+        // FunctionDecl, but for non-methods it does.
+        const FunctionDecl *FD = OE->getDirectCallee();
+        examineArguments(FD, std::next(OE->arg_begin()), OE->arg_end(),
+                         /*SkipFirstParam*/ !isa<CXXMethodDecl>(FD));
         break;
       }
     }
-  }
-
-  if (ExamineArgs) {
-    if (const FunctionDecl *FD = Exp->getDirectCallee()) {
-      // NO_THREAD_SAFETY_ANALYSIS does double duty here.  Normally it
-      // only turns off checking within the body of a function, but we also
-      // use it to turn off checking in arguments to the function.  This
-      // could result in some false negatives, but the alternative is to
-      // create yet another attribute.
-      if (!FD->hasAttr<NoThreadSafetyAnalysisAttr>()) {
-        unsigned Fn = FD->getNumParams();
-        unsigned Cn = Exp->getNumArgs();
-        unsigned Skip = 0;
-
-        unsigned i = 0;
-        if (OperatorFun) {
-          if (isa<CXXMethodDecl>(FD)) {
-            // First arg in operator call is implicit self argument,
-            // and doesn't appear in the FunctionDecl.
-            Skip = 1;
-            Cn--;
-          } else {
-            // Ignore the first argument of operators; it's been checked above.
-            i = 1;
-          }
-        }
-        // Ignore default arguments
-        unsigned n = (Fn < Cn) ? Fn : Cn;
-
-        for (; i < n; ++i) {
-          const ParmVarDecl *Pvd = FD->getParamDecl(i);
-          const Expr *Arg = Exp->getArg(i + Skip);
-          QualType Qt = Pvd->getType();
-          if (Qt->isReferenceType())
-            checkAccess(Arg, AK_Read, POK_PassByRef);
-        }
-      }
-    }
+  } else {
+    examineArguments(Exp->getDirectCallee(), Exp->arg_begin(), Exp->arg_end());
   }
 
   auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
@@ -2042,8 +2039,9 @@ void BuildLockset::VisitCXXConstructExpr(const CXXConstructExpr *Exp) {
   if (D && D->isCopyConstructor()) {
     const Expr* Source = Exp->getArg(0);
     checkAccess(Source, AK_Read);
+  } else {
+    examineArguments(D, Exp->arg_begin(), Exp->arg_end());
   }
-  // FIXME -- only handles constructors in DeclStmt below.
 }
 
 static CXXConstructorDecl *
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index ce3c6b9fcc0..9df0e2a3b4d 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4997,6 +4997,8 @@ public:
   void operator+(const Foo& f);
 
   void operator[](const Foo& g);
+
+  void operator()();
 };
 
 template<class T>
@@ -5014,8 +5016,23 @@ void destroy(Foo&& f);
 void operator/(const Foo& f, const Foo& g);
 void operator*(const Foo& f, const Foo& g);
 
+// Test constructors.
+struct FooRead {
+  FooRead(const Foo &);
+};
+struct FooWrite {
+  FooWrite(Foo &);
+};
 
+// Test variadic functions
+template<typename... T>
+void copyVariadic(T...) {}
+template<typename... T>
+void writeVariadic(T&...) {}
+template<typename... T>
+void readVariadic(const T&...) {}
 
+void copyVariadicC(int, ...);
 
 class Bar {
 public:
@@ -5047,6 +5064,14 @@ public:
     read2(10, foo);        // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     destroy(mymove(foo));  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
+    copyVariadic(foo);     // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
+    readVariadic(foo);     // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    writeVariadic(foo);    // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    copyVariadicC(1, foo); // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
+
+    FooRead reader(foo);   // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+    FooWrite writer(foo);  // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
+
     mwrite1(foo);           // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     mwrite2(10, foo);       // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     mread1(foo);            // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
@@ -5065,6 +5090,7 @@ public:
                              // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
     foo[foo2];               // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
                              // expected-warning {{passing variable 'foo2' by reference requires holding mutex 'mu'}}
+    foo();                   // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
     (*this) << foo;          // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
     copy(*foop);             // expected-warning {{reading the value pointed to by 'foop' requires holding mutex 'mu'}}