Index: lib/Analysis/ThreadSafety.cpp =================================================================== --- lib/Analysis/ThreadSafety.cpp +++ lib/Analysis/ThreadSafety.cpp @@ -1538,6 +1538,10 @@ 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 @@ 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()) + return; + + const ArrayRef 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(Exp)) { const auto *ME = dyn_cast(CE->getCallee()); // ME can be null when calling a method pointer @@ -1960,13 +1991,12 @@ checkAccess(CE->getImplicitObjectArgument(), AK_Read); } } - } else if (const auto *OE = dyn_cast(Exp)) { - OperatorFun = true; + examineArguments(CE->getDirectCallee(), CE->arg_begin(), CE->arg_end()); + } else if (const auto *OE = dyn_cast(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 @@ } 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(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()) { - unsigned Fn = FD->getNumParams(); - unsigned Cn = Exp->getNumArgs(); - unsigned Skip = 0; - - unsigned i = 0; - if (OperatorFun) { - if (isa(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(Exp->getCalleeDecl()); @@ -2042,8 +2039,9 @@ 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 * Index: test/SemaCXX/warn-thread-safety-analysis.cpp =================================================================== --- test/SemaCXX/warn-thread-safety-analysis.cpp +++ test/SemaCXX/warn-thread-safety-analysis.cpp @@ -4997,6 +4997,8 @@ void operator+(const Foo& f); void operator[](const Foo& g); + + void operator()(); }; template @@ -5014,8 +5016,23 @@ 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 +void copyVariadic(T...) {} +template +void writeVariadic(T&...) {} +template +void readVariadic(const T&...) {} - +void copyVariadicC(int, ...); class Bar { public: @@ -5047,6 +5064,14 @@ 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 @@ // 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'}}