[clang][dataflow] Re-land: Retrieve members from accessors called usi… #74336
Conversation
…ng member… (llvm#73978) … pointers. This initially landed with a broken test due to a mid-air collision with a new requirement for Environment initialization before field modeling. Have added that initialization in the test. From first landing: getMethodDecl does not handle pointers to members and returns nullptr for them. getMethodDecl contains a decade-plus-old FIXME to handle pointers to members, but two approaches I looked at for fixing it are more invasive or complex than simply swapping to getCalleeDecl. The first, have getMethodDecl call getCalleeDecl, creates a large tree of const-ness mismatches due to getMethodDecl returning a non-const value while being a const member function and getCalleeDecl only being a const member function when it returns a const value. The second, implementing an AST walk to match how CXXMemberCallExpr::getImplicitObjectArgument grabs the LHS of the binary operator, is basically reimplementing Expr::getReferencedDeclOfCallee, which is used by Expr::getCalleeDecl. We don't need another copy of that code.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis Author: Samira Bazuzi (bazuzi) Changes…ng member pointers. This initially landed with a broken test due to a mid-air collision with a new requirement for Environment initialization before field modeling. Have added that initialization in the test. From first landing: getMethodDecl does not handle pointers to members and returns nullptr for them. getMethodDecl contains a decade-plus-old FIXME to handle pointers to members, but two approaches I looked at for fixing it are more invasive or complex than simply swapping to getCalleeDecl. The first, have getMethodDecl call getCalleeDecl, creates a large tree of const-ness mismatches due to getMethodDecl returning a non-const value while being a const member function and getCalleeDecl only being a const member function when it returns a const value. The second, implementing an AST walk to match how Full diff: https://github.com/llvm/llvm-project/pull/74336.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 042402a129d10..b98037b736452 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -300,9 +300,12 @@ static void insertIfFunction(const Decl &D,
}
static MemberExpr *getMemberForAccessor(const CXXMemberCallExpr &C) {
- if (!C.getMethodDecl())
+ // Use getCalleeDecl instead of getMethodDecl in order to handle
+ // pointer-to-member calls.
+ const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(C.getCalleeDecl());
+ if (!MethodDecl)
return nullptr;
- auto *Body = dyn_cast_or_null<CompoundStmt>(C.getMethodDecl()->getBody());
+ auto *Body = dyn_cast_or_null<CompoundStmt>(MethodDecl->getBody());
if (!Body || Body->size() != 1)
return nullptr;
if (auto *RS = dyn_cast<ReturnStmt>(*Body->body_begin()))
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 3569b0eac7005..003434a58b107 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -25,6 +25,7 @@ namespace {
using namespace clang;
using namespace dataflow;
using ::clang::dataflow::test::getFieldValue;
+using ::testing::Contains;
using ::testing::IsNull;
using ::testing::NotNull;
@@ -311,6 +312,57 @@ TEST_F(EnvironmentTest, InitGlobalVarsConstructor) {
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
+// Pointers to Members are a tricky case of accessor calls, complicated further
+// when using templates where the pointer to the member is a template argument.
+// This is a repro of a failure case seen in the wild.
+TEST_F(EnvironmentTest,
+ ModelMemberForAccessorUsingMethodPointerThroughTemplate) {
+ using namespace ast_matchers;
+
+ std::string Code = R"cc(
+ struct S {
+ int accessor() {return member;}
+
+ int member = 0;
+ };
+
+ template <auto method>
+ int Target(S* S) {
+ return (S->*method)();
+ }
+
+ // We want to analyze the instantiation of Target for the accessor.
+ int Instantiator () {S S; return Target<&S::accessor>(&S); }
+ )cc";
+
+ auto Unit =
+ // C++17 for the simplifying use of auto in the template declaration.
+ tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++17"});
+ auto &Context = Unit->getASTContext();
+
+ ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+ auto Results = match(
+ decl(anyOf(functionDecl(hasName("Target"), isTemplateInstantiation())
+ .bind("target"),
+ fieldDecl(hasName("member")).bind("member"),
+ recordDecl(hasName("S")).bind("struct"))),
+ Context);
+ const auto *Fun = selectFirst<FunctionDecl>("target", Results);
+ const auto *Struct = selectFirst<RecordDecl>("struct", Results);
+ const auto *Member = selectFirst<FieldDecl>("member", Results);
+ ASSERT_THAT(Fun, NotNull());
+ ASSERT_THAT(Struct, NotNull());
+ ASSERT_THAT(Member, NotNull());
+
+ // Verify that `member` is modeled for `S` when we analyze
+ // `Target<&S::accessor>`.
+ Environment Env(DAContext, *Fun);
+ Env.initialize();
+ EXPECT_THAT(DAContext.getModeledFields(QualType(Struct->getTypeForDecl(), 0)),
+ Contains(Member));
+}
+
TEST_F(EnvironmentTest, RefreshRecordValue) {
using namespace ast_matchers;
|
|
@llvm/pr-subscribers-clang Author: Samira Bazuzi (bazuzi) Changes…ng member pointers. This initially landed with a broken test due to a mid-air collision with a new requirement for Environment initialization before field modeling. Have added that initialization in the test. From first landing: getMethodDecl does not handle pointers to members and returns nullptr for them. getMethodDecl contains a decade-plus-old FIXME to handle pointers to members, but two approaches I looked at for fixing it are more invasive or complex than simply swapping to getCalleeDecl. The first, have getMethodDecl call getCalleeDecl, creates a large tree of const-ness mismatches due to getMethodDecl returning a non-const value while being a const member function and getCalleeDecl only being a const member function when it returns a const value. The second, implementing an AST walk to match how Full diff: https://github.com/llvm/llvm-project/pull/74336.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 042402a129d10..b98037b736452 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -300,9 +300,12 @@ static void insertIfFunction(const Decl &D,
}
static MemberExpr *getMemberForAccessor(const CXXMemberCallExpr &C) {
- if (!C.getMethodDecl())
+ // Use getCalleeDecl instead of getMethodDecl in order to handle
+ // pointer-to-member calls.
+ const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(C.getCalleeDecl());
+ if (!MethodDecl)
return nullptr;
- auto *Body = dyn_cast_or_null<CompoundStmt>(C.getMethodDecl()->getBody());
+ auto *Body = dyn_cast_or_null<CompoundStmt>(MethodDecl->getBody());
if (!Body || Body->size() != 1)
return nullptr;
if (auto *RS = dyn_cast<ReturnStmt>(*Body->body_begin()))
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 3569b0eac7005..003434a58b107 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -25,6 +25,7 @@ namespace {
using namespace clang;
using namespace dataflow;
using ::clang::dataflow::test::getFieldValue;
+using ::testing::Contains;
using ::testing::IsNull;
using ::testing::NotNull;
@@ -311,6 +312,57 @@ TEST_F(EnvironmentTest, InitGlobalVarsConstructor) {
EXPECT_THAT(Env.getValue(*Var), NotNull());
}
+// Pointers to Members are a tricky case of accessor calls, complicated further
+// when using templates where the pointer to the member is a template argument.
+// This is a repro of a failure case seen in the wild.
+TEST_F(EnvironmentTest,
+ ModelMemberForAccessorUsingMethodPointerThroughTemplate) {
+ using namespace ast_matchers;
+
+ std::string Code = R"cc(
+ struct S {
+ int accessor() {return member;}
+
+ int member = 0;
+ };
+
+ template <auto method>
+ int Target(S* S) {
+ return (S->*method)();
+ }
+
+ // We want to analyze the instantiation of Target for the accessor.
+ int Instantiator () {S S; return Target<&S::accessor>(&S); }
+ )cc";
+
+ auto Unit =
+ // C++17 for the simplifying use of auto in the template declaration.
+ tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++17"});
+ auto &Context = Unit->getASTContext();
+
+ ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+ auto Results = match(
+ decl(anyOf(functionDecl(hasName("Target"), isTemplateInstantiation())
+ .bind("target"),
+ fieldDecl(hasName("member")).bind("member"),
+ recordDecl(hasName("S")).bind("struct"))),
+ Context);
+ const auto *Fun = selectFirst<FunctionDecl>("target", Results);
+ const auto *Struct = selectFirst<RecordDecl>("struct", Results);
+ const auto *Member = selectFirst<FieldDecl>("member", Results);
+ ASSERT_THAT(Fun, NotNull());
+ ASSERT_THAT(Struct, NotNull());
+ ASSERT_THAT(Member, NotNull());
+
+ // Verify that `member` is modeled for `S` when we analyze
+ // `Target<&S::accessor>`.
+ Environment Env(DAContext, *Fun);
+ Env.initialize();
+ EXPECT_THAT(DAContext.getModeledFields(QualType(Struct->getTypeForDecl(), 0)),
+ Contains(Member));
+}
+
TEST_F(EnvironmentTest, RefreshRecordValue) {
using namespace ast_matchers;
|
|
@martinboehme or @ymand Could you take a look and merge? The only modification from #73978 is the |
…ng member pointers.
This initially landed with a broken test due to a mid-air collision with a new requirement for Environment initialization before field modeling. Have added that initialization in the test.
From first landing:
getMethodDecl does not handle pointers to members and returns nullptr for them. getMethodDecl contains a decade-plus-old FIXME to handle pointers to members, but two approaches I looked at for fixing it are more invasive or complex than simply swapping to getCalleeDecl.
The first, have getMethodDecl call getCalleeDecl, creates a large tree of const-ness mismatches due to getMethodDecl returning a non-const value while being a const member function and getCalleeDecl only being a const member function when it returns a const value.
The second, implementing an AST walk to match how
CXXMemberCallExpr::getImplicitObjectArgument grabs the LHS of the binary operator, is basically reimplementing Expr::getReferencedDeclOfCallee, which is used by Expr::getCalleeDecl. We don't need another copy of that code.