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 <kin...@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 <kin...@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 <kin...@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 <kin...@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"))); }); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits