https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/188096
From 6d9c0cb612008dfd0a179e53f7adf36cbb1c850c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 19 Mar 2026 15:29:08 +0100 Subject: [PATCH 01/13] [NFC] Inline node creation methods of SwitchNodeBuilder They were both called only once. I was able to replace `C.getBlock()` with `getCurrBlock()` because `C`, the context of the `SwitchNodeBuilder` instance is initialized to the current node builder context of the `ExprEngine` singleton. --- .../Core/PathSensitive/CoreEngine.h | 6 ----- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 23 ------------------- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 19 ++++++++++++--- 3 files changed, 16 insertions(+), 32 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 76bd8346f269e..b614e78cf442b 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -341,12 +341,6 @@ class SwitchNodeBuilder : public NodeBuilder { 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 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 e9522a7975515..85cff05bedc94 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3131,8 +3131,10 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, StateMatching = State; } - if (StateMatching) - Builder.generateCaseStmtNode(Block, StateMatching, Node); + if (StateMatching) { + BlockEdge BE(getCurrBlock(), Block, Node->getLocationContext()); + Builder.generateNode(BE, StateMatching, Node); + } // If _not_ entering the current case is infeasible, then we are done // with processing the paths through the current Node. @@ -3154,7 +3156,18 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, continue; } - Builder.generateDefaultCaseNode(State, Node); + // Get the block for the default case. + const CFGBlock *Src = getCurrBlock(); + 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; + + BlockEdge BE(Src, DefaultBlock, Node->getLocationContext()); + Builder.generateNode(BE, State, Node); } } From 5a35a797fc05ce332e5b3c1089775210b056b24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 19 Mar 2026 17:19:49 +0100 Subject: [PATCH 02/13] [NFC] Inline iterators of SwitchNodeBuilder --- .../StaticAnalyzer/Core/PathSensitive/CoreEngine.h | 5 ----- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 14 +++++++++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index b614e78cf442b..2388aec14b95a 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -336,11 +336,6 @@ 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(); } }; } // namespace ento diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 85cff05bedc94..9e64eb195ecc5 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3101,7 +3101,19 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, std::optional<NonLoc> CondNL = CondV.getAs<NonLoc>(); - for (const CFGBlock *Block : Builder) { + // The reversed iteration order was arbitrarily introduced in 2008 by + // commit 80ebc1d1c95704b0ff0386b3a3cbc8b3ff960654 (which added support for + // control flow in switch statements). I don't see any advantage of this + // iteration order, but changing it would change the order of insertion + // into the work list, which would perturb the analyzer results. + // FIXME: With forward iterators it would be possible to replace this + // convoluted code with a simple range-based for loop over + // CFGBlock::succs(). + using iterator = CFGBlock::const_succ_reverse_iterator; + iterator LastCase = getCurrBlock()->succ_rbegin() + 1; + iterator BeforeFirstCase = getCurrBlock()->succ_rend(); + for (iterator I = LastCase; I < BeforeFirstCase; I++) { + const CFGBlock *Block = *I; // Successor may be pruned out during CFG construction. if (!Block) continue; From b76c4caf373b95a813addc131cb1761213278c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 19 Mar 2026 17:23:32 +0100 Subject: [PATCH 03/13] [NFC] Remove SwitchNodeBuilder ...replacing its single use with a plain NodeBuilder (for now). --- .../clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h | 7 ------- .../clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h | 1 - clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 2 +- clang/test/Analysis/switch-basics.c | 2 +- 4 files changed, 2 insertions(+), 10 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 2388aec14b95a..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,12 +331,6 @@ class BranchNodeBuilder : public NodeBuilder { ExplodedNode *Pred); }; -class SwitchNodeBuilder : public NodeBuilder { -public: - SwitchNodeBuilder(ExplodedNodeSet &DstSet, const NodeBuilderContext &Ctx) - : NodeBuilder(DstSet, Ctx) {} -}; - } // 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/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 9e64eb195ecc5..28c4faf8412db 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3084,7 +3084,7 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, ExplodedNodeSet &Dst) { const Expr *Condition = Switch->getCond(); - SwitchNodeBuilder Builder(Dst, *currBldrCtx); + NodeBuilder Builder(Dst, *currBldrCtx); ExplodedNodeSet CheckersOutSet; getCheckerManager().runCheckersForBranchCondition( diff --git a/clang/test/Analysis/switch-basics.c b/clang/test/Analysis/switch-basics.c index 2a0f9c2a3cf1b..99806b2134d49 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. From ca3e5b8ac139e61911f7d810ac12ce9aee863fd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 19 Mar 2026 17:26:47 +0100 Subject: [PATCH 04/13] [NFC] Replace use of NodeBuilder with makeNode() and insert() Inline `NodeBuilder::generateNode()` to completely eliminate the `NodeBuilder` from `processSwitch`. Note that the node set `Dst` starts as an empty set and each added node is different, so the calls to `Frontier.erase` in `generateNode()` never did anything. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 28c4faf8412db..2487ab3d401ef 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3084,7 +3084,6 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, ExplodedNodeSet &Dst) { const Expr *Condition = Switch->getCond(); - NodeBuilder Builder(Dst, *currBldrCtx); ExplodedNodeSet CheckersOutSet; getCheckerManager().runCheckersForBranchCondition( @@ -3145,7 +3144,7 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, if (StateMatching) { BlockEdge BE(getCurrBlock(), Block, Node->getLocationContext()); - Builder.generateNode(BE, StateMatching, Node); + Dst.insert(Engine.makeNode(BE, StateMatching, Node)); } // If _not_ entering the current case is infeasible, then we are done @@ -3179,7 +3178,7 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, return; BlockEdge BE(Src, DefaultBlock, Node->getLocationContext()); - Builder.generateNode(BE, State, Node); + Dst.insert(Engine.makeNode(BE, State, Node)); } } From f40c3dedf25daa34f291b2116dfd392ab41f9d56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 19 Mar 2026 17:43:58 +0100 Subject: [PATCH 05/13] [NFC] Store the location context in a variable --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 2487ab3d401ef..0742b50c0f7e2 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3091,13 +3091,13 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, for (ExplodedNode *Node : CheckersOutSet) { ProgramStateRef State = Node->getState(); + const LocationContext *LCtx = Node->getLocationContext(); - 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>(); // The reversed iteration order was arbitrarily introduced in 2008 by @@ -3143,7 +3143,7 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, } if (StateMatching) { - BlockEdge BE(getCurrBlock(), Block, Node->getLocationContext()); + BlockEdge BE(getCurrBlock(), Block, LCtx); Dst.insert(Engine.makeNode(BE, StateMatching, Node)); } @@ -3177,7 +3177,7 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, if (!DefaultBlock) return; - BlockEdge BE(Src, DefaultBlock, Node->getLocationContext()); + BlockEdge BE(Src, DefaultBlock, LCtx); Dst.insert(Engine.makeNode(BE, State, Node)); } } From 3c157b8daa98204481cbf2a4452b88a5a8fc54dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Fri, 20 Mar 2026 18:27:06 +0100 Subject: [PATCH 06/13] [NFC] Introduce another variable for the AST context as well --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 0742b50c0f7e2..16e720d7cd722 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3082,6 +3082,7 @@ 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 Expr *Condition = Switch->getCond(); ExplodedNodeSet CheckersOutSet; @@ -3120,14 +3121,14 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, const CaseStmt *Case = cast<CaseStmt>(Block->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; From 51d5daedfb2a95d518478de85c3c126302b0d5ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Fri, 20 Mar 2026 18:30:13 +0100 Subject: [PATCH 07/13] [NFC] Hoist the definition of LCtx The `BranchCondition` checkers cannot change the location context. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 16e720d7cd722..11c9d1a51ac4a 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3083,6 +3083,7 @@ void ExprEngine::processEndOfFunction(ExplodedNode *Pred, void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, ExplodedNodeSet &Dst) { const ASTContext &ACtx = getContext(); + const LocationContext *LCtx = Pred->getLocationContext(); const Expr *Condition = Switch->getCond(); ExplodedNodeSet CheckersOutSet; @@ -3092,7 +3093,6 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, for (ExplodedNode *Node : CheckersOutSet) { ProgramStateRef State = Node->getState(); - const LocationContext *LCtx = Node->getLocationContext(); SVal CondV = State->getSVal(Condition, LCtx); if (CondV.isUndef()) { From 67bee70ebb80f6db00460ec03f3469ef1e204316 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 23 Mar 2026 18:34:56 +0100 Subject: [PATCH 08/13] [NFC] Unify and clarify the use of CFG blocks --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 38 +++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 11c9d1a51ac4a..b04f936ce5d6d 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3086,6 +3086,17 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, const LocationContext *LCtx = Pred->getLocationContext(); const Expr *Condition = Switch->getCond(); + // The block that is terminated by the switch statement. + const CFGBlock *SwitchBlock = getCurrBlock(); + // 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( @@ -3101,24 +3112,12 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, } std::optional<NonLoc> CondNL = CondV.getAs<NonLoc>(); - // The reversed iteration order was arbitrarily introduced in 2008 by - // commit 80ebc1d1c95704b0ff0386b3a3cbc8b3ff960654 (which added support for - // control flow in switch statements). I don't see any advantage of this - // iteration order, but changing it would change the order of insertion - // into the work list, which would perturb the analyzer results. - // FIXME: With forward iterators it would be possible to replace this - // convoluted code with a simple range-based for loop over - // CFGBlock::succs(). - using iterator = CFGBlock::const_succ_reverse_iterator; - iterator LastCase = getCurrBlock()->succ_rbegin() + 1; - iterator BeforeFirstCase = getCurrBlock()->succ_rend(); - for (iterator I = LastCase; I < BeforeFirstCase; I++) { - const CFGBlock *Block = *I; + 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(ACtx); @@ -3144,7 +3143,7 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, } if (StateMatching) { - BlockEdge BE(getCurrBlock(), Block, LCtx); + BlockEdge BE(SwitchBlock, CaseBlock, LCtx); Dst.insert(Engine.makeNode(BE, StateMatching, Node)); } @@ -3168,17 +3167,12 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, continue; } - // Get the block for the default case. - const CFGBlock *Src = getCurrBlock(); - 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; - BlockEdge BE(Src, DefaultBlock, LCtx); + BlockEdge BE(SwitchBlock, DefaultBlock, LCtx); Dst.insert(Engine.makeNode(BE, State, Node)); } } From 455a183113cdf4cb1668249d54bc385c53b1826c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 30 Mar 2026 19:09:39 +0200 Subject: [PATCH 09/13] [NFC] Re-add an assertion --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index b04f936ce5d6d..03ebd3e853da9 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3088,6 +3088,8 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, // 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 From d598af034ffb673326197ad69b79ee16123d76e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 30 Mar 2026 19:15:14 +0200 Subject: [PATCH 10/13] [NFC] Add a testcase for empty switches I don't think that these would cause any problem, but let's be a bit paranoid :) --- clang/test/Analysis/switch-basics.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/clang/test/Analysis/switch-basics.c b/clang/test/Analysis/switch-basics.c index 99806b2134d49..233420f2ff26e 100644 --- a/clang/test/Analysis/switch-basics.c +++ b/clang/test/Analysis/switch-basics.c @@ -93,6 +93,19 @@ int switch_no_compound_stmt(int x) { return 0; } +int switch_empty(int x) { + // Validate that the engine does not crash on these "empty" switches. + // (These are pretty useless and the second is reported by a compiler + // warning, but the analyzer should still be prepared to handle them.) + + switch (x) {} + + switch (x); // expected-warning {{Switch statement has empty body}} + + return 0; +} + + int switch_with_case_range(int x) { // Validate that the GNU case range extension is properly handled. switch (x) { From e4cf217c77650799c08499664b1e68f2909fbec5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 30 Mar 2026 19:37:45 +0200 Subject: [PATCH 11/13] [NFC] Fix broken new testcase, avoid compiler warning --- clang/test/Analysis/switch-basics.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/test/Analysis/switch-basics.c b/clang/test/Analysis/switch-basics.c index 233420f2ff26e..c92dc3ea4dd6c 100644 --- a/clang/test/Analysis/switch-basics.c +++ b/clang/test/Analysis/switch-basics.c @@ -94,13 +94,13 @@ int switch_no_compound_stmt(int x) { } int switch_empty(int x) { - // Validate that the engine does not crash on these "empty" switches. - // (These are pretty useless and the second is reported by a compiler - // warning, but the analyzer should still be prepared to handle them.) + // 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); // expected-warning {{Switch statement has empty body}} + switch (x) + ; return 0; } From ddc87a012ddf85552d341dff0c6d0d6c08bfa05d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 1 Apr 2026 12:41:43 +0200 Subject: [PATCH 12/13] [NFC] Shorten the new test --- clang/test/Analysis/switch-basics.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/clang/test/Analysis/switch-basics.c b/clang/test/Analysis/switch-basics.c index c92dc3ea4dd6c..72c728eecacc1 100644 --- a/clang/test/Analysis/switch-basics.c +++ b/clang/test/Analysis/switch-basics.c @@ -93,7 +93,7 @@ int switch_no_compound_stmt(int x) { return 0; } -int switch_empty(int x) { +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.) @@ -101,11 +101,8 @@ int switch_empty(int x) { switch (x) ; - - return 0; } - int switch_with_case_range(int x) { // Validate that the GNU case range extension is properly handled. switch (x) { From ffbb916d02357084bb4104d0c9c0c1d72623df8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 1 Apr 2026 13:09:43 +0200 Subject: [PATCH 13/13] Fix handling of the "optimized out" default block The default block (i.e. the last successor of the CFG block terminated by the `switch`) can be null if it is "optimized out", i.e. it is clear during CFG creation that this transition is infeasible. In this case we should (obviously) not generate an edge to this null block. This situation was handled by an early return in the method `SwitchNodeBuilder::generateDefaultCaseNode` -- and I made a logic error when I inlined that method into `processSwitch` and kept the `return` statement. Most of the logic in `processSwitch` is contained within the body of `for (ExplodedNode *Node : CheckersOutSet)`, so the presence of this erronous early return meant that in `switch`es where the default block is null (= optimized out) the analyzer would use only the first `Node` from `CheckersOutSet` (because the early return skips the rest of the iterations). This ugly logical error wouldn't have caused practical problems right now, because curretnly there is no `BranchCondition` checker that "splits the state" and places more than one node in `CheckersOutSet`. Still, this commit restores logical correctness by replacing the `return` with a `continue` statement. Additionally, the comment before this `!DefaultBlock` check is clarified (I don't exactly know what are the "earlier stages" mentioned in the old comment) and this check is placed before the other, more complex check that skips the default branch when all enumerators are covered by `case:` branches. (The order of these two block doesn't matter, but I think it is more elegant to check the simple one first.) --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 03ebd3e853da9..4b24ac61337da 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3157,6 +3157,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. @@ -3169,11 +3173,6 @@ void ExprEngine::processSwitch(const SwitchStmt *Switch, ExplodedNode *Pred, continue; } - // Basic correctness check for default blocks that are unreachable and not - // caught by earlier stages. - if (!DefaultBlock) - return; - BlockEdge BE(SwitchBlock, DefaultBlock, LCtx); Dst.insert(Engine.makeNode(BE, State, Node)); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
