summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--clang/lib/Analysis/ThreadSafety.cpp100
-rw-r--r--clang/test/SemaCXX/warn-thread-safety-analysis.cpp26
2 files changed, 75 insertions, 51 deletions
diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index be1f8b8eebe..846dc80a4d0 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1538,6 +1538,10 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
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<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 CallExpr *Exp) {
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 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<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(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/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index ce3c6b9fcc0..9df0e2a3b4d 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/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<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'}}