Author: Donát Nagy Date: 2026-04-03T09:46:06+02:00 New Revision: c80443cd37b2e2788cba67ffa180a6331e5f0791
URL: https://github.com/llvm/llvm-project/commit/c80443cd37b2e2788cba67ffa180a6331e5f0791 DIFF: https://github.com/llvm/llvm-project/commit/c80443cd37b2e2788cba67ffa180a6331e5f0791.diff LOG: [NFC][analyzer] Eliminate SwitchNodeBuilder (#188096) This commit removes the class `SwitchNodeBuilder` because it just obscured the logic of switch handling by hiding some parts of it in another source file. Added: Modified: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h clang/lib/StaticAnalyzer/Core/CoreEngine.cpp clang/lib/StaticAnalyzer/Core/ExprEngine.cpp clang/test/Analysis/switch-basics.c Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 76bd8346f269e..7dfdca0d54a67 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -51,7 +51,6 @@ class CoreEngine { friend class ExprEngine; friend class NodeBuilder; friend class NodeBuilderContext; - friend class SwitchNodeBuilder; public: using BlocksExhausted = @@ -332,23 +331,6 @@ class BranchNodeBuilder : public NodeBuilder { ExplodedNode *Pred); }; -class SwitchNodeBuilder : public NodeBuilder { -public: - SwitchNodeBuilder(ExplodedNodeSet &DstSet, const NodeBuilderContext &Ctx) - : NodeBuilder(DstSet, Ctx) {} - - using iterator = CFGBlock::const_succ_reverse_iterator; - - iterator begin() { return C.getBlock()->succ_rbegin() + 1; } - iterator end() { return C.getBlock()->succ_rend(); } - - ExplodedNode *generateCaseStmtNode(const CFGBlock *Block, - ProgramStateRef State, ExplodedNode *Pred); - - ExplodedNode *generateDefaultCaseNode(ProgramStateRef State, - ExplodedNode *Pred); -}; - } // namespace ento } // namespace clang diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index c346c20a7af87..746354417be66 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -89,7 +89,6 @@ class ProgramState; class ProgramStateManager; class RegionAndSymbolInvalidationTraits; class SymbolManager; -class SwitchNodeBuilder; /// Hints for figuring out of a call should be inlined during evalCall(). struct EvalCallOptions { diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index a348f7947ad98..17bee557fe029 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -707,26 +707,3 @@ ExplodedNode *BranchNodeBuilder::generateNode(ProgramStateRef State, ExplodedNode *Succ = NodeBuilder::generateNode(Loc, State, NodePred); return Succ; } - -ExplodedNode *SwitchNodeBuilder::generateCaseStmtNode(const CFGBlock *Block, - ProgramStateRef St, - ExplodedNode *Pred) { - BlockEdge BE(C.getBlock(), Block, Pred->getLocationContext()); - return generateNode(BE, St, Pred); -} - -ExplodedNode *SwitchNodeBuilder::generateDefaultCaseNode(ProgramStateRef St, - ExplodedNode *Pred) { - // Get the block for the default case. - const CFGBlock *Src = C.getBlock(); - assert(Src->succ_rbegin() != Src->succ_rend()); - CFGBlock *DefaultBlock = *Src->succ_rbegin(); - - // Basic correctness check for default blocks that are unreachable and not - // caught by earlier stages. - if (!DefaultBlock) - return nullptr; - - BlockEdge BE(Src, DefaultBlock, Pred->getLocationContext()); - return generateNode(BE, St, Pred); -} diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index b6d2c96627520..ca544aad46e0a 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3083,9 +3083,23 @@ void ExprEngine::processEndOfFunction(ExplodedNode *Pred, /// nodes by processing the 'effects' of a switch statement. void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, ExplodedNodeSet &Dst) { + const ASTContext &ACtx = getContext(); + const LocationContext *LCtx = Pred->getLocationContext(); const Expr *Condition = Switch->getCond(); - SwitchNodeBuilder Builder(Dst, *currBldrCtx); + // The block that is terminated by the switch statement. + const CFGBlock *SwitchBlock = getCurrBlock(); + // Note that successors may be null if they are pruned as unreachable. + assert(SwitchBlock->succ_size() && "Switch must have at least one successor"); + // The reversed iteration order is present since the beginning, when in 2008 + // commit 80ebc1d1c95704b0ff0386b3a3cbc8b3ff960654 added support for handling + // switch statements. I don't see any advantage over regular forward + // iteration -- but switching the order would perturb the insertion order of + // the work list and therefore the analysis results. + llvm::iterator_range<CFGBlock::const_succ_reverse_iterator> CaseBlocks( + SwitchBlock->succ_rbegin() + 1, SwitchBlock->succ_rend()); + const CFGBlock *DefaultBlock = *SwitchBlock->succ_rbegin(); + ExplodedNodeSet CheckersOutSet; getCheckerManager().runCheckersForBranchCondition( @@ -3094,30 +3108,29 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, for (ExplodedNode *Node : CheckersOutSet) { ProgramStateRef State = Node->getState(); - SVal CondV = State->getSVal(Condition, Node->getLocationContext()); + SVal CondV = State->getSVal(Condition, LCtx); if (CondV.isUndef()) { // This can only happen if core.uninitialized.Branch is disabled. continue; } - std::optional<NonLoc> CondNL = CondV.getAs<NonLoc>(); - for (const CFGBlock *Block : Builder) { + for (const CFGBlock *CaseBlock : CaseBlocks) { // Successor may be pruned out during CFG construction. - if (!Block) + if (!CaseBlock) continue; - const CaseStmt *Case = cast<CaseStmt>(Block->getLabel()); + const CaseStmt *Case = cast<CaseStmt>(CaseBlock->getLabel()); // Evaluate the LHS of the case value. - llvm::APSInt V1 = Case->getLHS()->EvaluateKnownConstInt(getContext()); + llvm::APSInt V1 = Case->getLHS()->EvaluateKnownConstInt(ACtx); assert(V1.getBitWidth() == getContext().getIntWidth(Condition->getType())); // Get the RHS of the case, if it exists. llvm::APSInt V2; if (const Expr *E = Case->getRHS()) - V2 = E->EvaluateKnownConstInt(getContext()); + V2 = E->EvaluateKnownConstInt(ACtx); else V2 = V1; @@ -3132,8 +3145,10 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, StateMatching = State; } - if (StateMatching) - Builder.generateCaseStmtNode(Block, StateMatching, Node); + if (StateMatching) { + BlockEdge BE(SwitchBlock, CaseBlock, LCtx); + Dst.insert(Engine.makeNode(BE, StateMatching, Node)); + } // If _not_ entering the current case is infeasible, then we are done // with processing the paths through the current Node. @@ -3143,6 +3158,10 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, if (!State) continue; + // The default block may be null if it is "optimized out" by CFG creation. + if (!DefaultBlock) + continue; + // If we have switch(enum value), the default branch is not // feasible if all of the enum constants not covered by 'case:' statements // are not feasible values for the switch condition. @@ -3155,7 +3174,8 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, continue; } - Builder.generateDefaultCaseNode(State, Node); + BlockEdge BE(SwitchBlock, DefaultBlock, LCtx); + Dst.insert(Engine.makeNode(BE, State, Node)); } } diff --git a/clang/test/Analysis/switch-basics.c b/clang/test/Analysis/switch-basics.c index 2a0f9c2a3cf1b..72c728eecacc1 100644 --- a/clang/test/Analysis/switch-basics.c +++ b/clang/test/Analysis/switch-basics.c @@ -1,6 +1,6 @@ // RUN: %clang_analyze_cc1 -verify %s -analyzer-checker=core -// This file tests ExprEngine::processSwitch and the class SwitchNodeBuilder. +// This file tests ExprEngine::processSwitch. int switch_simple(int x) { // Validate that switch behaves as expected in a very simple situation. @@ -93,6 +93,16 @@ int switch_no_compound_stmt(int x) { return 0; } +void switch_empty(int x) { + // Validate that the engine does not crash on these empty switches. + // (These are pretty useless, but the analyzer should still handle them.) + + switch (x) {} + + switch (x) + ; +} + int switch_with_case_range(int x) { // Validate that the GNU case range extension is properly handled. switch (x) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
