[clang][dataflow] Fix bug in buildContainsExprConsumedInDifferentBlock().
#100874
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ck()`. 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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) ChangesThis was missing a call to To prevent this kind of bug in the future, the patch also adds a new method Full diff: https://github.com/llvm/llvm-project/pull/100874.diff 5 Files Affected:
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;
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was missing a call to
ignoreCFGOmittedNodes(). As a result, the functionwould erroneously conclude that a block did not contain an expression consumed
in a different block if the expression in question was surrounded by a
ParenExprin the consuming block. The patch adds a test that triggers thisscenario (and fails without the fix).
To prevent this kind of bug in the future, the patch also adds a new method
blockForStmt()toAdornedCFGthat callsignoreCFGOmittedNodes()and ispreferred over accessing
getStmtToBlock()directly.