Thread safety analysis: Examine constructor arguments

[aaronpuchert]

Summary

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

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

[aaronpuchert]

There is a (technical) merge conflict between this change and D52395, but that shouldn't be of any concern for the review. The issues are rather independent. (I think.)

[aaron.ballman]

This generally looks sensible to me.

lib/Analysis/ThreadSafety.cpp

1970

How should this interact with variadic functions? Either ones that use ... with a C varargs function, or ones that use parameter packs in C++. (I realize this is basically existing code, but the question remains.)

2039

Can you add a comment for the bool argument? e.g., /*OperatorFun*/false

[aaronpuchert]

lib/Analysis/ThreadSafety.cpp

1970

For instantiated variadic templates we match against the instantiation of the template, so we can match the parameters just as for an ordinary function. My understanding is that the thread safety analysis doesn't run over templates, only instantiations, so that should be fine.

With C variadic arguments we should have a shorter parameter list, so we don't check the matching variadic arguments. However, if I'm not mistakten, the variadic arguments are passed by value, so the reference checks aren't needed.

Maybe I can add some tests for these cases.

2039

Depending on OperatorFun, we shift some of the iterators. We could also do that on the caller site. I'll see if that looks better.

[aaron.ballman]

lib/Analysis/ThreadSafety.cpp

1970

Good points -- we're likely fine here, but if we lack some test coverage for this, it'd be good to add some. If you find you need to add tests that wind up Just Working, feel free to commit them without review.

[delesley]

This looks good, and resolves an outstanding bug that was on my list. Thanks for the patch!

lib/Analysis/ThreadSafety.cpp

1537

Nit: capitalization does not match other methods.

[delesley]

lib/Analysis/ThreadSafety.cpp

2035

As a side note, we should probably special-case the move constructor too, with AK_Written. That should be in a separate patch, though, and needs to be sequestered under -Wthread-safety-beta, which is complicated.

[aaronpuchert]

Moved iterator shifting to VisitCallExpr, eliminated boolean variables, and minor corrections as suggested in the review. There remains no intended functional change to VisitCallExpr.

[aaronpuchert]

Since the (remaining) arguments are examined in a separate function, I thought I'd eliminate the boolean variables in VisitCallExpr. Apparently I prefer control flow over booleans, but if you disagree I can obviously change it back.

lib/Analysis/ThreadSafety.cpp

2035

I think your arguments from D52395 apply here as well: the move constructor does not necessarily write. Many simple types are trivially move constructible, and then the move constructor is effectively the same as the copy constructor.

[delesley]

lib/Analysis/ThreadSafety.cpp

2035

Good point.

lib/Analysis/ThreadSafety.cpp

void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
                        StringRef DiagKind);
 
   void checkAccess(const Expr *Exp, AccessKind AK,
                    ProtectedOperationKind POK = POK_VarAccess);
   void checkPtAccess(const Expr *Exp, AccessKind AK,
                      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)
       : ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
         LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
 
   void VisitUnaryOperator(const UnaryOperator *UO);
   void VisitBinaryOperator(const BinaryOperator *BO);

 /// need to ensure we hold any required mutexes.
 /// FIXME: Deal with non-primitive types.
 void BuildLockset::VisitCastExpr(const CastExpr *CE) {
   if (CE->getCastKind() != CK_LValueToRValue)
     return;
   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
     const CXXMethodDecl *MD = CE->getMethodDecl();
 
     if (ME && MD) {
       if (ME->isArrow()) {
         if (MD->isConst())
           checkPtAccess(CE->getImplicitObjectArgument(), AK_Read);
         else // FIXME -- should be AK_Written
           checkPtAccess(CE->getImplicitObjectArgument(), AK_Read);
       } else {
         if (MD->isConst())
           checkAccess(CE->getImplicitObjectArgument(), AK_Read);
         else     // FIXME -- should be AK_Written
           checkAccess(CE->getImplicitObjectArgument(), AK_Read);
       }
     }
-  } else if (const auto *OE = dyn_cast<CXXOperatorCallExpr>(Exp)) {
-    OperatorFun = true;
 
+    examineArguments(Exp->getDirectCallee(), Exp->arg_begin(), Exp->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);
         checkAccess(Source, AK_Read);
         break;
       }
       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);
-        }
-        break;
+          checkPtAccess(OE->getArg(0), AK_Read);
         }
+        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 = Exp->getDirectCallee();
+        examineArguments(FD, std::next(Exp->arg_begin()), Exp->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);
-        }
-      }
-    }
+    examineArguments(Exp->getDirectCallee(), Exp->arg_begin(), Exp->arg_end());
   }
 
   auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
   if(!D || !D->hasAttrs())
     return;
   handleCall(Exp, D);
 }
 
 void BuildLockset::VisitCXXConstructExpr(const CXXConstructExpr *Exp) {
   const CXXConstructorDecl *D = Exp->getConstructor();
   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 *
 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.

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

public:
   Foo() : a(0), b(0) { }
 
   int a;
   int b;
 
   void operator+(const Foo& f);
 
   void operator[](const Foo& g);
+
+  void operator()();
 };
 
 template<class T>
 T&& mymove(T& f);
 
 
 // test top-level functions
 void copy(Foo f);
 void write1(Foo& f);
 void write2(int a, Foo& f);
 void read1(const Foo& f);
 void read2(int a, const Foo& f);
 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:
   Mutex mu;
   Foo           foo   GUARDED_BY(mu);
   Foo           foo2  GUARDED_BY(mu);
   Foo*          foop  PT_GUARDED_BY(mu);
   SmartPtr<Foo> foosp PT_GUARDED_BY(mu);
public:
   void test1() {
     copy(foo);             // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
     write1(foo);           // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     write2(10, foo);       // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     read1(foo);            // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     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'}}
     mread2(10, foo);        // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
 
     smwrite1(foo);           // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     smwrite2(10, foo);       // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     smread1(foo);            // expected-warning {{passing variable 'foo' by reference requires holding mutex 'mu'}}
     smread2(10, foo);        // expected-warning {{passing variable 'foo' 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 / foo2;              // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} \
                              // 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[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'}}
     write1(*foop);           // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
     write2(10, *foop);       // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
     read1(*foop);            // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
     read2(10, *foop);        // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}
     destroy(mymove(*foop));  // expected-warning {{passing the value that 'foop' points to by reference requires holding mutex 'mu'}}