aaronpuchert created this revision.
aaronpuchert added reviewers: aaron.ballman, delesley.
Herald added a subscriber: cfe-commits.
Instead of only examining call arguments, we also examine constructor
arguments applying the same rules.
That was an oppurtunity for refactoring the examination procedure to
work with iterators instead of integer indices. For the case of
CallExprs no functional change is intended.
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
@@ -4999,8 +4999,13 @@
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 &);
+};
class Bar {
public:
@@ -5032,6 +5037,9 @@
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'}}
+ 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'}}
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 ExamineCallArguments(const FunctionDecl *FD,
+ CallExpr::const_arg_iterator ArgBegin,
+ CallExpr::const_arg_iterator ArgEnd,
+ bool OperatorFun);
public:
BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
@@ -1934,6 +1938,43 @@
checkAccess(CE->getSubExpr(), AK_Read);
}
+void BuildLockset::ExamineCallArguments(const FunctionDecl *FD,
+ CallExpr::const_arg_iterator ArgBegin,
+ CallExpr::const_arg_iterator ArgEnd,
+ bool OperatorFun) {
+ // 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 (OperatorFun) {
+ // Ignore the first argument of operators; it's been checked elsewhere.
+ ++ArgBegin;
+
+ // For method operators, the first argument is the implicit self argument,
+ // and doesn't appear in the FunctionDecl, but for non-methods it does.
+ if (!isa<CXXMethodDecl>(FD))
+ ++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) {
bool ExamineArgs = true;
bool OperatorFun = false;
@@ -1990,41 +2031,8 @@
}
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);
- }
- }
- }
+ const FunctionDecl *FD = Exp->getDirectCallee();
+ ExamineCallArguments(FD, Exp->arg_begin(), Exp->arg_end(), OperatorFun);
}
auto *D = dyn_cast_or_null<NamedDecl>(Exp->getCalleeDecl());
@@ -2038,8 +2046,9 @@
if (D && D->isCopyConstructor()) {
const Expr* Source = Exp->getArg(0);
checkAccess(Source, AK_Read);
+ } else {
+ ExamineCallArguments(D, Exp->arg_begin(), Exp->arg_end(), false);
}
- // 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
