[clang] [clang][dataflow] Model the fields that are accessed via inline accessors (PR #66368)
Kinuko Yasuda via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 15 17:07:22 PDT 2023
https://github.com/kinu updated https://github.com/llvm/llvm-project/pull/66368
>From d311f12fe3ca0d30a40e659236ba7eaccda24a8b Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda <kinuko at chromium.org>
Date: Thu, 14 Sep 2023 12:45:04 +0000
Subject: [PATCH 1/4] [clang][dataflow] Model the fields that are accessed via
inline accessors
So that the values that are accessed via such accessors can be analyzed
as a limited version of context-sensitive analysis.
We can potentially do this only when some option is set, but doing
additional modeling like this won't be expensive and intrusive, so
we do it by default for now.
---
.../FlowSensitive/DataflowEnvironment.cpp | 18 ++++++++
.../Analysis/FlowSensitive/TransferTest.cpp | 44 +++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index e13f880896fc071..713df62e5009336 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -288,6 +288,18 @@ static void insertIfFunction(const Decl &D,
Funcs.insert(FD);
}
+static Expr *getRetValueFromSingleReturnStmtMethod(const CXXMemberCallExpr &C) {
+ auto *D = cast_or_null<CXXMethodDecl>(C.getMethodDecl()->getDefinition());
+ if (!D)
+ return nullptr;
+ auto *S = cast<CompoundStmt>(D->getBody());
+ if (S->size() != 1)
+ return nullptr;
+ if (auto *RS = dyn_cast<ReturnStmt>(*S->body_begin()))
+ return RS->getRetValue()->IgnoreParenImpCasts();
+ return nullptr;
+}
+
static void
getFieldsGlobalsAndFuncs(const Decl &D, FieldSet &Fields,
llvm::DenseSet<const VarDecl *> &Vars,
@@ -324,6 +336,12 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
} else if (auto *E = dyn_cast<DeclRefExpr>(&S)) {
insertIfGlobal(*E->getDecl(), Vars);
insertIfFunction(*E->getDecl(), Funcs);
+ } else if (const auto *C = dyn_cast<CXXMemberCallExpr>(&S)) {
+ // If this is a method that returns a member variable but does nothing else,
+ // model the field of the return value.
+ if (MemberExpr *E = dyn_cast_or_null<MemberExpr>(
+ getRetValueFromSingleReturnStmtMethod(*C)))
+ getFieldsGlobalsAndFuncs(*E, Fields, Vars, Funcs);
} else if (auto *E = dyn_cast<MemberExpr>(&S)) {
// FIXME: should we be using `E->getFoundDecl()`?
const ValueDecl *VD = E->getMemberDecl();
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index cdb1bc3cd16ac7b..355d18bc1fe55a6 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1446,6 +1446,50 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
+TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
+ std::string Code = R"(
+ class S {
+ int *P;
+ int *Q;
+ int X;
+ int Y;
+ int Z;
+ public:
+ int *getPtr() const { return P; }
+ int *getPtrNonConst() { return Q; }
+ int getInt() const { return X; }
+ int getInt(int i) const { return Y; }
+ int getIntNonConst() { return Z; }
+ int getIntNoDefinition() const;
+ };
+
+ void target() {
+ S s;
+ int *p = s.getPtr();
+ int *q = s.getPtrNonConst();
+ int x = s.getInt();
+ int y = s.getIntNonConst();
+ int z = s.getIntNoDefinition();
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env =
+ getEnvironmentAtAnnotation(Results, "p");
+ auto &SLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "s");
+ std::vector<const ValueDecl*> Fields;
+ for (auto [Field, _] : SLoc.children())
+ Fields.push_back(Field);
+ // Only the fields that have corresponding const accessor methods
+ // should be modeled.
+ ASSERT_THAT(Fields, UnorderedElementsAre(
+ findValueDecl(ASTCtx, "P"), findValueDecl(ASTCtx, "X")));
+ });
+}
+
TEST(TransferTest, StructModeledFieldsWithComplicatedInheritance) {
std::string Code = R"(
struct Base1 {
>From ec7e051b7b0901b5776fe0c0fe751b3a1d2f995d Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda <kinuko at chromium.org>
Date: Thu, 14 Sep 2023 15:09:29 +0000
Subject: [PATCH 2/4] Update the broken test
---
.../Analysis/FlowSensitive/TransferTest.cpp | 25 +++++++++++--------
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 355d18bc1fe55a6..bf3d002567cc06a 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1446,7 +1446,7 @@ TEST(TransferTest, BaseClassInitializer) {
llvm::Succeeded());
}
-TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
+TEST(TransferTest, StructModeledFieldsWithAccessor) {
std::string Code = R"(
class S {
int *P;
@@ -1457,9 +1457,9 @@ TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
public:
int *getPtr() const { return P; }
int *getPtrNonConst() { return Q; }
- int getInt() const { return X; }
- int getInt(int i) const { return Y; }
- int getIntNonConst() { return Z; }
+ int getInt(int i) const { return X; }
+ int getWithOtherWork(int i) { Y += i; return Y; }
+ int getIntNotCalled() const { return Z; }
int getIntNoDefinition() const;
};
@@ -1467,9 +1467,9 @@ TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
S s;
int *p = s.getPtr();
int *q = s.getPtrNonConst();
- int x = s.getInt();
- int y = s.getIntNonConst();
- int z = s.getIntNoDefinition();
+ int x = s.getInt(1);
+ int y = s.getWithOtherWork(1);
+ int w = s.getIntNoDefinition();
// [[p]]
}
)";
@@ -1481,12 +1481,15 @@ TEST(TransferTest, StructModeledFieldsWithConstAccessor) {
getEnvironmentAtAnnotation(Results, "p");
auto &SLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "s");
std::vector<const ValueDecl*> Fields;
- for (auto [Field, _] : SLoc.children())
+ for (auto [Field, _] : SLoc.children()) {
Fields.push_back(Field);
- // Only the fields that have corresponding const accessor methods
- // should be modeled.
+ llvm::dbgs() << Field->getNameAsString() << "\n";
+ }
+ // Only the fields that have simple accessor methods (that have a
+ // single statement body that returns the member variable) should be modeled.
ASSERT_THAT(Fields, UnorderedElementsAre(
- findValueDecl(ASTCtx, "P"), findValueDecl(ASTCtx, "X")));
+ findValueDecl(ASTCtx, "P"), findValueDecl(ASTCtx, "Q"),
+ findValueDecl(ASTCtx, "X")));
});
}
>From 62403de9ed1157d9a70866d87fbf4c5da7cc858f Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda <kinuko at chromium.org>
Date: Thu, 14 Sep 2023 15:11:24 +0000
Subject: [PATCH 3/4] Remove logging
---
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index bf3d002567cc06a..f1fb9576fefcf14 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1481,10 +1481,8 @@ TEST(TransferTest, StructModeledFieldsWithAccessor) {
getEnvironmentAtAnnotation(Results, "p");
auto &SLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "s");
std::vector<const ValueDecl*> Fields;
- for (auto [Field, _] : SLoc.children()) {
+ for (auto [Field, _] : SLoc.children())
Fields.push_back(Field);
- llvm::dbgs() << Field->getNameAsString() << "\n";
- }
// Only the fields that have simple accessor methods (that have a
// single statement body that returns the member variable) should be modeled.
ASSERT_THAT(Fields, UnorderedElementsAre(
>From 4a0a47be037521da42564719d9b5a968189ac11f Mon Sep 17 00:00:00 2001
From: Kinuko Yasuda <kinuko at chromium.org>
Date: Fri, 15 Sep 2023 23:16:39 +0000
Subject: [PATCH 4/4] Addressed comments
---
.../FlowSensitive/DataflowEnvironment.cpp | 19 ++++-----
.../Analysis/FlowSensitive/TransferTest.cpp | 40 ++++++++++---------
2 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 713df62e5009336..26e097349057238 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -288,15 +288,12 @@ static void insertIfFunction(const Decl &D,
Funcs.insert(FD);
}
-static Expr *getRetValueFromSingleReturnStmtMethod(const CXXMemberCallExpr &C) {
- auto *D = cast_or_null<CXXMethodDecl>(C.getMethodDecl()->getDefinition());
- if (!D)
+static MemberExpr *getMemberForAccessor(const CXXMemberCallExpr &C) {
+ auto *Body = dyn_cast_or_null<CompoundStmt>(C.getMethodDecl()->getBody());
+ if (!Body || Body->size() != 1)
return nullptr;
- auto *S = cast<CompoundStmt>(D->getBody());
- if (S->size() != 1)
- return nullptr;
- if (auto *RS = dyn_cast<ReturnStmt>(*S->body_begin()))
- return RS->getRetValue()->IgnoreParenImpCasts();
+ if (auto *RS = dyn_cast<ReturnStmt>(*Body->body_begin()))
+ return dyn_cast<MemberExpr>(RS->getRetValue()->IgnoreParenImpCasts());
return nullptr;
}
@@ -339,9 +336,9 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
} else if (const auto *C = dyn_cast<CXXMemberCallExpr>(&S)) {
// If this is a method that returns a member variable but does nothing else,
// model the field of the return value.
- if (MemberExpr *E = dyn_cast_or_null<MemberExpr>(
- getRetValueFromSingleReturnStmtMethod(*C)))
- getFieldsGlobalsAndFuncs(*E, Fields, Vars, Funcs);
+ if (MemberExpr *E = getMemberForAccessor(*C))
+ if (const auto *FD = dyn_cast<FieldDecl>(E->getMemberDecl()))
+ Fields.insert(FD);
} else if (auto *E = dyn_cast<MemberExpr>(&S)) {
// FIXME: should we be using `E->getFoundDecl()`?
const ValueDecl *VD = E->getMemberDecl();
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index f1fb9576fefcf14..14188f5acd5b36e 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1449,27 +1449,30 @@ TEST(TransferTest, BaseClassInitializer) {
TEST(TransferTest, StructModeledFieldsWithAccessor) {
std::string Code = R"(
class S {
- int *P;
- int *Q;
- int X;
- int Y;
- int Z;
+ int *Ptr;
+ int *PtrNonConst;
+ int Int;
+ int IntWithInc;
+ int IntNotAccessed;
+ int IntRef;
public:
- int *getPtr() const { return P; }
- int *getPtrNonConst() { return Q; }
- int getInt(int i) const { return X; }
- int getWithOtherWork(int i) { Y += i; return Y; }
- int getIntNotCalled() const { return Z; }
+ int *getPtr() const { return Ptr; }
+ int *getPtrNonConst() { return PtrNonConst; }
+ int getInt(int i) const { return Int; }
+ int getWithInc(int i) { IntWithInc += i; return IntWithInc; }
+ int getIntNotAccessed() const { return IntNotAccessed; }
int getIntNoDefinition() const;
+ int &getIntRef() { return IntRef; }
};
void target() {
S s;
- int *p = s.getPtr();
- int *q = s.getPtrNonConst();
- int x = s.getInt(1);
- int y = s.getWithOtherWork(1);
- int w = s.getIntNoDefinition();
+ int *p1 = s.getPtr();
+ int *p2 = s.getPtrNonConst();
+ int i1 = s.getInt(1);
+ int i2 = s.getWithInc(1);
+ int i3 = s.getIntNoDefinition();
+ int &iref = s.getIntRef();
// [[p]]
}
)";
@@ -1484,10 +1487,11 @@ TEST(TransferTest, StructModeledFieldsWithAccessor) {
for (auto [Field, _] : SLoc.children())
Fields.push_back(Field);
// Only the fields that have simple accessor methods (that have a
- // single statement body that returns the member variable) should be modeled.
+ // single statement body that returns the member variable) should be
+ // modeled.
ASSERT_THAT(Fields, UnorderedElementsAre(
- findValueDecl(ASTCtx, "P"), findValueDecl(ASTCtx, "Q"),
- findValueDecl(ASTCtx, "X")));
+ findValueDecl(ASTCtx, "Ptr"), findValueDecl(ASTCtx, "PtrNonConst"),
+ findValueDecl(ASTCtx, "Int"), findValueDecl(ASTCtx, "IntRef")));
});
}
More information about the cfe-commits
mailing list