[clang] [analyzer][NFC] Cleanup BranchNodeBuilder (PR #117898)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 27 07:46:42 PST 2024
https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/117898
Previously `BranchNodeBuilder` had a machinery to mark the two possible branches (true, false) as infeasible, but this was completely useless in practice, because the `BranchNodeBuilder` objects where short-lived local variables so the `markInfeasible()` calls did not affect any later calls.
The only theoretical exception was that in `ExprEngine::processBranch` the methods of `BranchNodeBuilder` were called within a `for` loop that iterates over the nodes created by the `check::BranchCondition` callbacks.
However, currently only two checkers listen to `check::BranchCondition` and neither of them produces multiple out nodes. This is fortunate, because if the `for` loop had multiple iterations, then the lingering effects of `markInfeasible()` would have caused wildly incorrect behavior.
_For example, let's assume that a hypothetical `check::BranchCondition` callback transitions to two different states, and the condition expression happens to be true in the first and false in the second. In this situation the first iteration of the loop would mark the false branch as 'infeasible' and then in the second iteration the analyzer would skip creating the transition to the false branch (from the state where that is the 'right' path forward)._
After removing `markInfeasible()`, it became clear that the `isFeasible()` calls in `ExprEngine::processBranch` are redundant because they only guarded a `generateNode` call -- which immediately calls `isFeasible()` and does nothing on an infeasible branch.
At this point in the refactoring the only code writing the feasibility data members is their initialization in the constructors of `BranchNodeBuilder` and the only code reading them is the check at the beginning of `BranchNodeBuilder::generateNode`, so it was straightforward to remove them completely and simplify the logic of `generateNode()` to let it directly check the nullness of the `CFGBlock*` pointer that it wants to use.
Finally, after these changes it became clear that in `ExprEngine::processBranch` the `else` branch
(corresponding to the case when `assumeCondition` fails) is equivalent to the "normal" case, so I eliminated it as well.
I also update the capitalization of a few variables that are already affected by this change.
>From 4d2ef600a0f5d259c5586b8844a417553eccbc3a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Wed, 27 Nov 2024 15:18:33 +0100
Subject: [PATCH] [analyzer][NFC] Cleanup BranchNodeBuilder
Previously `BranchNodeBuilder` had a machinery to mark the two possible
branches (true, false) as infeasible, but this was completely useless in
practice, because the `BranchNodeBuilder` objects where short-lived
local variables so the `markInfeasible()` calls did not affect any later
calls.
The only theoretical exception was that in `ExprEngine::processBranch`
the methods of `BranchNodeBuilder` were called within a `for` loop that
iterates over the nodes created by the `check::BranchCondition`
callbacks.
However, currently only two checkers listen to `check::BranchCondition` and
neither of them produces multiple out nodes. This is fortunate, because
if the `for` loop had multiple iterations, then the lingering effects of
`markInfeasible()` would have caused wildly incorrect behavior.
_For example, let's assume that a hypothetical `check::BranchCondition`
callback transitions to two different states, and the condition
expression happens to be true in the first and false in the second. In
this situation the first iteration of the loop would mark the false
branch as 'infeasible' and then in the second iteration the analyzer
would skip creating the transition to the false branch (from the state
where that is the 'right' path forward)._
After removing `markInfeasible()`, it became clear that the
`isFeasible()` calls in `ExprEngine::processBranch` are redundant
because they only guarded a `generateNode` call -- which immediately
calls `isFeasible()` and does nothing on an infeasible branch.
At this point in the refactoring the only code writing the feasibility
data members is their initialization in the constructors of
`BranchNodeBuilder` and the only code reading them is the check at the
beginning of `BranchNodeBuilder::generateNode`, so it was
straightforward to remove them completely and simplify the logic of
`generateNode()` to let it directly check the nullness of the
`CFGBlock*` pointer that it wants to use.
Finally, after these changes it became clear that in
`ExprEngine::processBranch` the `else` branch
(corresponding to the case when `assumeCondition` fails) is equivalent
to the "normal" case, so I eliminated it as well.
---
.../Core/PathSensitive/CoreEngine.h | 32 +++-------------
clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 11 +++---
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 37 +++++--------------
3 files changed, 21 insertions(+), 59 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
index 0825ecbced3f5a..a6d05a3ac67b4e 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h
@@ -429,47 +429,27 @@ class BranchNodeBuilder: public NodeBuilder {
const CFGBlock *DstT;
const CFGBlock *DstF;
- bool InFeasibleTrue;
- bool InFeasibleFalse;
-
void anchor() override;
public:
BranchNodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet,
- const NodeBuilderContext &C,
- const CFGBlock *dstT, const CFGBlock *dstF)
- : NodeBuilder(SrcNode, DstSet, C), DstT(dstT), DstF(dstF),
- InFeasibleTrue(!DstT), InFeasibleFalse(!DstF) {
+ const NodeBuilderContext &C, const CFGBlock *DT,
+ const CFGBlock *DF)
+ : NodeBuilder(SrcNode, DstSet, C), DstT(DT), DstF(DF) {
// The branch node builder does not generate autotransitions.
// If there are no successors it means that both branches are infeasible.
takeNodes(SrcNode);
}
BranchNodeBuilder(const ExplodedNodeSet &SrcSet, ExplodedNodeSet &DstSet,
- const NodeBuilderContext &C,
- const CFGBlock *dstT, const CFGBlock *dstF)
- : NodeBuilder(SrcSet, DstSet, C), DstT(dstT), DstF(dstF),
- InFeasibleTrue(!DstT), InFeasibleFalse(!DstF) {
+ const NodeBuilderContext &C, const CFGBlock *DT,
+ const CFGBlock *DF)
+ : NodeBuilder(SrcSet, DstSet, C), DstT(DT), DstF(DF) {
takeNodes(SrcSet);
}
ExplodedNode *generateNode(ProgramStateRef State, bool branch,
ExplodedNode *Pred);
-
- const CFGBlock *getTargetBlock(bool branch) const {
- return branch ? DstT : DstF;
- }
-
- void markInfeasible(bool branch) {
- if (branch)
- InFeasibleTrue = true;
- else
- InFeasibleFalse = true;
- }
-
- bool isFeasible(bool branch) {
- return branch ? !InFeasibleTrue : !InFeasibleFalse;
- }
};
class IndirectGotoNodeBuilder {
diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
index 8605fa149e4f52..67b7d30853d9de 100644
--- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp
@@ -649,14 +649,15 @@ StmtNodeBuilder::~StmtNodeBuilder() {
void BranchNodeBuilder::anchor() {}
ExplodedNode *BranchNodeBuilder::generateNode(ProgramStateRef State,
- bool branch,
+ bool Branch,
ExplodedNode *NodePred) {
- // If the branch has been marked infeasible we should not generate a node.
- if (!isFeasible(branch))
+ const CFGBlock *Dst = Branch ? DstT : DstF;
+
+ if (!Dst)
return nullptr;
- ProgramPoint Loc = BlockEdge(C.getBlock(), branch ? DstT : DstF,
- NodePred->getLocationContext());
+ ProgramPoint Loc =
+ BlockEdge(C.getBlock(), Dst, NodePred->getLocationContext());
ExplodedNode *Succ = generateNodeImpl(Loc, State, NodePred);
return Succ;
}
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 22eab9f66418d4..857af9cff267a3 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1655,10 +1655,8 @@ void ExprEngine::processCleanupTemporaryBranch(const CXXBindTemporaryExpr *BTE,
ProgramStateRef State = Pred->getState();
const LocationContext *LC = Pred->getLocationContext();
if (getObjectUnderConstruction(State, BTE, LC)) {
- TempDtorBuilder.markInfeasible(false);
TempDtorBuilder.generateNode(State, true, Pred);
} else {
- TempDtorBuilder.markInfeasible(true);
TempDtorBuilder.generateNode(State, false, Pred);
}
}
@@ -2778,7 +2776,6 @@ void ExprEngine::processBranch(const Stmt *Condition,
// Check for NULL conditions; e.g. "for(;;)"
if (!Condition) {
BranchNodeBuilder NullCondBldr(Pred, Dst, BldCtx, DstT, DstF);
- NullCondBldr.markInfeasible(false);
NullCondBldr.generateNode(Pred->getState(), true, Pred);
return;
}
@@ -2798,40 +2795,25 @@ void ExprEngine::processBranch(const Stmt *Condition,
if (CheckersOutSet.empty())
return;
- BranchNodeBuilder builder(CheckersOutSet, Dst, BldCtx, DstT, DstF);
+ BranchNodeBuilder Builder(CheckersOutSet, Dst, BldCtx, DstT, DstF);
for (ExplodedNode *PredN : CheckersOutSet) {
if (PredN->isSink())
continue;
ProgramStateRef PrevState = PredN->getState();
- ProgramStateRef StTrue, StFalse;
+ ProgramStateRef StTrue = PrevState, StFalse = PrevState;
if (const auto KnownCondValueAssumption = assumeCondition(Condition, PredN))
std::tie(StTrue, StFalse) = *KnownCondValueAssumption;
- else {
- assert(!isa<ObjCForCollectionStmt>(Condition));
- builder.generateNode(PrevState, true, PredN);
- builder.generateNode(PrevState, false, PredN);
- continue;
- }
+
if (StTrue && StFalse)
assert(!isa<ObjCForCollectionStmt>(Condition));
- // Process the true branch.
- if (builder.isFeasible(true)) {
- if (StTrue)
- builder.generateNode(StTrue, true, PredN);
- else
- builder.markInfeasible(true);
- }
+ if (StTrue)
+ Builder.generateNode(StTrue, true, PredN);
- // Process the false branch.
- if (builder.isFeasible(false)) {
- if (StFalse)
- builder.generateNode(StFalse, false, PredN);
- else
- builder.markInfeasible(false);
- }
+ if (StFalse)
+ Builder.generateNode(StFalse, false, PredN);
}
currBldrCtx = nullptr;
}
@@ -2853,14 +2835,13 @@ void ExprEngine::processStaticInitializer(const DeclStmt *DS,
const auto *VD = cast<VarDecl>(DS->getSingleDecl());
ProgramStateRef state = Pred->getState();
bool initHasRun = state->contains<InitializedGlobalsSet>(VD);
- BranchNodeBuilder builder(Pred, Dst, BuilderCtx, DstT, DstF);
+ BranchNodeBuilder Builder(Pred, Dst, BuilderCtx, DstT, DstF);
if (!initHasRun) {
state = state->add<InitializedGlobalsSet>(VD);
}
- builder.generateNode(state, initHasRun, Pred);
- builder.markInfeasible(!initHasRun);
+ Builder.generateNode(state, initHasRun, Pred);
currBldrCtx = nullptr;
}
More information about the cfe-commits
mailing list