llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: None (martinboehme) <details> <summary>Changes</summary> This was missing a call to `ignoreCFGOmittedNodes()`. As a result, the function would erroneously conclude that a block did not contain an expression consumed in a different block if the expression in question was surrounded by a `ParenExpr` in the consuming block. The patch adds a test that triggers this scenario (and fails without the fix). To prevent this kind of bug in the future, the patch also adds a new method `blockForStmt()` to `AdornedCFG` that calls `ignoreCFGOmittedNodes()` and is preferred over accessing `getStmtToBlock()` directly. --- Full diff: https://github.com/llvm/llvm-project/pull/100874.diff 5 Files Affected: - (modified) clang/include/clang/Analysis/FlowSensitive/AdornedCFG.h (+31-4) - (modified) clang/lib/Analysis/FlowSensitive/AdornedCFG.cpp (+11-5) - (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+5-6) - (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+5-4) - (modified) clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp (+38-1) ``````````diff diff --git a/clang/include/clang/Analysis/FlowSensitive/AdornedCFG.h b/clang/include/clang/Analysis/FlowSensitive/AdornedCFG.h index 420f13ce11bfd..5de66fcb0e3af 100644 --- a/clang/include/clang/Analysis/FlowSensitive/AdornedCFG.h +++ b/clang/include/clang/Analysis/FlowSensitive/AdornedCFG.h @@ -18,6 +18,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/Stmt.h" #include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/ASTOps.h" #include "llvm/ADT/BitVector.h" #include "llvm/ADT/DenseMap.h" #include "llvm/Support/Error.h" @@ -27,6 +28,24 @@ namespace clang { namespace dataflow { +namespace internal { +class StmtToBlockMap { +public: + StmtToBlockMap(const CFG &Cfg); + + const CFGBlock *lookup(const Stmt &S) const { + return StmtToBlock.lookup(&ignoreCFGOmittedNodes(S)); + } + + const llvm::DenseMap<const Stmt *, const CFGBlock *> &getMap() const { + return StmtToBlock; + } + +private: + llvm::DenseMap<const Stmt *, const CFGBlock *> StmtToBlock; +}; +} // namespace internal + /// Holds CFG with additional information derived from it that is needed to /// perform dataflow analysis. class AdornedCFG { @@ -49,8 +68,17 @@ class AdornedCFG { const CFG &getCFG() const { return *Cfg; } /// Returns a mapping from statements to basic blocks that contain them. + /// Deprecated. Use `blockForStmt()` instead (which prevents the potential + /// error of forgetting to call `ignoreCFGOmittedNodes()` on the statement to + /// look up). const llvm::DenseMap<const Stmt *, const CFGBlock *> &getStmtToBlock() const { - return StmtToBlock; + return StmtToBlock.getMap(); + } + + /// Returns the basic block that contains `S`, or null if no basic block + /// containing `S` is found. + const CFGBlock *blockForStmt(const Stmt &S) const { + return StmtToBlock.lookup(S); } /// Returns whether `B` is reachable from the entry block. @@ -73,8 +101,7 @@ class AdornedCFG { private: AdornedCFG( const Decl &D, std::unique_ptr<CFG> Cfg, - llvm::DenseMap<const Stmt *, const CFGBlock *> StmtToBlock, - llvm::BitVector BlockReachable, + internal::StmtToBlockMap StmtToBlock, llvm::BitVector BlockReachable, llvm::DenseSet<const CFGBlock *> ContainsExprConsumedInDifferentBlock) : ContainingDecl(D), Cfg(std::move(Cfg)), StmtToBlock(std::move(StmtToBlock)), @@ -85,7 +112,7 @@ class AdornedCFG { /// The `Decl` containing the statement used to construct the CFG. const Decl &ContainingDecl; std::unique_ptr<CFG> Cfg; - llvm::DenseMap<const Stmt *, const CFGBlock *> StmtToBlock; + internal::StmtToBlockMap StmtToBlock; llvm::BitVector BlockReachable; llvm::DenseSet<const CFGBlock *> ContainsExprConsumedInDifferentBlock; }; diff --git a/clang/lib/Analysis/FlowSensitive/AdornedCFG.cpp b/clang/lib/Analysis/FlowSensitive/AdornedCFG.cpp index 255543021a998..876b5a3db5249 100644 --- a/clang/lib/Analysis/FlowSensitive/AdornedCFG.cpp +++ b/clang/lib/Analysis/FlowSensitive/AdornedCFG.cpp @@ -16,6 +16,7 @@ #include "clang/AST/Decl.h" #include "clang/AST/Stmt.h" #include "clang/Analysis/CFG.h" +#include "clang/Analysis/FlowSensitive/ASTOps.h" #include "llvm/ADT/BitVector.h" #include "llvm/ADT/DenseMap.h" #include "llvm/Support/Error.h" @@ -96,8 +97,7 @@ static llvm::BitVector findReachableBlocks(const CFG &Cfg) { static llvm::DenseSet<const CFGBlock *> buildContainsExprConsumedInDifferentBlock( - const CFG &Cfg, - const llvm::DenseMap<const Stmt *, const CFGBlock *> &StmtToBlock) { + const CFG &Cfg, const internal::StmtToBlockMap &StmtToBlock) { llvm::DenseSet<const CFGBlock *> Result; auto CheckChildExprs = [&Result, &StmtToBlock](const Stmt *S, @@ -105,7 +105,7 @@ buildContainsExprConsumedInDifferentBlock( for (const Stmt *Child : S->children()) { if (!isa_and_nonnull<Expr>(Child)) continue; - const CFGBlock *ChildBlock = StmtToBlock.lookup(Child); + const CFGBlock *ChildBlock = StmtToBlock.lookup(*Child); if (ChildBlock != Block) Result.insert(ChildBlock); } @@ -126,6 +126,13 @@ buildContainsExprConsumedInDifferentBlock( return Result; } +namespace internal { + +StmtToBlockMap::StmtToBlockMap(const CFG &Cfg) + : StmtToBlock(buildStmtToBasicBlockMap(Cfg)) {} + +} // namespace internal + llvm::Expected<AdornedCFG> AdornedCFG::build(const FunctionDecl &Func) { if (!Func.doesThisDeclarationHaveABody()) return llvm::createStringError( @@ -166,8 +173,7 @@ llvm::Expected<AdornedCFG> AdornedCFG::build(const Decl &D, Stmt &S, std::make_error_code(std::errc::invalid_argument), "CFG::buildCFG failed"); - llvm::DenseMap<const Stmt *, const CFGBlock *> StmtToBlock = - buildStmtToBasicBlockMap(*Cfg); + internal::StmtToBlockMap StmtToBlock(*Cfg); llvm::BitVector BlockReachable = findReachableBlocks(*Cfg); diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 3c896d373a211..9c54eb16d2224 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -40,17 +40,16 @@ namespace clang { namespace dataflow { const Environment *StmtToEnvMap::getEnvironment(const Stmt &S) const { - auto BlockIt = ACFG.getStmtToBlock().find(&ignoreCFGOmittedNodes(S)); - if (BlockIt == ACFG.getStmtToBlock().end()) { + const CFGBlock *Block = ACFG.blockForStmt(S); + if (Block == nullptr) { assert(false); - // Return null to avoid dereferencing the end iterator in non-assert builds. return nullptr; } - if (!ACFG.isBlockReachable(*BlockIt->getSecond())) + if (!ACFG.isBlockReachable(*Block)) return nullptr; - if (BlockIt->getSecond()->getBlockID() == CurBlockID) + if (Block->getBlockID() == CurBlockID) return &CurState.Env; - const auto &State = BlockToState[BlockIt->getSecond()->getBlockID()]; + const auto &State = BlockToState[Block->getBlockID()]; if (!(State)) return nullptr; return &State->Env; diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index 200682faafd6a..8afd18b315d28 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -243,10 +243,11 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) { // See `NoreturnDestructorTest` for concrete examples. if (Block.succ_begin()->getReachableBlock() != nullptr && Block.succ_begin()->getReachableBlock()->hasNoReturnElement()) { - auto &StmtToBlock = AC.ACFG.getStmtToBlock(); - auto StmtBlock = StmtToBlock.find(Block.getTerminatorStmt()); - assert(StmtBlock != StmtToBlock.end()); - llvm::erase(Preds, StmtBlock->getSecond()); + const CFGBlock *StmtBlock = nullptr; + if (const Stmt *Terminator = Block.getTerminatorStmt()) + StmtBlock = AC.ACFG.blockForStmt(*Terminator); + assert(StmtBlock != nullptr); + llvm::erase(Preds, StmtBlock); } } diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 1a52b82d65665..8717d9753d161 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -9,6 +9,7 @@ #include "TestingSupport.h" #include "clang/AST/Decl.h" #include "clang/AST/ExprCXX.h" +#include "clang/AST/OperationKinds.h" #include "clang/AST/Type.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" @@ -79,7 +80,7 @@ class DataflowAnalysisTest : public Test { /// Returns the `CFGBlock` containing `S` (and asserts that it exists). const CFGBlock *blockForStmt(const Stmt &S) { - const CFGBlock *Block = ACFG->getStmtToBlock().lookup(&S); + const CFGBlock *Block = ACFG->blockForStmt(S); assert(Block != nullptr); return Block; } @@ -370,6 +371,42 @@ TEST_F(DiscardExprStateTest, ConditionalOperator) { EXPECT_EQ(CallGState.Env.get<PointerValue>(AddrOfI), nullptr); } +TEST_F(DiscardExprStateTest, CallWithParenExprTreatedCorrectly) { + // This is a regression test. + // In the CFG for `target()` below, the expression that evaluates the function + // pointer for `expect` and the actual call are separated into different + // baseic blocks (because of the control flow introduced by the `||` + // operator). + // The value for the `expect` function pointer was erroneously discarded + // from the environment between these two blocks because the code that + // determines whether the expression values for a block need to be preserved + // did not ignore the `ParenExpr` around `(i == 1)` (which is not represented + // in the CFG). + std::string Code = R"( + bool expect(bool, bool); + void target(int i) { + expect(false || (i == 1), false); + } + )"; + auto BlockStates = llvm::cantFail(runAnalysis<NoopAnalysis>( + Code, [](ASTContext &C) { return NoopAnalysis(C); })); + + const auto &FnToPtrDecay = matchNode<ImplicitCastExpr>( + implicitCastExpr(hasCastKind(CK_FunctionToPointerDecay))); + const auto &CallExpect = + matchNode<CallExpr>(callExpr(callee(functionDecl(hasName("expect"))))); + + // In the block that evaluates the implicit cast of `expect` to a pointer, + // this expression is associated with a value. + const auto &FnToPtrDecayState = blockStateForStmt(BlockStates, FnToPtrDecay); + EXPECT_NE(FnToPtrDecayState.Env.getValue(FnToPtrDecay), nullptr); + + // In the block that calls `expect()`, the implicit cast of `expect` to a + // pointer is still associated with a value. + const auto &CallExpectState = blockStateForStmt(BlockStates, CallExpect); + EXPECT_NE(CallExpectState.Env.getValue(FnToPtrDecay), nullptr); +} + struct NonConvergingLattice { int State; `````````` </details> https://github.com/llvm/llvm-project/pull/100874 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
