aaronpuchert updated this revision to Diff 167856.
aaronpuchert marked 4 inline comments as done.
aaronpuchert added a comment.
Moved iterator shifting to VisitCallExpr, eliminated boolean variables, and
minor corrections as suggested in the review. There remains no intended
functional change to VisitCallExpr.
Repository:
rC Clang
https://reviews.llvm.org/D52443
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- test/SemaCXX/warn-thread-safety-analysis.cpp
+++ test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -4982,6 +4982,8 @@
void operator+(const Foo& f);
void operator[](const Foo& g);
+
+ void operator()();
};
template<class T>
@@ -4999,8 +5001,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<typename... T>
+void copyVariadic(T...) {}
+template<typename... T>
+void writeVariadic(T&...) {}
+template<typename... T>
+void readVariadic(const T&...) {}
+void copyVariadicC(int, ...);
class Bar {
public:
@@ -5032,6 +5049,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'}}
@@ -5050,6 +5075,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'}}
Index: lib/Analysis/ThreadSafety.cpp
===================================================================
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1534,6 +1534,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)
@@ -1934,10 +1938,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<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
@@ -1956,75 +1987,41 @@
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);
+ 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 = 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);
- }
- }
- }
+ } else {
+ examineArguments(Exp->getDirectCallee(), Exp->arg_begin(), Exp->arg_end());
}
auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
@@ -2038,8 +2035,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 *
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
