Skip to content

[dataflow] CXXForRangeStmt should extend flow condition #80989

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[dataflow] CXXForRangeStmt should extend flow condition #80989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,12 @@ class TerminatorVisitor
return {nullptr, false};
}

TerminatorVisitorRetTy VisitCXXForRangeStmt(const CXXForRangeStmt *) {
// Don't do anything special for CXXForRangeStmt, because the condition
// (being implicitly generated) isn't visible from the loop body.
TerminatorVisitorRetTy VisitCXXForRangeStmt(const CXXForRangeStmt *S) {
// Even though the condition isn't visible from the loop body, analysis
// might depend on the implicit implicit statements implied by the loop.
auto *Cond = S->getCond();
if (Cond != nullptr)
return extendFlowCondition(*Cond);
return {nullptr, false};
}

Expand Down
21 changes: 21 additions & 0 deletions clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1710,4 +1710,25 @@ TEST_F(TopTest, ForRangeStmtConverges) {
// analysis converged.
});
}

TEST_F(TopTest, ForRangeStmtHasFlowCondition) {
std::string Code = R"(
#include <array>
void target(bool Foo) {
std::array<int, 5> t;
for (auto& i : t) {
(void)0;
/*[[p1]]*/
}
}
Comment on lines +1716 to +1723
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <array>
void target(bool Foo) {
std::array<int, 5> t;
for (auto& i : t) {
(void)0;
/*[[p1]]*/
}
}
#include <initializer_list>
void target() {
for (auto& i : {1, 2}) {
(void)0;
/*[[p1]]*/
}
}

Try to keep code snippets as simple as possible:

  • I don't think Foo is needed here?
  • An initializer_list is probably the simplest thing you can use to iterate over (it doesn't matter specifically what you're iterating over, right?)

)";
runDataflow(Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
const AnalysisOutputs &AO) {
ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1"));
const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
Comment on lines +1728 to +1729
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1"));
const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");

  • A lot of tests still do the ASSERT_THAT(Results.keys(), UnorderedElementsAre(...));, but it's unnecessary -- getEnvironmentAtAnnotation() itself asserts that the annotation exists.

  • When there is only one environment you want to query, the usual names are Env and p (will need to be changed in the code snippet too, obviously).

ASSERT_TRUE(Env1.proves(Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ASSERT_TRUE(Env1.proves(Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));
EXPECT_TRUE(Env1.proves(Env1.arena().makeAtomRef(Env1.getFlowConditionToken())));

ASSERT_ aborts the test if the check fails and should generally only be used if it's not meaningful to execute the rest of the test in this case. Otherwise, use EXPECT_ so that you can continue executing the rest of the test.

It obviously doesn't make a difference in this case, but a) it's good style to default to EXPECT_ anyway, and b) people might add more checks later, and they might forget to change the ASSERT_ to an EXPECT_.

});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fail without the other changes in this PR?

If it does, this looks to me more like a bug in DataflowEnvironment; this assertion should generally be true for any environment, whether or not we extended the flow condition. (In that sense, the name of the test isn't accurate, because we would expect this to pass even if the flow condition was not extended.)

}
Comment on lines +1714 to +1732
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this test work @martinboehme ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments above -- I'm not sure that the test, if it fails, means that we need a flow condition for the range-based for. (User code will never be able to "observe" this flow condition, so I'm not sure why we need it.)

I still don't understand what the underlying purpose is that you need this flow condition for in the iterator check. Can you expand? (I want to make sure we're not dealing with an XY problem.)


} // namespace
Loading