llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/117898.diff 3 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h (+6-26) - (modified) clang/lib/StaticAnalyzer/Core/CoreEngine.cpp (+6-5) - (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+9-28) ``````````diff 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; } `````````` </details> https://github.com/llvm/llvm-project/pull/117898 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
