r343831 - Thread safety analysis: Examine constructor arguments
Aaron Puchert via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 4 16:51:15 PDT 2018
Author: aaronpuchert
Date: Thu Oct 4 16:51:14 2018
New Revision: 343831
URL: http://llvm.org/viewvc/llvm-project?rev=343831&view=rev
Log:
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
Modified:
cfe/trunk/lib/Analysis/ThreadSafety.cpp
cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=343831&r1=343830&r2=343831&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
+++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Thu Oct 4 16:51:14 2018
@@ -1538,6 +1538,10 @@ class BuildLockset : public ConstStmtVis
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 C
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 C
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 C
}
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
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 *
Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=343831&r1=343830&r2=343831&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Thu Oct 4 16:51:14 2018
@@ -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'}}
More information about the cfe-commits
mailing list