[clang] a3fe9cb - [clang][dataflow] Retrieve members from accessors called using member… (#73978)

[via cfe-commits]

Author: Samira Bazuzi
Date: 2023-12-04T10:10:07+01:00
New Revision: a3fe9cb24da302a40c53d187271e472a6432c4f1

URL: https://github.com/llvm/llvm-project/commit/a3fe9cb24da302a40c53d187271e472a6432c4f1
DIFF: https://github.com/llvm/llvm-project/commit/a3fe9cb24da302a40c53d187271e472a6432c4f1.diff

LOG: [clang][dataflow] Retrieve members from accessors called using member… (#73978)

… 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.

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
    clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp

Removed: 
    


################################################################################
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..c0f59c9de4131 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,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;