Skip to content

Commit e899641

Browse files
martinboehmemartinboehme
authored
[clang][dataflow] Fix inaccuracies in buildStmtToBasicBlockMap(). (#82496)
See the comments added to the code for details on the inaccuracies that have now been fixed. The patch adds tests that fail with the old implementation.
1 parent edd4aee commit e899641

File tree

2 files changed

+140
-34
lines changed

2 files changed

+140
-34
lines changed

‎clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,35 @@ buildStmtToBasicBlockMap(const CFG &Cfg) {
3939

4040
StmtToBlock[Stmt->getStmt()] = Block;
4141
}
42-
if (const Stmt *TerminatorStmt = Block->getTerminatorStmt())
43-
StmtToBlock[TerminatorStmt] = Block;
42+
}
43+
// Some terminator conditions don't appear as a `CFGElement` anywhere else -
44+
// for example, this is true if the terminator condition is a `&&` or `||`
45+
// operator.
46+
// We associate these conditions with the block the terminator appears in,
47+
// but only if the condition has not already appeared as a regular
48+
// `CFGElement`. (The `insert()` below does nothing if the key already exists
49+
// in the map.)
50+
for (const CFGBlock *Block : Cfg) {
51+
if (Block != nullptr)
52+
if (const Stmt *TerminatorCond = Block->getTerminatorCondition())
53+
StmtToBlock.insert({TerminatorCond, Block});
54+
}
55+
// Terminator statements typically don't appear as a `CFGElement` anywhere
56+
// else, so we want to associate them with the block that they terminate.
57+
// However, there are some important special cases:
58+
// - The conditional operator is a type of terminator, but it also appears
59+
// as a regular `CFGElement`, and we want to associate it with the block
60+
// in which it appears as a `CFGElement`.
61+
// - The `&&` and `||` operators are types of terminators, but like the
62+
// conditional operator, they can appear as a regular `CFGElement` or
63+
// as a terminator condition (see above).
64+
// We process terminators last to make sure that we only associate them with
65+
// the block they terminate if they haven't previously occurred as a regular
66+
// `CFGElement` or as a terminator condition.
67+
for (const CFGBlock *Block : Cfg) {
68+
if (Block != nullptr)
69+
if (const Stmt *TerminatorStmt = Block->getTerminatorStmt())
70+
StmtToBlock.insert({TerminatorStmt, Block});
4471
}
4572
return StmtToBlock;
4673
}

‎clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Lines changed: 111 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -77,17 +77,33 @@ class DataflowAnalysisTest : public Test {
7777
return runDataflowAnalysis(*CFCtx, Analysis, Env);
7878
}
7979

80+
/// Returns the `CFGBlock` containing `S` (and asserts that it exists).
81+
const CFGBlock *blockForStmt(const Stmt &S) {
82+
const CFGBlock *Block = CFCtx->getStmtToBlock().lookup(&S);
83+
assert(Block != nullptr);
84+
return Block;
85+
}
86+
8087
template <typename StateT>
8188
const StateT &
8289
blockStateForStmt(const std::vector<std::optional<StateT>> &BlockStates,
83-
const Stmt *S) {
84-
const CFGBlock *Block = CFCtx->getStmtToBlock().lookup(S);
85-
assert(Block != nullptr);
86-
const std::optional<StateT> &MaybeState = BlockStates[Block->getBlockID()];
90+
const Stmt &S) {
91+
const std::optional<StateT> &MaybeState =
92+
BlockStates[blockForStmt(S)->getBlockID()];
8793
assert(MaybeState.has_value());
8894
return *MaybeState;
8995
}
9096

97+
/// Returns the first node that matches `Matcher` (and asserts that the match
98+
/// was successful, i.e. the returned node is not null).
99+
template <typename NodeT, typename MatcherT>
100+
const NodeT &matchNode(MatcherT Matcher) {
101+
const auto *Node = selectFirst<NodeT>(
102+
"node", match(Matcher.bind("node"), AST->getASTContext()));
103+
assert(Node != nullptr);
104+
return *Node;
105+
}
106+
91107
std::unique_ptr<ASTUnit> AST;
92108
std::unique_ptr<ControlFlowContext> CFCtx;
93109
std::unique_ptr<DataflowAnalysisContext> DACtx;
@@ -130,6 +146,79 @@ TEST_F(DataflowAnalysisTest, DiagnoseFunctionDiagnoserCalledOnEachElement) {
130146
" (Lifetime ends)\n")));
131147
}
132148

149+
// Tests for the statement-to-block map.
150+
using StmtToBlockTest = DataflowAnalysisTest;
151+
152+
TEST_F(StmtToBlockTest, ConditionalOperator) {
153+
std::string Code = R"(
154+
void target(bool b) {
155+
int i = b ? 1 : 0;
156+
}
157+
)";
158+
ASSERT_THAT_ERROR(runAnalysis<NoopAnalysis>(
159+
Code, [](ASTContext &C) { return NoopAnalysis(C); })
160+
.takeError(),
161+
llvm::Succeeded());
162+
163+
const auto &IDecl = matchNode<DeclStmt>(declStmt(has(varDecl(hasName("i")))));
164+
const auto &ConditionalOp =
165+
matchNode<ConditionalOperator>(conditionalOperator());
166+
167+
// The conditional operator should be associated with the same block as the
168+
// `DeclStmt` for `i`. (Specifically, the conditional operator should not be
169+
// associated with the block for which it is the terminator.)
170+
EXPECT_EQ(blockForStmt(IDecl), blockForStmt(ConditionalOp));
171+
}
172+
173+
TEST_F(StmtToBlockTest, LogicalAnd) {
174+
std::string Code = R"(
175+
void target(bool b1, bool b2) {
176+
bool b = b1 && b2;
177+
}
178+
)";
179+
ASSERT_THAT_ERROR(runAnalysis<NoopAnalysis>(
180+
Code, [](ASTContext &C) { return NoopAnalysis(C); })
181+
.takeError(),
182+
llvm::Succeeded());
183+
184+
const auto &BDecl = matchNode<DeclStmt>(declStmt(has(varDecl(hasName("b")))));
185+
const auto &AndOp =
186+
matchNode<BinaryOperator>(binaryOperator(hasOperatorName("&&")));
187+
188+
// The `&&` operator should be associated with the same block as the
189+
// `DeclStmt` for `b`. (Specifically, the `&&` operator should not be
190+
// associated with the block for which it is the terminator.)
191+
EXPECT_EQ(blockForStmt(BDecl), blockForStmt(AndOp));
192+
}
193+
194+
TEST_F(StmtToBlockTest, IfStatementWithLogicalAnd) {
195+
std::string Code = R"(
196+
void target(bool b1, bool b2) {
197+
if (b1 && b2)
198+
;
199+
}
200+
)";
201+
ASSERT_THAT_ERROR(runAnalysis<NoopAnalysis>(
202+
Code, [](ASTContext &C) { return NoopAnalysis(C); })
203+
.takeError(),
204+
llvm::Succeeded());
205+
206+
const auto &If = matchNode<IfStmt>(ifStmt());
207+
const auto &B2 =
208+
matchNode<DeclRefExpr>(declRefExpr(to(varDecl(hasName("b2")))));
209+
const auto &AndOp =
210+
matchNode<BinaryOperator>(binaryOperator(hasOperatorName("&&")));
211+
212+
// The if statement is the terminator for the block that contains both `b2`
213+
// and the `&&` operator (which appears only as a terminator condition, not
214+
// as a regular `CFGElement`).
215+
const CFGBlock *IfBlock = blockForStmt(If);
216+
const CFGBlock *B2Block = blockForStmt(B2);
217+
const CFGBlock *AndOpBlock = blockForStmt(AndOp);
218+
EXPECT_EQ(IfBlock, B2Block);
219+
EXPECT_EQ(IfBlock, AndOpBlock);
220+
}
221+
133222
// Tests that check we discard state for expressions correctly.
134223
using DiscardExprStateTest = DataflowAnalysisTest;
135224

@@ -144,25 +233,20 @@ TEST_F(DiscardExprStateTest, WhileStatement) {
144233
auto BlockStates = llvm::cantFail(runAnalysis<NoopAnalysis>(
145234
Code, [](ASTContext &C) { return NoopAnalysis(C); }));
146235

147-
auto *NotEqOp = selectFirst<BinaryOperator>(
148-
"op", match(binaryOperator(hasOperatorName("!=")).bind("op"),
149-
AST->getASTContext()));
150-
ASSERT_NE(NotEqOp, nullptr);
151-
152-
auto *CallFoo = selectFirst<CallExpr>(
153-
"call", match(callExpr(callee(functionDecl(hasName("foo")))).bind("call"),
154-
AST->getASTContext()));
155-
ASSERT_NE(CallFoo, nullptr);
236+
const auto &NotEqOp =
237+
matchNode<BinaryOperator>(binaryOperator(hasOperatorName("!=")));
238+
const auto &CallFoo =
239+
matchNode<CallExpr>(callExpr(callee(functionDecl(hasName("foo")))));
156240

157241
// In the block that evaluates the expression `p != nullptr`, this expression
158242
// is associated with a value.
159243
const auto &NotEqOpState = blockStateForStmt(BlockStates, NotEqOp);
160-
EXPECT_NE(NotEqOpState.Env.getValue(*NotEqOp), nullptr);
244+
EXPECT_NE(NotEqOpState.Env.getValue(NotEqOp), nullptr);
161245

162246
// In the block that calls `foo(p)`, the value for `p != nullptr` is discarded
163247
// because it is not consumed by this block.
164248
const auto &CallFooState = blockStateForStmt(BlockStates, CallFoo);
165-
EXPECT_EQ(CallFooState.Env.getValue(*NotEqOp), nullptr);
249+
EXPECT_EQ(CallFooState.Env.getValue(NotEqOp), nullptr);
166250
}
167251

168252
TEST_F(DiscardExprStateTest, BooleanOperator) {
@@ -174,29 +258,24 @@ TEST_F(DiscardExprStateTest, BooleanOperator) {
174258
auto BlockStates = llvm::cantFail(runAnalysis<NoopAnalysis>(
175259
Code, [](ASTContext &C) { return NoopAnalysis(C); }));
176260

177-
auto *AndOp = selectFirst<BinaryOperator>(
178-
"op", match(binaryOperator(hasOperatorName("&&")).bind("op"),
179-
AST->getASTContext()));
180-
ASSERT_NE(AndOp, nullptr);
181-
182-
auto *Return = selectFirst<ReturnStmt>(
183-
"return", match(returnStmt().bind("return"), AST->getASTContext()));
184-
ASSERT_NE(Return, nullptr);
261+
const auto &AndOp =
262+
matchNode<BinaryOperator>(binaryOperator(hasOperatorName("&&")));
263+
const auto &Return = matchNode<ReturnStmt>(returnStmt());
185264

186265
// In the block that evaluates the LHS of the `&&` operator, the LHS is
187266
// associated with a value, while the right-hand side is not (unsurprisingly,
188267
// as it hasn't been evaluated yet).
189-
const auto &LHSState = blockStateForStmt(BlockStates, AndOp->getLHS());
190-
auto *LHSValue = cast<BoolValue>(LHSState.Env.getValue(*AndOp->getLHS()));
268+
const auto &LHSState = blockStateForStmt(BlockStates, *AndOp.getLHS());
269+
auto *LHSValue = cast<BoolValue>(LHSState.Env.getValue(*AndOp.getLHS()));
191270
ASSERT_NE(LHSValue, nullptr);
192-
EXPECT_EQ(LHSState.Env.getValue(*AndOp->getRHS()), nullptr);
271+
EXPECT_EQ(LHSState.Env.getValue(*AndOp.getRHS()), nullptr);
193272

194273
// In the block that evaluates the RHS, the RHS is associated with a
195274
// value. The value for the LHS has been discarded as it is not consumed by
196275
// this block.
197-
const auto &RHSState = blockStateForStmt(BlockStates, AndOp->getRHS());
198-
EXPECT_EQ(RHSState.Env.getValue(*AndOp->getLHS()), nullptr);
199-
auto *RHSValue = cast<BoolValue>(RHSState.Env.getValue(*AndOp->getRHS()));
276+
const auto &RHSState = blockStateForStmt(BlockStates, *AndOp.getRHS());
277+
EXPECT_EQ(RHSState.Env.getValue(*AndOp.getLHS()), nullptr);
278+
auto *RHSValue = cast<BoolValue>(RHSState.Env.getValue(*AndOp.getRHS()));
200279
ASSERT_NE(RHSValue, nullptr);
201280

202281
// In the block that evaluates the return statement, the expression `b1 && b2`
@@ -217,9 +296,9 @@ TEST_F(DiscardExprStateTest, BooleanOperator) {
217296
// operands, rather than from the environment for the block that contains the
218297
// `&&`.
219298
const auto &ReturnState = blockStateForStmt(BlockStates, Return);
220-
EXPECT_EQ(ReturnState.Env.getValue(*AndOp->getLHS()), nullptr);
221-
EXPECT_EQ(ReturnState.Env.getValue(*AndOp->getRHS()), nullptr);
222-
EXPECT_EQ(ReturnState.Env.getValue(*AndOp),
299+
EXPECT_EQ(ReturnState.Env.getValue(*AndOp.getLHS()), nullptr);
300+
EXPECT_EQ(ReturnState.Env.getValue(*AndOp.getRHS()), nullptr);
301+
EXPECT_EQ(ReturnState.Env.getValue(AndOp),
223302
&ReturnState.Env.makeAnd(*LHSValue, *RHSValue));
224303
}
225304

0 commit comments

Comments
 (0)