https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/183540
Simplify the code of `CoreEngine` by using the method `CoreEngine::makeNode` (which was introduced recently in commit 1a81673922a3f5b75f342e416e925a69822c4613) and removing two similar, but less practical utility methods. This is technically speaking not an NFC change, because it no longer allows generating non-sink nodes with `PosteriorlyOverconstrained` state in these locations (which was logically unsound previously). However I labelled this as an NFCI change, as I'm fairly confident that this won't lead to any observable changes because: - `PosteriorlyOverconstrained` states are very rare. - A state derived from a `PosteriorlyOverconstrained` state is also posteriorly overconstrained, so even without this commit "surviving" `PosteriorlyOverconstrained` paths would been sunk anyway within a few steps. Note that simulated paths with a `PosteriorlyOverconstrained` state do not correspond to real paths that can occur during the actual execution of the program, so the analyzer should stop following them as soon as possible. For more explanation see the doc-comment above the data member `bool PosteriorlyOverconstrained` within `ProgramState`. ----- Note for reviewers: The steps of this refactoring are preserved as individual commits within the PR. This PR is orthogonal to https://github.com/llvm/llvm-project/pull/183354 and is a bit less complex than it. From 42c533c831f465e1da49e962789ffbaa2dd88e2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 26 Feb 2026 14:17:38 +0100 Subject: [PATCH 1/5] [analyzer] Use CoreEngine::makeNode in similar methods Simplify a few methods of `CoreEngine` by calling the method `CoreEngine::makeNode` (which was introduced recently in commit 1a81673922a3f5b75f342e416e925a69822c4613) instead of direct manipulation of the `ExplodedGraph`. These transformations are _almost_ NFC, the only difference is that after this commit these methods won't generate non-sink nodes with `PosteriorlyOverconstrained` state. (Previously they were able to do so in theory, which was logically unsound.) However, I'm fairly confident that this change won't cause observable changes because `PosteriorlyOverconstrained` states are very rare and later transitions would sink those "impossible" paths anyway. For more information see 1a81673922a3f5b75f342e416e925a69822c4613 and (which also does a similar change) and the doc-comment above the data member `bool PosteriorlyOverconstrained` within `ProgramState`. --- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 22 ++++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index 29bd103e57d2c..b482c1b32df31 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -579,20 +579,15 @@ ExplodedNode *CoreEngine::makeNode(const ProgramPoint &Loc, /// generateNode - Utility method to generate nodes, hook up successors, /// and add nodes to the worklist. -/// TODO: This and other similar methods should call CoreEngine::makeNode() -/// instead of duplicating its logic. This would also fix that currently these -/// can generate non-sink nodes with PosteriorlyOverconstrained state. void CoreEngine::generateNode(const ProgramPoint &Loc, ProgramStateRef State, ExplodedNode *Pred) { assert(Pred); - bool IsNew; - ExplodedNode *Node = G.getNode(Loc, State, false, &IsNew); - - Node->addPredecessor(Pred, G); // Link 'Node' with its predecessor. + ExplodedNode *Node = makeNode(Loc, State, Pred); // Only add 'Node' to the worklist if it was freshly generated. - if (IsNew) WList->enqueue(Node); + if (Node) + WList->enqueue(Node); } void CoreEngine::enqueueStmtNode(ExplodedNode *N, @@ -637,11 +632,9 @@ void CoreEngine::enqueueStmtNode(ExplodedNode *N, return; } - bool IsNew; - ExplodedNode *Succ = G.getNode(Loc, N->getState(), false, &IsNew); - Succ->addPredecessor(N, G); + ExplodedNode *Succ = makeNode(Loc, N->getState(), N); - if (IsNew) + if (Succ) WList->enqueue(Succ, Block, Idx+1); } @@ -653,10 +646,7 @@ ExplodedNode *CoreEngine::generateCallExitBeginNode(ExplodedNode *N, // Use the callee location context. CallExitBegin Loc(LocCtx, RS); - bool isNew; - ExplodedNode *Node = G.getNode(Loc, N->getState(), false, &isNew); - Node->addPredecessor(N, G); - return isNew ? Node : nullptr; + return makeNode(Loc, N->getState(), N); } std::optional<unsigned> From a653be52005e346dcd62846560ace1753a72ab2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 26 Feb 2026 14:44:33 +0100 Subject: [PATCH 2/5] [NFC][analyzer] Eliminate generateCallExitBeginNode This method of `CoreEngine` was very specific, had only one call site and after the previous simplification it became so trivial that I decided to iniline it. This method claimed to "Create a CallExitBegin node and enqueue it." -- but the "enqueue" part was already a lie when it was introduced (through copypasting) by 0ec04bf73885df3e10bd7fcd5c8ce901cad7d76c in 2011. --- .../Core/PathSensitive/CoreEngine.h | 3 --- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 18 ++++++------------ 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index a8b33f22fcc33..449036677c0e6 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -121,9 +121,6 @@ class CoreEngine { void HandleVirtualBaseBranch(const CFGBlock *B, ExplodedNode *Pred); private: - ExplodedNode *generateCallExitBeginNode(ExplodedNode *N, - const ReturnStmt *RS); - /// Helper function called by `HandleBranch()`. If the currently handled /// branch corresponds to a loop, this returns the number of already /// completed iterations in that loop, otherwise the return value is diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index b482c1b32df31..f74e3e8ad6ab3 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -638,17 +638,6 @@ void CoreEngine::enqueueStmtNode(ExplodedNode *N, WList->enqueue(Succ, Block, Idx+1); } -ExplodedNode *CoreEngine::generateCallExitBeginNode(ExplodedNode *N, - const ReturnStmt *RS) { - // Create a CallExitBegin node and enqueue it. - const auto *LocCtx = cast<StackFrameContext>(N->getLocationContext()); - - // Use the callee location context. - CallExitBegin Loc(LocCtx, RS); - - return makeNode(Loc, N->getState(), N); -} - std::optional<unsigned> CoreEngine::getCompletedIterationCount(const CFGBlock *B, ExplodedNode *Pred) const { @@ -688,7 +677,12 @@ void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet &Set, const ReturnStmt *RS for (auto *I : Set) { // If we are in an inlined call, generate CallExitBegin node. if (I->getLocationContext()->getParent()) { - I = generateCallExitBeginNode(I, RS); + const auto *LocCtx = cast<StackFrameContext>(I->getLocationContext()); + + // Use the callee location context. + CallExitBegin Loc(LocCtx, RS); + + I = makeNode(Loc, I->getState(), I); if (I) WList->enqueue(I); } else { From e8be9512ae8476e607f8f2a1b3c2e4df6fca9dc5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 26 Feb 2026 15:14:28 +0100 Subject: [PATCH 3/5] [NFC][analyzer] Prettify CoreEngine::enqueueEndOfFunction After inlining a function into its body. --- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index f74e3e8ad6ab3..bf8c5680a36f9 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -674,20 +674,18 @@ void CoreEngine::enqueue(ExplodedNodeSet &Set, } void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet &Set, const ReturnStmt *RS) { - for (auto *I : Set) { - // If we are in an inlined call, generate CallExitBegin node. - if (I->getLocationContext()->getParent()) { - const auto *LocCtx = cast<StackFrameContext>(I->getLocationContext()); + for (ExplodedNode *Node : Set) { + const LocationContext *LocCtx = Node->getLocationContext(); + // If we are in an inlined call, generate CallExitBegin node. + if (LocCtx->getParent()) { // Use the callee location context. - CallExitBegin Loc(LocCtx, RS); - - I = makeNode(Loc, I->getState(), I); - if (I) - WList->enqueue(I); + CallExitBegin Loc(cast<StackFrameContext>(LocCtx), RS); + if (ExplodedNode *Succ = makeNode(Loc, Node->getState(), Node)) + WList->enqueue(Succ); } else { // TODO: We should run remove dead bindings here. - G.addEndOfPath(I); + G.addEndOfPath(Node); NumPathsExplored++; } } From a14242cefde056248114c0f3fdc7efc79ad15e36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 26 Feb 2026 15:30:11 +0100 Subject: [PATCH 4/5] [NFC][analyzer] Eliminate CoreEngine::generateNode This method of `CoreEngine` was unfortunately named (easy to confuse with `generateNode` methods of `NodeBuilder`s), had only two call sites and after the previous simplification it became so trivial that I decided to iniline it. Note that `assert(Pred)` was redundant -- that argument was dereferenced multiple times at both callsites before the call. --- .../Core/PathSensitive/CoreEngine.h | 4 ---- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 23 +++++-------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 449036677c0e6..759f15d79b604 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -97,10 +97,6 @@ class CoreEngine { void setBlockCounter(BlockCounter C); - void generateNode(const ProgramPoint &Loc, - ProgramStateRef State, - ExplodedNode *Pred); - void HandleBlockEdge(const BlockEdge &E, ExplodedNode *Pred); void HandleBlockEntrance(const BlockEntrance &E, ExplodedNode *Pred); void HandleBlockExit(const CFGBlock *B, ExplodedNode *Pred); diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index bf8c5680a36f9..f1593412e3599 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -394,8 +394,9 @@ void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) { for (CFGBlock::const_succ_iterator it = B->succ_begin(), et = B->succ_end(); it != et; ++it) { if (const CFGBlock *succ = *it) { - generateNode(BlockEdge(B, succ, Pred->getLocationContext()), - Pred->State, Pred); + BlockEdge BE(B, succ, Pred->getLocationContext()); + if (ExplodedNode *N = makeNode(BE, Pred->State, Pred)) + WList->enqueue(N); } } return; @@ -479,8 +480,9 @@ void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) { assert(B->succ_size() == 1 && "Blocks with no terminator should have at most 1 successor."); - generateNode(BlockEdge(B, *(B->succ_begin()), Pred->getLocationContext()), - Pred->State, Pred); + BlockEdge BE(B, *(B->succ_begin()), Pred->getLocationContext()); + if (ExplodedNode *N = makeNode(BE, Pred->State, Pred)) + WList->enqueue(N); } void CoreEngine::HandleCallEnter(const CallEnter &CE, ExplodedNode *Pred) { @@ -577,19 +579,6 @@ ExplodedNode *CoreEngine::makeNode(const ProgramPoint &Loc, return IsNew ? N : nullptr; } -/// generateNode - Utility method to generate nodes, hook up successors, -/// and add nodes to the worklist. -void CoreEngine::generateNode(const ProgramPoint &Loc, - ProgramStateRef State, - ExplodedNode *Pred) { - assert(Pred); - ExplodedNode *Node = makeNode(Loc, State, Pred); - - // Only add 'Node' to the worklist if it was freshly generated. - if (Node) - WList->enqueue(Node); -} - void CoreEngine::enqueueStmtNode(ExplodedNode *N, const CFGBlock *Block, unsigned Idx) { assert(Block); From fd8696596f751289f91b7e5c3ee6788afcad0f5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 26 Feb 2026 15:35:37 +0100 Subject: [PATCH 5/5] [NFC][analyzer] Use range-based for loop --- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index f1593412e3599..f0f2a7619de0b 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -391,10 +391,9 @@ void CoreEngine::HandleBlockExit(const CFGBlock * B, ExplodedNode *Pred) { case Stmt::CXXTryStmtClass: // Generate a node for each of the successors. // Our logic for EH analysis can certainly be improved. - for (CFGBlock::const_succ_iterator it = B->succ_begin(), - et = B->succ_end(); it != et; ++it) { - if (const CFGBlock *succ = *it) { - BlockEdge BE(B, succ, Pred->getLocationContext()); + for (const CFGBlock *Succ : B->succs()) { + if (Succ) { + BlockEdge BE(B, Succ, Pred->getLocationContext()); if (ExplodedNode *N = makeNode(BE, Pred->State, Pred)) WList->enqueue(N); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
