From: Aaron Puchert Date: Thu, 4 Oct 2018 23:51:14 +0000 (+0000) Subject: Thread safety analysis: Examine constructor arguments X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=3d84c7ca0d8468fc4a275638c891a8d43b79c476;p=clang 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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@343831 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp index be1f8b8eeb..846dc80a4d 100644 --- a/lib/Analysis/ThreadSafety.cpp +++ b/lib/Analysis/ThreadSafety.cpp @@ -1538,6 +1538,10 @@ class BuildLockset : public ConstStmtVisitor { 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()) + 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 @@ void BuildLockset::VisitCallExpr(const CallExpr *Exp) { 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 @@ 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(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 @@ 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/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index ce3c6b9fcc..9df0e2a3b4 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/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 @@ -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 +void copyVariadic(T...) {} +template +void writeVariadic(T&...) {} +template +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'}}