diff --git a/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp b/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp index c9ebffe6f3780..8aed19544be6a 100644 --- a/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp +++ b/clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp @@ -39,8 +39,35 @@ buildStmtToBasicBlockMap(const CFG &Cfg) { StmtToBlock[Stmt->getStmt()] = Block; } - if (const Stmt *TerminatorStmt = Block->getTerminatorStmt()) - StmtToBlock[TerminatorStmt] = Block; + } + // Some terminator conditions don't appear as a `CFGElement` anywhere else - + // for example, this is true if the terminator condition is a `&&` or `||` + // operator. + // We associate these conditions with the block the terminator appears in, + // but only if the condition has not already appeared as a regular + // `CFGElement`. (The `insert()` below does nothing if the key already exists + // in the map.) + for (const CFGBlock *Block : Cfg) { + if (Block != nullptr) + if (const Stmt *TerminatorCond = Block->getTerminatorCondition()) + StmtToBlock.insert({TerminatorCond, Block}); + } + // Terminator statements typically don't appear as a `CFGElement` anywhere + // else, so we want to associate them with the block that they terminate. + // However, there are some important special cases: + // - The conditional operator is a type of terminator, but it also appears + // as a regular `CFGElement`, and we want to associate it with the block + // in which it appears as a `CFGElement`. + // - The `&&` and `||` operators are types of terminators, but like the + // conditional operator, they can appear as a regular `CFGElement` or + // as a terminator condition (see above). + // We process terminators last to make sure that we only associate them with + // the block they terminate if they haven't previously occurred as a regular + // `CFGElement` or as a terminator condition. + for (const CFGBlock *Block : Cfg) { + if (Block != nullptr) + if (const Stmt *TerminatorStmt = Block->getTerminatorStmt()) + StmtToBlock.insert({TerminatorStmt, Block}); } return StmtToBlock; } diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 3bca9cced8d6f..34f9b0b23719f 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -77,17 +77,33 @@ class DataflowAnalysisTest : public Test { return runDataflowAnalysis(*CFCtx, Analysis, Env); } + /// Returns the `CFGBlock` containing `S` (and asserts that it exists). + const CFGBlock *blockForStmt(const Stmt &S) { + const CFGBlock *Block = CFCtx->getStmtToBlock().lookup(&S); + assert(Block != nullptr); + return Block; + } + template const StateT & blockStateForStmt(const std::vector> &BlockStates, - const Stmt *S) { - const CFGBlock *Block = CFCtx->getStmtToBlock().lookup(S); - assert(Block != nullptr); - const std::optional &MaybeState = BlockStates[Block->getBlockID()]; + const Stmt &S) { + const std::optional &MaybeState = + BlockStates[blockForStmt(S)->getBlockID()]; assert(MaybeState.has_value()); return *MaybeState; } + /// Returns the first node that matches `Matcher` (and asserts that the match + /// was successful, i.e. the returned node is not null). + template + const NodeT &matchNode(MatcherT Matcher) { + const auto *Node = selectFirst( + "node", match(Matcher.bind("node"), AST->getASTContext())); + assert(Node != nullptr); + return *Node; + } + std::unique_ptr AST; std::unique_ptr CFCtx; std::unique_ptr DACtx; @@ -130,6 +146,79 @@ TEST_F(DataflowAnalysisTest, DiagnoseFunctionDiagnoserCalledOnEachElement) { " (Lifetime ends)\n"))); } +// Tests for the statement-to-block map. +using StmtToBlockTest = DataflowAnalysisTest; + +TEST_F(StmtToBlockTest, ConditionalOperator) { + std::string Code = R"( + void target(bool b) { + int i = b ? 1 : 0; + } + )"; + ASSERT_THAT_ERROR(runAnalysis( + Code, [](ASTContext &C) { return NoopAnalysis(C); }) + .takeError(), + llvm::Succeeded()); + + const auto &IDecl = matchNode(declStmt(has(varDecl(hasName("i"))))); + const auto &ConditionalOp = + matchNode(conditionalOperator()); + + // The conditional operator should be associated with the same block as the + // `DeclStmt` for `i`. (Specifically, the conditional operator should not be + // associated with the block for which it is the terminator.) + EXPECT_EQ(blockForStmt(IDecl), blockForStmt(ConditionalOp)); +} + +TEST_F(StmtToBlockTest, LogicalAnd) { + std::string Code = R"( + void target(bool b1, bool b2) { + bool b = b1 && b2; + } + )"; + ASSERT_THAT_ERROR(runAnalysis( + Code, [](ASTContext &C) { return NoopAnalysis(C); }) + .takeError(), + llvm::Succeeded()); + + const auto &BDecl = matchNode(declStmt(has(varDecl(hasName("b"))))); + const auto &AndOp = + matchNode(binaryOperator(hasOperatorName("&&"))); + + // The `&&` operator should be associated with the same block as the + // `DeclStmt` for `b`. (Specifically, the `&&` operator should not be + // associated with the block for which it is the terminator.) + EXPECT_EQ(blockForStmt(BDecl), blockForStmt(AndOp)); +} + +TEST_F(StmtToBlockTest, IfStatementWithLogicalAnd) { + std::string Code = R"( + void target(bool b1, bool b2) { + if (b1 && b2) + ; + } + )"; + ASSERT_THAT_ERROR(runAnalysis( + Code, [](ASTContext &C) { return NoopAnalysis(C); }) + .takeError(), + llvm::Succeeded()); + + const auto &If = matchNode(ifStmt()); + const auto &B2 = + matchNode(declRefExpr(to(varDecl(hasName("b2"))))); + const auto &AndOp = + matchNode(binaryOperator(hasOperatorName("&&"))); + + // The if statement is the terminator for the block that contains both `b2` + // and the `&&` operator (which appears only as a terminator condition, not + // as a regular `CFGElement`). + const CFGBlock *IfBlock = blockForStmt(If); + const CFGBlock *B2Block = blockForStmt(B2); + const CFGBlock *AndOpBlock = blockForStmt(AndOp); + EXPECT_EQ(IfBlock, B2Block); + EXPECT_EQ(IfBlock, AndOpBlock); +} + // Tests that check we discard state for expressions correctly. using DiscardExprStateTest = DataflowAnalysisTest; @@ -144,25 +233,20 @@ TEST_F(DiscardExprStateTest, WhileStatement) { auto BlockStates = llvm::cantFail(runAnalysis( Code, [](ASTContext &C) { return NoopAnalysis(C); })); - auto *NotEqOp = selectFirst( - "op", match(binaryOperator(hasOperatorName("!=")).bind("op"), - AST->getASTContext())); - ASSERT_NE(NotEqOp, nullptr); - - auto *CallFoo = selectFirst( - "call", match(callExpr(callee(functionDecl(hasName("foo")))).bind("call"), - AST->getASTContext())); - ASSERT_NE(CallFoo, nullptr); + const auto &NotEqOp = + matchNode(binaryOperator(hasOperatorName("!="))); + const auto &CallFoo = + matchNode(callExpr(callee(functionDecl(hasName("foo"))))); // In the block that evaluates the expression `p != nullptr`, this expression // is associated with a value. const auto &NotEqOpState = blockStateForStmt(BlockStates, NotEqOp); - EXPECT_NE(NotEqOpState.Env.getValue(*NotEqOp), nullptr); + EXPECT_NE(NotEqOpState.Env.getValue(NotEqOp), nullptr); // In the block that calls `foo(p)`, the value for `p != nullptr` is discarded // because it is not consumed by this block. const auto &CallFooState = blockStateForStmt(BlockStates, CallFoo); - EXPECT_EQ(CallFooState.Env.getValue(*NotEqOp), nullptr); + EXPECT_EQ(CallFooState.Env.getValue(NotEqOp), nullptr); } TEST_F(DiscardExprStateTest, BooleanOperator) { @@ -174,29 +258,24 @@ TEST_F(DiscardExprStateTest, BooleanOperator) { auto BlockStates = llvm::cantFail(runAnalysis( Code, [](ASTContext &C) { return NoopAnalysis(C); })); - auto *AndOp = selectFirst( - "op", match(binaryOperator(hasOperatorName("&&")).bind("op"), - AST->getASTContext())); - ASSERT_NE(AndOp, nullptr); - - auto *Return = selectFirst( - "return", match(returnStmt().bind("return"), AST->getASTContext())); - ASSERT_NE(Return, nullptr); + const auto &AndOp = + matchNode(binaryOperator(hasOperatorName("&&"))); + const auto &Return = matchNode(returnStmt()); // In the block that evaluates the LHS of the `&&` operator, the LHS is // associated with a value, while the right-hand side is not (unsurprisingly, // as it hasn't been evaluated yet). - const auto &LHSState = blockStateForStmt(BlockStates, AndOp->getLHS()); - auto *LHSValue = cast(LHSState.Env.getValue(*AndOp->getLHS())); + const auto &LHSState = blockStateForStmt(BlockStates, *AndOp.getLHS()); + auto *LHSValue = cast(LHSState.Env.getValue(*AndOp.getLHS())); ASSERT_NE(LHSValue, nullptr); - EXPECT_EQ(LHSState.Env.getValue(*AndOp->getRHS()), nullptr); + EXPECT_EQ(LHSState.Env.getValue(*AndOp.getRHS()), nullptr); // In the block that evaluates the RHS, the RHS is associated with a // value. The value for the LHS has been discarded as it is not consumed by // this block. - const auto &RHSState = blockStateForStmt(BlockStates, AndOp->getRHS()); - EXPECT_EQ(RHSState.Env.getValue(*AndOp->getLHS()), nullptr); - auto *RHSValue = cast(RHSState.Env.getValue(*AndOp->getRHS())); + const auto &RHSState = blockStateForStmt(BlockStates, *AndOp.getRHS()); + EXPECT_EQ(RHSState.Env.getValue(*AndOp.getLHS()), nullptr); + auto *RHSValue = cast(RHSState.Env.getValue(*AndOp.getRHS())); ASSERT_NE(RHSValue, nullptr); // In the block that evaluates the return statement, the expression `b1 && b2` @@ -217,9 +296,9 @@ TEST_F(DiscardExprStateTest, BooleanOperator) { // operands, rather than from the environment for the block that contains the // `&&`. const auto &ReturnState = blockStateForStmt(BlockStates, Return); - EXPECT_EQ(ReturnState.Env.getValue(*AndOp->getLHS()), nullptr); - EXPECT_EQ(ReturnState.Env.getValue(*AndOp->getRHS()), nullptr); - EXPECT_EQ(ReturnState.Env.getValue(*AndOp), + EXPECT_EQ(ReturnState.Env.getValue(*AndOp.getLHS()), nullptr); + EXPECT_EQ(ReturnState.Env.getValue(*AndOp.getRHS()), nullptr); + EXPECT_EQ(ReturnState.Env.getValue(AndOp), &ReturnState.Env.makeAnd(*LHSValue, *RHSValue)); }