-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][dataflow] Retrieve members from accessors called using member… #73978
Conversation
… pointers. getMethodDecl does not handle pointers to members and returns nullptr.
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-analysis Author: Samira Bazuzi (bazuzi) Changes… pointers. 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. Full diff: https://github.com/llvm/llvm-project/pull/73978.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 525ab188b01b8aa..5a37fa0f02f5d21 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -300,9 +300,10 @@ static void insertIfFunction(const Decl &D,
}
static MemberExpr *getMemberForAccessor(const CXXMemberCallExpr &C) {
- if (!C.getMethodDecl())
+ 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 ff6cbec5d20b744..55fb6549621737a 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -306,6 +306,56 @@ 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);
+ EXPECT_THAT(DAContext.getModeledFields(QualType(Struct->getTypeForDecl(), 0)),
+ Contains(Member));
+}
+
TEST_F(EnvironmentTest, RefreshRecordValue) {
using namespace ast_matchers;
|
@ymand Can you review? |
sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -300,9 +300,10 @@ static void insertIfFunction(const Decl &D, | |||
} | |||
|
|||
static MemberExpr *getMemberForAccessor(const CXXMemberCallExpr &C) { | |||
if (!C.getMethodDecl()) | |||
const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(C.getCalleeDecl()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment explaining why we prefer this method over getMethodDecl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
And add comment.
✅ With the latest revision this PR passed the C/C++ code formatter. |
I will eventually get used to not amending and clang-format only touching the modified files.
Can you merge for me, now that I've finally remembered to format everything? |
@bazuzi and @martinboehme this change seems to be causing a test failure on many bots. Can you take a look and revert if you need time to investigate?
|
Hi, on my Release+Asserts build this is causing:
|
Thanks for the heads up, and sorry for the breakage. Reverting. |
…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.
… pointers.
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.