[PATCH] D52443: Thread safety analysis: Examine constructor arguments
This revision was automatically updated to reflect the committed changes.
Closed by commit rC343831: Thread safety analysis: Examine constructor
arguments (authored by aaronpuchert, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D52443?vs=167856=168414#toc
Repository:
rC Clang
https://reviews.llvm.org/D52443
Files:
lib/Analysis/ThreadSafety.cpp
test/SemaCXX/warn-thread-safety-analysis.cpp
Index: lib/Analysis/ThreadSafety.cpp
===
--- lib/Analysis/ThreadSafety.cpp
+++ lib/Analysis/ThreadSafety.cpp
@@ -1538,6 +1538,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 )
@@ -1938,10 +1942,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())
+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,75 +1991,41 @@
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);
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 = 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
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
delesley accepted this revision.
delesley added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Analysis/ThreadSafety.cpp:2046
const CXXConstructorDecl *D = Exp->getConstructor();
if (D && D->isCopyConstructor()) {
const Expr* Source = Exp->getArg(0);
aaronpuchert wrote:
> delesley wrote:
> > As a side note, we should probably special-case the move constructor too,
> > with AK_Written. That should be in a separate patch, though, and needs to
> > be sequestered under -Wthread-safety-beta, which is complicated.
> I think your arguments from D52395 apply here as well: the move constructor
> does not necessarily write. Many simple types are trivially move
> constructible, and then the move constructor is effectively the same as the
> copy constructor.
Good point.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
aaronpuchert marked 2 inline comments as done.
aaronpuchert added a comment.
Since the (remaining) arguments are examined in a separate function, I thought
I'd eliminate the boolean variables in `VisitCallExpr`. Apparently I prefer
control flow over booleans, but if you disagree I can obviously change it back.
Comment at: lib/Analysis/ThreadSafety.cpp:2046
const CXXConstructorDecl *D = Exp->getConstructor();
if (D && D->isCopyConstructor()) {
const Expr* Source = Exp->getArg(0);
delesley wrote:
> As a side note, we should probably special-case the move constructor too,
> with AK_Written. That should be in a separate patch, though, and needs to be
> sequestered under -Wthread-safety-beta, which is complicated.
I think your arguments from D52395 apply here as well: the move constructor
does not necessarily write. Many simple types are trivially move constructible,
and then the move constructor is effectively the same as the copy constructor.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
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
@@ -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
+void copyVariadic(T...) {}
+template
+void writeVariadic(T&...) {}
+template
+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 )
@@ -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())
+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
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
delesley added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:2046
const CXXConstructorDecl *D = Exp->getConstructor();
if (D && D->isCopyConstructor()) {
const Expr* Source = Exp->getArg(0);
As a side note, we should probably special-case the move constructor too, with
AK_Written. That should be in a separate patch, though, and needs to be
sequestered under -Wthread-safety-beta, which is complicated.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
delesley added a comment. This looks good, and resolves an outstanding bug that was on my list. Thanks for the patch! Comment at: lib/Analysis/ThreadSafety.cpp:1537 void handleCall(const Expr *Exp, const NamedDecl *D, VarDecl *VD = nullptr); + void ExamineCallArguments(const FunctionDecl *FD, +CallExpr::const_arg_iterator ArgBegin, Nit: capitalization does not match other methods. Repository: rC Clang https://reviews.llvm.org/D52443 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
aaron.ballman added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:1970
+ // 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) {
aaronpuchert wrote:
> aaron.ballman wrote:
> > How should this interact with variadic functions? Either ones that use
> > `...` with a C varargs function, or ones that use parameter packs in C++.
> > (I realize this is basically existing code, but the question remains.)
> For instantiated variadic templates we match against the instantiation of the
> template, so we can match the parameters just as for an ordinary function. My
> understanding is that the thread safety analysis doesn't run over templates,
> only instantiations, so that should be fine.
>
> With C variadic arguments we should have a shorter parameter list, so we
> don't check the matching variadic arguments. However, if I'm not mistakten,
> the variadic arguments are passed by value, so the reference checks aren't
> needed.
>
> Maybe I can add some tests for these cases.
Good points -- we're likely fine here, but if we lack some test coverage for
this, it'd be good to add some. If you find you need to add tests that wind up
Just Working, feel free to commit them without review.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
aaronpuchert added inline comments.
Comment at: lib/Analysis/ThreadSafety.cpp:1970
+ // 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) {
aaron.ballman wrote:
> How should this interact with variadic functions? Either ones that use `...`
> with a C varargs function, or ones that use parameter packs in C++. (I
> realize this is basically existing code, but the question remains.)
For instantiated variadic templates we match against the instantiation of the
template, so we can match the parameters just as for an ordinary function. My
understanding is that the thread safety analysis doesn't run over templates,
only instantiations, so that should be fine.
With C variadic arguments we should have a shorter parameter list, so we don't
check the matching variadic arguments. However, if I'm not mistakten, the
variadic arguments are passed by value, so the reference checks aren't needed.
Maybe I can add some tests for these cases.
Comment at: lib/Analysis/ThreadSafety.cpp:2050
+ } else {
+ExamineCallArguments(D, Exp->arg_begin(), Exp->arg_end(), false);
}
aaron.ballman wrote:
> Can you add a comment for the bool argument? e.g., `/*OperatorFun*/false`
Depending on `OperatorFun`, we shift some of the iterators. We could also do
that on the caller site. I'll see if that looks better.
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
aaron.ballman added a comment.
This generally looks sensible to me.
Comment at: lib/Analysis/ThreadSafety.cpp:1970
+ // 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) {
How should this interact with variadic functions? Either ones that use `...`
with a C varargs function, or ones that use parameter packs in C++. (I realize
this is basically existing code, but the question remains.)
Comment at: lib/Analysis/ThreadSafety.cpp:2050
+ } else {
+ExamineCallArguments(D, Exp->arg_begin(), Exp->arg_end(), false);
}
Can you add a comment for the bool argument? e.g., `/*OperatorFun*/false`
Repository:
rC Clang
https://reviews.llvm.org/D52443
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
aaronpuchert added a comment. There is a (technical) merge conflict between this change and https://reviews.llvm.org/D52395, but that shouldn't be of any concern for the review. The issues are rather independent. (I think.) Repository: rC Clang https://reviews.llvm.org/D52443 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52443: Thread safety analysis: Examine constructor arguments
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 )
@@ -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())
+return;
+
+ const ArrayRef 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(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()) {
-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--;
