[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added a comment. I think you can commit, there was enough opportunity to respond and we pinged directly as well. Repository: rC Clang https://reviews.llvm.org/D52219 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang updated this revision to Diff 168578.
shuaiwang marked 5 inline comments as done.
shuaiwang added a comment.
Resolved review comments.
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -50,13 +50,23 @@
}
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
- const auto *const S = selectFirst("stmt", Results);
- const auto *const E = selectFirst("expr", Results);
+ const auto *S = selectFirst("stmt", Results);
+ const auto *E = selectFirst("expr", Results);
+ assert(S && E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *S = selectFirst("stmt", Results);
+ const auto *E = selectFirst("expr", Results);
+ assert(S && E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ EXPECT_TRUE(isMutated(Results, AST));
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
@@ -71,6 +81,19 @@
return Chain;
}
+std::string pointeeMutatedBy(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ EXPECT_TRUE(isPointeeMutated(Results, AST));
+ const auto *S = selectFirst("stmt", Results);
+ const auto *E = selectFirst("expr", Results);
+ ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+ std::string buffer;
+ llvm::raw_string_ostream stream(buffer);
+ const Stmt *By = Analyzer.findPointeeMutation(E);
+ By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+ return StringRef(stream.str()).trim().str();
+}
+
std::string removeSpace(std::string s) {
s.erase(std::remove_if(s.begin(), s.end(),
[](char c) { return std::isspace(c); }),
@@ -100,10 +123,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -134,41 +161,111 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = buildASTFromCode(
+ auto AST = buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *x; Foo * = x; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
- AST = buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }",
- {"-fno-delayed-template-parsing"});
+ AST =
+ buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }"
+ "template void g() { T *p; p->mf(); }",
+
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added a comment.
LGTM, with a few nits and a question. Please wait a bit if @aaron.ballman or
@george.karpenkov have any comments before committing.
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:88
+bool isPointeeMutable(const Expr *Exp, const ASTContext ) {
+ if (const auto *PT = Exp->getType()->getAs()) {
+return !PT->getPointeeType().isConstant(Context);
you can elide the braces here
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:101
+ return false;
+// Check whether returning non-const reference.
+if (const auto *RT =
Please make this a full sentence (adding `... the overloaded 'operator*' is
returning ...` is enough.)
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:227
+ // `Exp` can be *directly* mutated if the type of `Exp` is not const.
+ // Bail out early otherwise.
+ if (Exp->getType().isConstant(Context))
This comment is one sentence, just replace `const. Bail` with `const, bail`
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:87
+ EXPECT_TRUE(isPointeeMutated(Results, AST));
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
pointer is `const`, please remove the last `const`
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:645
+ // TODO: this should work when casts of pointers are handled.
+ // EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "return x;");
I am confused. The `const int*` guarantees, that `x`-pointee is not mutated.
Why shall this be diagnosed as a mutation?
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang updated this revision to Diff 166947.
shuaiwang marked 6 inline comments as done.
shuaiwang added a comment.
Addressed review comments.
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -50,13 +50,23 @@
}
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
- const auto *const S = selectFirst("stmt", Results);
- const auto *const E = selectFirst("expr", Results);
+ const auto *S = selectFirst("stmt", Results);
+ const auto *E = selectFirst("expr", Results);
+ assert(S && E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *S = selectFirst("stmt", Results);
+ const auto *E = selectFirst("expr", Results);
+ assert(S && E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ EXPECT_TRUE(isMutated(Results, AST));
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
@@ -71,6 +81,19 @@
return Chain;
}
+std::string pointeeMutatedBy(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ EXPECT_TRUE(isPointeeMutated(Results, AST));
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+ std::string buffer;
+ llvm::raw_string_ostream stream(buffer);
+ const Stmt *By = Analyzer.findPointeeMutation(E);
+ By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+ return StringRef(stream.str()).trim().str();
+}
+
std::string removeSpace(std::string s) {
s.erase(std::remove_if(s.begin(), s.end(),
[](char c) { return std::isspace(c); }),
@@ -100,10 +123,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -134,41 +161,111 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = buildASTFromCode(
+ auto AST = buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *x; Foo * = x; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
- AST = buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }",
- {"-fno-delayed-template-parsing"});
+ AST =
+ buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }"
+ "template void g() { T *p; p->mf(); }",
+
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
shuaiwang wrote:
> JonasToth wrote:
> > shuaiwang wrote:
> > > JonasToth wrote:
> > > > JonasToth wrote:
> > > > > I feel that there a multiple tests missing:
> > > > >
> > > > > - multiple levels of pointers `int ***`, `int * const *`
> > > > > - pointers to references `int &*`
> > > > > - references to pointers `int *&`
> > > > > - ensure that having a const pointer does no influence the pointee
> > > > > analysis `int * const p = *p = 42;`
> > > > > - a class with `operator*` + `operator->` const/non-const and the
> > > > > analysis for pointers to that class
> > > > > - pointer returned from a function
> > > > > - non-const reference returned
> > > > > ```
> > > > > int& foo(int *p) {
> > > > > return *p;
> > > > > }
> > > > > ```
> > > > >
> > > > for the multi-level pointer mutation: it would be enough to test, that
> > > > the second layer is analyzed properly, and that the `int * >const< *`
> > > > would be detected.
> > > Added except for:
> > > - Anything that requires following a dereference, we need
> > > `findPointeeDerefMutation` for that.
> > > - Pointer to a class with `operator*` + `operator->`, I think those two
> > > operators doesn't matter, there's no way to accidentally invoke them from
> > > a pointer.
> > >
> > But we want to analyze smart pointers in the future as well, not? It would
> > be good to already prepare that in the testing department.
> > Or would the nature of `operator*` already make that happen magically?
> Yes we'll handle smart pointers, and we'll handle that in
> `findPointeeDerefMutation`, basically it'll look like:
> ```
> if (native pointer && derefed with *) findMutation(deref expr)
> if (smart pointer && called operator*) findMutation(operator call expr)
> if (smart pointer && called operator->) findPointeeMutation(operator call
> expr)
> ```
> I think it would be more clear if we can match the implementation progress
> with unit test cases as that shows what kind of cases starts to be supported
> by the change.
Ok.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:61
+ ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
Is there a reason why the pointer is marked `const`? Usually this is not done
in the codebase and I feel consistency is key (similar to `const` for values).
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:90
+ ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+ const Stmt *By = Analyzer.findPointeeMutation(E);
+ std::string buffer;
You can put this line 2 lines below, 'initialize late'
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:639
EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "return x;");
a nice test to add would be `const int* f() { int* x; return x; }.
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang updated this revision to Diff 166710.
shuaiwang added a comment.
Added test case place holder for cases that should be supported in later
patches.
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -52,11 +52,21 @@
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
const auto *const S = selectFirst("stmt", Results);
const auto *const E = selectFirst("expr", Results);
+ assert(S && E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ assert(S && E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ EXPECT_TRUE(isMutated(Results, AST));
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
@@ -71,6 +81,19 @@
return Chain;
}
+std::string pointeeMutatedBy(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ EXPECT_TRUE(isPointeeMutated(Results, AST));
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+ const Stmt *By = Analyzer.findPointeeMutation(E);
+ std::string buffer;
+ llvm::raw_string_ostream stream(buffer);
+ By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+ return StringRef(stream.str()).trim().str();
+}
+
std::string removeSpace(std::string s) {
s.erase(std::remove_if(s.begin(), s.end(),
[](char c) { return std::isspace(c); }),
@@ -100,10 +123,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -134,41 +161,111 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = buildASTFromCode(
+ auto AST = buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *x; Foo * = x; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
- AST = buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }",
- {"-fno-delayed-template-parsing"});
+ AST =
+ buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }"
+ "template void g() { T *p; p->mf(); }",
+ {"-fno-delayed-template-parsing"});
Results =
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added a comment.
TDD, thats ok ;)
Am 22.09.2018 um 19:37 schrieb Shuai Wang via Phabricator:
> shuaiwang added inline comments.
>
>
> Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
>
> EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
>
> +
> + AST = tooling::buildASTFromCode(
>
>
>
> JonasToth wrote:
>
>> shuaiwang wrote:
>>
>>> JonasToth wrote:
>>>
JonasToth wrote:
> I feel that there a multiple tests missing:
>
> - multiple levels of pointers `int ***`, `int * const *`
> - pointers to references `int &*`
> - references to pointers `int *&`
> - ensure that having a const pointer does no influence the pointee
> analysis `int * const p = *p = 42;`
> - a class with `operator*` + `operator->` const/non-const and the
> analysis for pointers to that class
> - pointer returned from a function
> - non-const reference returned ``` int& foo(int *p) { return *p; } ```
for the multi-level pointer mutation: it would be enough to test, that the
second layer is analyzed properly, and that the `int * >const< *` would be
detected.
>>>
>>> Added except for:
>>>
>>> - Anything that requires following a dereference, we need
>>> `findPointeeDerefMutation` for that.
>>> - Pointer to a class with `operator*` + `operator->`, I think those two
>>> operators doesn't matter, there's no way to accidentally invoke them from a
>>> pointer.
>>
>> But we want to analyze smart pointers in the future as well, not? It would
>> be good to already prepare that in the testing department.
>> Or would the nature of `operator*` already make that happen magically?
>
> Yes we'll handle smart pointers, and we'll handle that in
> `findPointeeDerefMutation`, basically it'll look like:
>
> if (native pointer && derefed with *) findMutation(deref expr)
> if (smart pointer && called operator*) findMutation(operator call expr)
> if (smart pointer && called operator->) findPointeeMutation(operator call
> expr)
>
>
> I think it would be more clear if we can match the implementation progress
> with unit test cases as that shows what kind of cases starts to be supported
> by the change.
>
> Repository:
>
> rC Clang
>
> https://reviews.llvm.org/D52219
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
JonasToth wrote:
> shuaiwang wrote:
> > JonasToth wrote:
> > > JonasToth wrote:
> > > > I feel that there a multiple tests missing:
> > > >
> > > > - multiple levels of pointers `int ***`, `int * const *`
> > > > - pointers to references `int &*`
> > > > - references to pointers `int *&`
> > > > - ensure that having a const pointer does no influence the pointee
> > > > analysis `int * const p = *p = 42;`
> > > > - a class with `operator*` + `operator->` const/non-const and the
> > > > analysis for pointers to that class
> > > > - pointer returned from a function
> > > > - non-const reference returned
> > > > ```
> > > > int& foo(int *p) {
> > > > return *p;
> > > > }
> > > > ```
> > > >
> > > for the multi-level pointer mutation: it would be enough to test, that
> > > the second layer is analyzed properly, and that the `int * >const< *`
> > > would be detected.
> > Added except for:
> > - Anything that requires following a dereference, we need
> > `findPointeeDerefMutation` for that.
> > - Pointer to a class with `operator*` + `operator->`, I think those two
> > operators doesn't matter, there's no way to accidentally invoke them from a
> > pointer.
> >
> But we want to analyze smart pointers in the future as well, not? It would be
> good to already prepare that in the testing department.
> Or would the nature of `operator*` already make that happen magically?
Yes we'll handle smart pointers, and we'll handle that in
`findPointeeDerefMutation`, basically it'll look like:
```
if (native pointer && derefed with *) findMutation(deref expr)
if (smart pointer && called operator*) findMutation(operator call expr)
if (smart pointer && called operator->) findPointeeMutation(operator call expr)
```
I think it would be more clear if we can match the implementation progress with
unit test cases as that shows what kind of cases starts to be supported by the
change.
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
shuaiwang wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > I feel that there a multiple tests missing:
> > >
> > > - multiple levels of pointers `int ***`, `int * const *`
> > > - pointers to references `int &*`
> > > - references to pointers `int *&`
> > > - ensure that having a const pointer does no influence the pointee
> > > analysis `int * const p = *p = 42;`
> > > - a class with `operator*` + `operator->` const/non-const and the
> > > analysis for pointers to that class
> > > - pointer returned from a function
> > > - non-const reference returned
> > > ```
> > > int& foo(int *p) {
> > > return *p;
> > > }
> > > ```
> > >
> > for the multi-level pointer mutation: it would be enough to test, that the
> > second layer is analyzed properly, and that the `int * >const< *` would be
> > detected.
> Added except for:
> - Anything that requires following a dereference, we need
> `findPointeeDerefMutation` for that.
> - Pointer to a class with `operator*` + `operator->`, I think those two
> operators doesn't matter, there's no way to accidentally invoke them from a
> pointer.
>
But we want to analyze smart pointers in the future as well, not? It would be
good to already prepare that in the testing department.
Or would the nature of `operator*` already make that happen magically?
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang updated this revision to Diff 166595.
shuaiwang marked 5 inline comments as done.
shuaiwang added a comment.
Addresses review comments.
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -52,11 +52,21 @@
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
const auto *const S = selectFirst("stmt", Results);
const auto *const E = selectFirst("expr", Results);
+ assert(S && E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ assert(S && E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ EXPECT_TRUE(isMutated(Results, AST));
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
@@ -71,6 +81,19 @@
return Chain;
}
+std::string pointeeMutatedBy(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ EXPECT_TRUE(isPointeeMutated(Results, AST));
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+ const Stmt *By = Analyzer.findPointeeMutation(E);
+ std::string buffer;
+ llvm::raw_string_ostream stream(buffer);
+ By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
+ return StringRef(stream.str()).trim().str();
+}
+
std::string removeSpace(std::string s) {
s.erase(std::remove_if(s.begin(), s.end(),
[](char c) { return std::isspace(c); }),
@@ -100,10 +123,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -134,41 +161,111 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = buildASTFromCode(
+ auto AST = buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *x; Foo * = x; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_EQ(pointeeMutatedBy(Results, AST.get()), "p->mf()");
- AST = buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }",
- {"-fno-delayed-template-parsing"});
+ AST =
+ buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }"
+ "template void g() { T *p; p->mf(); }",
+ {"-fno-delayed-template-parsing"});
Results =
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang marked 10 inline comments as done.
shuaiwang added inline comments.
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:86
+// - Pointer to non-const
+// - Pointer-like type with `operator*` returning non-const reference
+bool isPointeeMutable(const Expr *Exp, const ASTContext ) {
Szelethus wrote:
> Didn't you mean these to be doxygen comments?
I feel these don't need to be doxygen comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
JonasToth wrote:
> JonasToth wrote:
> > I feel that there a multiple tests missing:
> >
> > - multiple levels of pointers `int ***`, `int * const *`
> > - pointers to references `int &*`
> > - references to pointers `int *&`
> > - ensure that having a const pointer does no influence the pointee analysis
> > `int * const p = *p = 42;`
> > - a class with `operator*` + `operator->` const/non-const and the analysis
> > for pointers to that class
> > - pointer returned from a function
> > - non-const reference returned
> > ```
> > int& foo(int *p) {
> > return *p;
> > }
> > ```
> >
> for the multi-level pointer mutation: it would be enough to test, that the
> second layer is analyzed properly, and that the `int * >const< *` would be
> detected.
Added except for:
- Anything that requires following a dereference, we need
`findPointeeDerefMutation` for that.
- Pointer to a class with `operator*` + `operator->`, I think those two
operators doesn't matter, there's no way to accidentally invoke them from a
pointer.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:686
+ AST = tooling::buildASTFromCode(
+ "void g(const int*); void f() { const int x[2] = {}; g(x); }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
JonasToth wrote:
> Please add a mutating example for array access through a pointer (as its very
> common in C-style code).
This also need `findPointeeDerefMutation`.
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
shuaiwang wrote:
> JonasToth wrote:
> > shuaiwang wrote:
> > > JonasToth wrote:
> > > > That doesn't look like a super idea. It is super hidden that variable
> > > > 'p*' will be analyzed for pointee mutation and others aren't.
> > > > Especially in 12 months and when someone else wants to change
> > > > something, this will be the source of headaches.
> > > >
> > > > Don't you think there could be a better way of doing this switch?
> > > Yeah... agree :)
> > > But how about let's keep it this way just for now, and I'll pause the
> > > work on supporting pointee analysis and fix it properly but in a separate
> > > diff following this one?
> > >
> > > I've been thinking about this and also did some prototyping, and here are
> > > my thoughts:
> > > In order to properly fix this we need to change the interface of
> > > `ExprMutationAnalyzer` to not simply return a `Stmt` but instead return a
> > > `MutationTrace` with additional metadata. This wasn't needed before
> > > because we can reconstruct a mutation chain by continuously calling
> > > `findMutation`, and that works well for cases like "int x; int& r0 = x;
> > > int& r1 = r0; r1 = 123;". But now that pointers come into play, and we
> > > need to handle cases like "int *p; int *p2 = p; int& r = *p2; r = 123;",
> > > and in order to reconstruct the mutation chain, we need to do
> > > `findPointeeMutation(p)` -> `findPointeeMutation(p2)` ->
> > > `findMutation(r)`, notice that we need to switch from calling
> > > `findPointeeMutation` to calling `findMutation`. However we don't know
> > > where the switch should happen simply based on what's returned from
> > > `findPointeeMutation`, we need additional metadata for that.
> > >
> > > That interface change will unavoidably affect how memoization is done so
> > > we need to change memoization as well.
> > >
> > > Now since we need to change both the interface and memoization anyway, we
> > > might as well design them properly so that multiple-layers of pointers
> > > can be supported nicely. I think we can end up with something like this:
> > > ```
> > > class ExprMutationAnalyzer {
> > > public:
> > > struct MutationTrace {
> > > const Stmt *Value;
> > > const MutationTrace *Next;
> > > // Other metadata
> > > };
> > >
> > > // Finds whether any mutative operations are performed on the given
> > > expression after dereferencing `DerefLayers` times.
> > > const MutationTrace *findMutation(const Expr *, int DerefLayers = 0);
> > > const MutationTrace *findMutation(const Decl *, int DerefLayers = 0);
> > > };
> > > ```
> > > This involves quite some refactoring, and doing this refactoring before
> > > this diff and later rebasing seems quite some trouble that I'd like to
> > > avoid.
> > >
> > > Let me know what do you think :)
> > Is the second part of your answer part to adressing this testing issue?
> > Given the whole Analyzer is WIP it is ok to postpone the testing change to
> > later for me. But I feel that this is a quality issue that more experience
> > clang develops should comment on because it still lands in trunk. Are you
> > aware of other patches then clang-tidy that rely on EMA?
> >
> > Regarding you second part (its related to multi-layer pointer mutation?!):
> > Having a dependency-graph that follows the general data flow _with_
> > mutation annotations would be nice to have. There is probably even
> > something in clang (e.g. `CFG` is probably similar in idea, just with all
> > execution paths), but I don't know enough to properly judge here.
> >
> > In my opinion it is ok, to not do the refactoring now and just support one
> > layer of pointer indirection. Especially in C++ there is usually no need
> > for multi-layer pointers but more a relict from C-style.
> > C on the other hand relies on that.
> > Designing EMA new for the more generalized functionality (if necessary)
> > would be of course great, but again: Better analyzer ppl should be involved
> > then me, even though I would still be very interested to help :)
> The second part is more of trying to explain why I'd prefer to keep the test
> this way just for this diff.
> Basically we need a refactoring that makes EMA returns MutationTrace in order
> to fix this test util here.
>
> But thinking more about this, maybe we don't need that (for now), this util
> is to help restore a mutation chain, and since we don't support a chain of
> mutation for pointers yet we can have a simpler version of `pointeeMutatedBy`
> that doesn't support chaining.
>
> I mentioned multi-layer pointer mutation because _if_ we were to do a
> refactoring, we'd better just do it right and take possible features needed
> in
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
JonasToth wrote:
> shuaiwang wrote:
> > JonasToth wrote:
> > > That doesn't look like a super idea. It is super hidden that variable
> > > 'p*' will be analyzed for pointee mutation and others aren't. Especially
> > > in 12 months and when someone else wants to change something, this will
> > > be the source of headaches.
> > >
> > > Don't you think there could be a better way of doing this switch?
> > Yeah... agree :)
> > But how about let's keep it this way just for now, and I'll pause the work
> > on supporting pointee analysis and fix it properly but in a separate diff
> > following this one?
> >
> > I've been thinking about this and also did some prototyping, and here are
> > my thoughts:
> > In order to properly fix this we need to change the interface of
> > `ExprMutationAnalyzer` to not simply return a `Stmt` but instead return a
> > `MutationTrace` with additional metadata. This wasn't needed before because
> > we can reconstruct a mutation chain by continuously calling `findMutation`,
> > and that works well for cases like "int x; int& r0 = x; int& r1 = r0; r1 =
> > 123;". But now that pointers come into play, and we need to handle cases
> > like "int *p; int *p2 = p; int& r = *p2; r = 123;", and in order to
> > reconstruct the mutation chain, we need to do `findPointeeMutation(p)` ->
> > `findPointeeMutation(p2)` -> `findMutation(r)`, notice that we need to
> > switch from calling `findPointeeMutation` to calling `findMutation`.
> > However we don't know where the switch should happen simply based on what's
> > returned from `findPointeeMutation`, we need additional metadata for that.
> >
> > That interface change will unavoidably affect how memoization is done so we
> > need to change memoization as well.
> >
> > Now since we need to change both the interface and memoization anyway, we
> > might as well design them properly so that multiple-layers of pointers can
> > be supported nicely. I think we can end up with something like this:
> > ```
> > class ExprMutationAnalyzer {
> > public:
> > struct MutationTrace {
> > const Stmt *Value;
> > const MutationTrace *Next;
> > // Other metadata
> > };
> >
> > // Finds whether any mutative operations are performed on the given
> > expression after dereferencing `DerefLayers` times.
> > const MutationTrace *findMutation(const Expr *, int DerefLayers = 0);
> > const MutationTrace *findMutation(const Decl *, int DerefLayers = 0);
> > };
> > ```
> > This involves quite some refactoring, and doing this refactoring before
> > this diff and later rebasing seems quite some trouble that I'd like to
> > avoid.
> >
> > Let me know what do you think :)
> Is the second part of your answer part to adressing this testing issue?
> Given the whole Analyzer is WIP it is ok to postpone the testing change to
> later for me. But I feel that this is a quality issue that more experience
> clang develops should comment on because it still lands in trunk. Are you
> aware of other patches then clang-tidy that rely on EMA?
>
> Regarding you second part (its related to multi-layer pointer mutation?!):
> Having a dependency-graph that follows the general data flow _with_ mutation
> annotations would be nice to have. There is probably even something in clang
> (e.g. `CFG` is probably similar in idea, just with all execution paths), but
> I don't know enough to properly judge here.
>
> In my opinion it is ok, to not do the refactoring now and just support one
> layer of pointer indirection. Especially in C++ there is usually no need for
> multi-layer pointers but more a relict from C-style.
> C on the other hand relies on that.
> Designing EMA new for the more generalized functionality (if necessary) would
> be of course great, but again: Better analyzer ppl should be involved then
> me, even though I would still be very interested to help :)
The second part is more of trying to explain why I'd prefer to keep the test
this way just for this diff.
Basically we need a refactoring that makes EMA returns MutationTrace in order
to fix this test util here.
But thinking more about this, maybe we don't need that (for now), this util is
to help restore a mutation chain, and since we don't support a chain of
mutation for pointers yet we can have a simpler version of `pointeeMutatedBy`
that doesn't support chaining.
I mentioned multi-layer pointer mutation because _if_ we were to do a
refactoring, we'd better just do it right and take possible features needed in
the future into account. But the refactoring itself is motivated by the need of
fixing this testing util (and making the return value from `findMutation`
generally more useful.)
BTW by
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
shuaiwang wrote:
> JonasToth wrote:
> > That doesn't look like a super idea. It is super hidden that variable 'p*'
> > will be analyzed for pointee mutation and others aren't. Especially in 12
> > months and when someone else wants to change something, this will be the
> > source of headaches.
> >
> > Don't you think there could be a better way of doing this switch?
> Yeah... agree :)
> But how about let's keep it this way just for now, and I'll pause the work on
> supporting pointee analysis and fix it properly but in a separate diff
> following this one?
>
> I've been thinking about this and also did some prototyping, and here are my
> thoughts:
> In order to properly fix this we need to change the interface of
> `ExprMutationAnalyzer` to not simply return a `Stmt` but instead return a
> `MutationTrace` with additional metadata. This wasn't needed before because
> we can reconstruct a mutation chain by continuously calling `findMutation`,
> and that works well for cases like "int x; int& r0 = x; int& r1 = r0; r1 =
> 123;". But now that pointers come into play, and we need to handle cases like
> "int *p; int *p2 = p; int& r = *p2; r = 123;", and in order to reconstruct
> the mutation chain, we need to do `findPointeeMutation(p)` ->
> `findPointeeMutation(p2)` -> `findMutation(r)`, notice that we need to switch
> from calling `findPointeeMutation` to calling `findMutation`. However we
> don't know where the switch should happen simply based on what's returned
> from `findPointeeMutation`, we need additional metadata for that.
>
> That interface change will unavoidably affect how memoization is done so we
> need to change memoization as well.
>
> Now since we need to change both the interface and memoization anyway, we
> might as well design them properly so that multiple-layers of pointers can be
> supported nicely. I think we can end up with something like this:
> ```
> class ExprMutationAnalyzer {
> public:
> struct MutationTrace {
> const Stmt *Value;
> const MutationTrace *Next;
> // Other metadata
> };
>
> // Finds whether any mutative operations are performed on the given
> expression after dereferencing `DerefLayers` times.
> const MutationTrace *findMutation(const Expr *, int DerefLayers = 0);
> const MutationTrace *findMutation(const Decl *, int DerefLayers = 0);
> };
> ```
> This involves quite some refactoring, and doing this refactoring before this
> diff and later rebasing seems quite some trouble that I'd like to avoid.
>
> Let me know what do you think :)
Is the second part of your answer part to adressing this testing issue?
Given the whole Analyzer is WIP it is ok to postpone the testing change to
later for me. But I feel that this is a quality issue that more experience
clang develops should comment on because it still lands in trunk. Are you aware
of other patches then clang-tidy that rely on EMA?
Regarding you second part (its related to multi-layer pointer mutation?!):
Having a dependency-graph that follows the general data flow _with_ mutation
annotations would be nice to have. There is probably even something in clang
(e.g. `CFG` is probably similar in idea, just with all execution paths), but I
don't know enough to properly judge here.
In my opinion it is ok, to not do the refactoring now and just support one
layer of pointer indirection. Especially in C++ there is usually no need for
multi-layer pointers but more a relict from C-style.
C on the other hand relies on that.
Designing EMA new for the more generalized functionality (if necessary) would
be of course great, but again: Better analyzer ppl should be involved then me,
even though I would still be very interested to help :)
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
JonasToth wrote:
> I feel that there a multiple tests missing:
>
> - multiple levels of pointers `int ***`, `int * const *`
> - pointers to references `int &*`
> - references to pointers `int *&`
> - ensure that having a const pointer does no influence the pointee analysis
> `int * const p = *p = 42;`
> - a class with `operator*` + `operator->` const/non-const and the analysis
> for pointers to that class
> - pointer returned from a function
> - non-const reference returned
> ```
> int& foo(int *p) {
> return *p;
> }
> ```
>
for the multi-level pointer mutation: it would be enough to test, that the
second layer is analyzed properly, and that the `int * >const< *` would be
detected.
Repository:
rC Clang
https://reviews.llvm.org/D52219
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang marked 5 inline comments as done.
shuaiwang added inline comments.
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
JonasToth wrote:
> That doesn't look like a super idea. It is super hidden that variable 'p*'
> will be analyzed for pointee mutation and others aren't. Especially in 12
> months and when someone else wants to change something, this will be the
> source of headaches.
>
> Don't you think there could be a better way of doing this switch?
Yeah... agree :)
But how about let's keep it this way just for now, and I'll pause the work on
supporting pointee analysis and fix it properly but in a separate diff
following this one?
I've been thinking about this and also did some prototyping, and here are my
thoughts:
In order to properly fix this we need to change the interface of
`ExprMutationAnalyzer` to not simply return a `Stmt` but instead return a
`MutationTrace` with additional metadata. This wasn't needed before because we
can reconstruct a mutation chain by continuously calling `findMutation`, and
that works well for cases like "int x; int& r0 = x; int& r1 = r0; r1 = 123;".
But now that pointers come into play, and we need to handle cases like "int *p;
int *p2 = p; int& r = *p2; r = 123;", and in order to reconstruct the mutation
chain, we need to do `findPointeeMutation(p)` -> `findPointeeMutation(p2)` ->
`findMutation(r)`, notice that we need to switch from calling
`findPointeeMutation` to calling `findMutation`. However we don't know where
the switch should happen simply based on what's returned from
`findPointeeMutation`, we need additional metadata for that.
That interface change will unavoidably affect how memoization is done so we
need to change memoization as well.
Now since we need to change both the interface and memoization anyway, we might
as well design them properly so that multiple-layers of pointers can be
supported nicely. I think we can end up with something like this:
```
class ExprMutationAnalyzer {
public:
struct MutationTrace {
const Stmt *Value;
const MutationTrace *Next;
// Other metadata
};
// Finds whether any mutative operations are performed on the given
expression after dereferencing `DerefLayers` times.
const MutationTrace *findMutation(const Expr *, int DerefLayers = 0);
const MutationTrace *findMutation(const Decl *, int DerefLayers = 0);
};
```
This involves quite some refactoring, and doing this refactoring before this
diff and later rebasing seems quite some trouble that I'd like to avoid.
Let me know what do you think :)
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
Szelethus added inline comments.
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:86
+// - Pointer to non-const
+// - Pointer-like type with `operator*` returning non-const reference
+bool isPointeeMutable(const Expr *Exp, const ASTContext ) {
Didn't you mean these to be doxygen comments?
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added inline comments.
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:481
+ const auto AsArg =
+ anyOf(callExpr(hasAnyArgument(equalsNode(Exp))),
+cxxConstructExpr(hasAnyArgument(equalsNode(Exp))),
shuaiwang wrote:
> JonasToth wrote:
> > shouldn't be the constness of the argument considered here?
> We need that for non-pointee version, but not for pointee version, for
> example:
> ```
> void g1(int * const);
>
> void f1() {
> int *x;
> g1(x); // <-- x is passed to `g1`, we consider that as a mutation, the
> argument type do have a top-level const
> }
>
> void g2(const int *);
>
> void f2() {
> int *x;
> g2(x); // <-- declRefExp(to(x)) is NOT directly passed to `g2`, there's a
> layer a ImplicitCastExpr in between, and after the implicit cast, the
> type of the expression becomes "const int *" instead of just "int*", so it'll
> fail the `isPointeeMutable` check at the beginning of
> `findPointeeDirectMutation`
> }
> ```
>
> In summary, we rely on:
> - Checking whether pointee is actually mutable at the beginning
> - Carefully handling casts by not trivially ignoring them unless absolutely
> safe
I see. That makes sense, thanks for clarification :)
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang updated this revision to Diff 166068.
shuaiwang marked 7 inline comments as done.
shuaiwang added a comment.
[WIP] Addressed some of review comments.
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -52,16 +52,34 @@
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
const auto *const S = selectFirst("stmt", Results);
const auto *const E = selectFirst("expr", Results);
+ assert(S && E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ assert(S && E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ const Stmt *(ExprMutationAnalyzer::*MutationFinder)(const Expr *) =
+ ::findMutation;
+ const Stmt *(ExprMutationAnalyzer::*PointeeMutationFinder)(const Expr *) =
+ ::findPointeeMutation;
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
-const Stmt *By = Analyzer.findMutation(E);
+auto Finder = MutationFinder;
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
+}
+const Stmt *By = (Analyzer.*Finder)(E);
std::string buffer;
llvm::raw_string_ostream stream(buffer);
By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
@@ -100,10 +118,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -134,41 +156,104 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = buildASTFromCode(
+ auto AST = buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_TRUE(isMutated(Results, AST.get()));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
- AST = buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }",
- {"-fno-delayed-template-parsing"});
+ AST =
+ buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }"
+ "template void g() { T *p; p->mf(); }",
+ {"-fno-delayed-template-parsing"});
Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
AST = buildASTFromCodeWithArgs(
"template
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang added a comment.
In https://reviews.llvm.org/D52219#1238423, @JonasToth wrote:
> Do you think it would be possible to the analysis for `>const?< int
> ***`-cases? (recursively checking through the pointer levels)
I think that should be possible, will do after single-layer pointee analysis is
done. I believe we can build multi-layer analysis based on much of the
single-layer analysis.
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:198
const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // `Exp` can be *directly* mutated if the type of `Exp` is not const.
+ // Bail out early otherwise.
JonasToth wrote:
> Just to be sure that i understand:
> the changes here are more performance optimizations then directly related to
> detect pointee mutations?
Yes :)
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:481
+ const auto AsArg =
+ anyOf(callExpr(hasAnyArgument(equalsNode(Exp))),
+cxxConstructExpr(hasAnyArgument(equalsNode(Exp))),
JonasToth wrote:
> shouldn't be the constness of the argument considered here?
We need that for non-pointee version, but not for pointee version, for example:
```
void g1(int * const);
void f1() {
int *x;
g1(x); // <-- x is passed to `g1`, we consider that as a mutation, the
argument type do have a top-level const
}
void g2(const int *);
void f2() {
int *x;
g2(x); // <-- declRefExp(to(x)) is NOT directly passed to `g2`, there's a
layer a ImplicitCastExpr in between, and after the implicit cast, the
type of the expression becomes "const int *" instead of just "int*", so it'll
fail the `isPointeeMutable` check at the beginning of
`findPointeeDirectMutation`
}
```
In summary, we rely on:
- Checking whether pointee is actually mutable at the beginning
- Carefully handling casts by not trivially ignoring them unless absolutely safe
Repository:
rC Clang
https://reviews.llvm.org/D52219
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang updated this revision to Diff 166065.
shuaiwang added a comment.
Rebase
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -52,16 +52,34 @@
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
const auto *const S = selectFirst("stmt", Results);
const auto *const E = selectFirst("expr", Results);
+ assert(E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ assert(E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ const Stmt *(ExprMutationAnalyzer::*MutationFinder)(const Expr *) =
+ ::findMutation;
+ const Stmt *(ExprMutationAnalyzer::*PointeeMutationFinder)(const Expr *) =
+ ::findPointeeMutation;
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
-const Stmt *By = Analyzer.findMutation(E);
+auto Finder = MutationFinder;
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
+}
+const Stmt *By = (Analyzer.*Finder)(E);
std::string buffer;
llvm::raw_string_ostream stream(buffer);
By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
@@ -100,10 +118,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -134,41 +156,104 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = buildASTFromCode(
+ auto AST = buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_TRUE(isMutated(Results, AST.get()));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
- AST = buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }",
- {"-fno-delayed-template-parsing"});
+ AST =
+ buildASTFromCodeWithArgs("template void f() { T x; x.mf(); }"
+ "template void g() { T *p; p->mf(); }",
+ {"-fno-delayed-template-parsing"});
Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
AST = buildASTFromCodeWithArgs(
"template struct X;"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
JonasToth added a comment.
Looks like a good start! I think getting the pointers right will be most
difficult, because of the multiple levels of indirection they allow. Do you
think it would be possible to the analysis for `>const?< int ***`-cases?
(recursively checking through the pointer levels)
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:198
const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
+ // `Exp` can be *directly* mutated if the type of `Exp` is not const.
+ // Bail out early otherwise.
Just to be sure that i understand:
the changes here are more performance optimizations then directly related to
detect pointee mutations?
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:440
+ return nullptr;
+ } else if (const auto *RT = Exp->getType()->getAs()) {
+if (const CXXRecordDecl *RecordDecl = RT->getAsCXXRecordDecl()) {
no `else` after return. this makes the short circuit logic clearer
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:442
+if (const CXXRecordDecl *RecordDecl = RT->getAsCXXRecordDecl()) {
+ const bool ExpIsConst = Exp->getType().isConstant(Context);
+ if (llvm::any_of(
values are not marked as const by the llvm code guideline
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:459
+ }
+ const bool ExpIsPointer = Exp->getType()->getAs();
+
implicit conversion from pointer to bool? Please make that better visible and
please dont use const on values.
Not sure for the matchers, I have seen both, but they should be treated as
values as well i think.
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:463
+ // A member function is assumed to be non-const when it is unresolved.
+ // We don't handle pointer-like type here as non-const member function of
+ // pointee can't be directly invoked without dereferencing the pointer-like
Please make that sentence clearer, i could not understand it (only interpret :D)
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:481
+ const auto AsArg =
+ anyOf(callExpr(hasAnyArgument(equalsNode(Exp))),
+cxxConstructExpr(hasAnyArgument(equalsNode(Exp))),
shouldn't be the constness of the argument considered here?
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:488
+
+ // Returned.
+ const auto AsReturnValue = returnStmt(hasReturnValue(equalsNode(Exp)));
Either remove the comment or make it a full sentence please. I think the
variable naming is clear enough to elide
Comment at: lib/Analysis/ExprMutationAnalyzer.cpp:500
+const Stmt *ExprMutationAnalyzer::findPointeeCastMutation(const Expr *Exp) {
+ // Only handling LValueToRValue for now for easier unit testing during
+ // implementation.
Please add a todo to ensure the missing casts are not forgotten
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:43
const auto *const E = selectFirst("expr", Results);
+ assert(E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
maybe even `assert(E && S)`?
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:67
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
That doesn't look like a super idea. It is super hidden that variable 'p*' will
be analyzed for pointee mutation and others aren't. Especially in 12 months and
when someone else wants to change something, this will be the source of
headaches.
Don't you think there could be a better way of doing this switch?
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:156
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
I feel that there a multiple tests missing:
- multiple levels of pointers `int ***`, `int * const *`
- pointers to references `int &*`
- references to pointers `int *&`
- ensure that having a const pointer does no influence the pointee analysis
`int * const p = *p = 42;`
- a class with `operator*` + `operator->` const/non-const and the analysis for
pointers to that class
- pointer returned from a function
- non-const reference returned
```
int& foo(int *p) {
return *p;
}
```
Comment at: unittests/Analysis/ExprMutationAnalyzerTest.cpp:175
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
Could you please add the `EXPECT_FALSE(isMutated()) &&
EXPECT_TRUE(isPointeeMutated()`?
Maybe a short helper for that like `isOnlyPointeeMutated()` would be nice.
Here and the other cases as well
[PATCH] D52219: [analyzer] (1/n) Support pointee mutation analysis in ExprMutationAnalyzer.
shuaiwang created this revision.
shuaiwang added reviewers: JonasToth, aaron.ballman.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho, a.sidorin,
szepet, xazax.hun.
Herald added a reviewer: george.karpenkov.
We handle pointee mutation for native pointers & pointer-like types
(loosely defined as having an `operator*` returning non-const reference)
This diff alone just implemented the cases where pointee of an `Exp` is
*directly* mutated (e.g. invoking non-const member function.)
The most trivial class of casts is handled as well for easier unit
testing, `findPointeeCastMutation` is not done yet.
Planned future work:
- `findPointeeDerefMutation`: `Exp` is first dereferenced, and then mutated.
- `findPointee{Member,Array}Mutation`: member (or array element) of pointee is
accessed and then mutated.
- `findPointeeArithmeticMutation`: handling pointer arithmetic
- `findPointeeAssignmentMutation`: pointer is assigned to another var and then
mutated.
Repository:
rC Clang
https://reviews.llvm.org/D52219
Files:
include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
lib/Analysis/ExprMutationAnalyzer.cpp
unittests/Analysis/ExprMutationAnalyzerTest.cpp
Index: unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -40,16 +40,34 @@
bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
const auto *const S = selectFirst("stmt", Results);
const auto *const E = selectFirst("expr", Results);
+ assert(E);
return ExprMutationAnalyzer(*S, AST->getASTContext()).isMutated(E);
}
+bool isPointeeMutated(const SmallVectorImpl ,
+ ASTUnit *AST) {
+ const auto *const S = selectFirst("stmt", Results);
+ const auto *const E = selectFirst("expr", Results);
+ assert(E);
+ return ExprMutationAnalyzer(*S, AST->getASTContext()).isPointeeMutated(E);
+}
+
SmallVector
mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+ const Stmt *(ExprMutationAnalyzer::*MutationFinder)(const Expr *) =
+ ::findMutation;
+ const Stmt *(ExprMutationAnalyzer::*PointeeMutationFinder)(const Expr *) =
+ ::findPointeeMutation;
const auto *const S = selectFirst("stmt", Results);
SmallVector Chain;
ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
-const Stmt *By = Analyzer.findMutation(E);
+auto Finder = MutationFinder;
+if (const auto *DRE = dyn_cast(E)) {
+ if (DRE->getNameInfo().getAsString()[0] == 'p')
+Finder = PointeeMutationFinder;
+}
+const Stmt *By = (Analyzer.*Finder)(E);
std::string buffer;
llvm::raw_string_ostream stream(buffer);
By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
@@ -88,10 +106,14 @@
} // namespace
TEST(ExprMutationAnalyzerTest, Trivial) {
- const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
- const auto Results =
+ auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_FALSE(isMutated(Results, AST.get()));
+
+ AST = tooling::buildASTFromCode("void f() { const int x = 0; x; }");
+ Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
}
class AssignmentTest : public ::testing::TestWithParam {};
@@ -124,42 +146,104 @@
Values("++x", "--x", "x++", "x--"), );
TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
- const auto AST = tooling::buildASTFromCode(
+ auto AST = tooling::buildASTFromCode(
"void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
- const auto Results =
+ auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+ EXPECT_TRUE(isMutated(Results, AST.get()));
+ EXPECT_FALSE(isPointeeMutated(Results, AST.get()));
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+ AST = tooling::buildASTFromCode(
+ "void f() { struct Foo { void mf(); }; Foo *p; p->mf(); }");
+ Results = match(withEnclosingCompound(declRefTo("p")), AST->getASTContext());
+ EXPECT_FALSE(isMutated(Results, AST.get()));
+ EXPECT_TRUE(isPointeeMutated(Results, AST.get()));
+ EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("p->mf()"));
}
TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
auto AST = tooling::buildASTFromCodeWithArgs(
"struct X { template void mf(); };"
- "template void f() { X x; x.mf(); }",
+ "template void f() { X x; x.mf(); }"
+ "template void g() { X *p; p->mf(); }",
{"-fno-delayed-template-parsing"});
auto Results =
match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
