https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/217319
This change concludes the removal of the class `NodeBuilder` which previously added lots of unnecessary complications to the logic of the analyzer engine. The main "feature" of a `NodeBuilder` was that it tracked a "frontier" set of exploded nodes, which were freshly created and not yet superseded by the creation of another node. This was contraproductive in almost all code that used `NodeBuilder`s -- with the exception of `CheckerContext` where this was useful to support arbitrary chains of `addTransition` calls in checkers. As earlier commits removed the contraproductive use of `NodeBuilder`s, there was only one surviving `NodeBuilder`, a data member of `CheckerContext`, and its `generateNode` method was called only once, so this commit inlines still relevant fragments of `NodeBuilder` into `CheckerContext` and removes `NodeBuilder` as a separate class. This change also applies trivial code quality improvements (e.g. fixing typos) in the surrounding code. The class `NodeBuilderContext` will be removed soon by a follow-up commit. From 27d7b9ceb7e78216f715901653092e932bdf6bd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 17 Aug 2026 17:48:48 +0200 Subject: [PATCH 01/10] Eliminate hack that ensured early destruction of CheckerContext A long time ago `CheckerContext` had a nontrivial destructor which populated some node set, but this was (thankfully) eliminated a long time ago, so there is no need to introduce a local scope for the `CheckerContext` instance in `runCheckersForEvalCall`. This reverts commit 8c57c4ba277190eebbb0f9f55e10896febe12e09 from 2011. --- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index 4db6b6ecaa9f7..6314b5310fe59 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -774,13 +774,9 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, ProgramPoint L = ProgramPoint::getProgramPoint( UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind, Pred->getStackFrame(), EvalCallChecker.Checker); - bool evaluated = false; - { // CheckerContext generates transitions (populates checkDest) on - // destruction, so introduce the scope to make sure it gets properly - // populated. - CheckerContext C(B, Eng, Pred, L); - evaluated = EvalCallChecker(*UpdatedCall, C); - } + + CheckerContext C(B, Eng, Pred, L); + bool evaluated = EvalCallChecker(*UpdatedCall, C); #ifndef NDEBUG if (evaluated && evaluatorChecker) { const auto toString = [](const CallEvent &Call) -> std::string { From 558b2e9f42d8fcf9d49309fac61befe932502e99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 18 Aug 2026 16:10:18 +0200 Subject: [PATCH 02/10] Capitalize variable names in CheckerContext constructor It will be rewritten by follow-up changes, so let's clean it up. --- .../Core/PathSensitive/CheckerContext.h | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 18e55862bc855..593cb4e0d794c 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -38,20 +38,13 @@ class CheckerContext { /// call was inlined. In all other cases it will be false. const bool wasInlined; - CheckerContext(NodeBuilder &builder, - ExprEngine &eng, - ExplodedNode *pred, - const ProgramPoint &loc, - bool wasInlined = false) - : Eng(eng), - Pred(pred), - Changed(false), - Location(loc), - NB(builder), - wasInlined(wasInlined) { + CheckerContext(NodeBuilder &Builder, ExprEngine &Eng, ExplodedNode *Pred, + const ProgramPoint &Loc, bool WasInlined = false) + : Eng(Eng), Pred(Pred), Changed(false), Location(Loc), NB(Builder), + wasInlined(WasInlined) { assert(Pred->getState() && "We should not call the checkers on an empty state."); - assert(loc.getTag() && "The ProgramPoint associated with CheckerContext " + assert(Loc.getTag() && "The ProgramPoint associated with CheckerContext " "must be tagged with the active checker."); } From 7446860bae92b2070bdb51fda26ba7e7af7550e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 18 Aug 2026 16:46:35 +0200 Subject: [PATCH 03/10] Remove NodeBuilder from signature of CheckerContext constructor As a step in the gradual removal of the class `NodeBuilder`, remove it from the signature of the constructor of `CheckerContext`. This commit relies on the fact that the only state of a `NodeBuilder` instance is the identity of its `Frontier` and if two `NodeBuilder` instances share the same `Frontier`, then it does not matter which instance is used. --- .../Core/PathSensitive/CheckerContext.h | 8 +++--- .../Core/PathSensitive/CoreEngine.h | 4 +++ .../StaticAnalyzer/Core/CheckerManager.cpp | 27 ++++++++++--------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 593cb4e0d794c..de4be62b328ea 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -31,17 +31,17 @@ class CheckerContext { bool Changed; /// The tagged location, which is used to generate all new nodes. const ProgramPoint Location; - NodeBuilder &NB; + NodeBuilder NB; public: /// If we are post visiting a call, this flag will be set if the /// call was inlined. In all other cases it will be false. const bool wasInlined; - CheckerContext(NodeBuilder &Builder, ExprEngine &Eng, ExplodedNode *Pred, + CheckerContext(ExprEngine &Eng, ExplodedNode *Pred, ExplodedNodeSet &Dst, const ProgramPoint &Loc, bool WasInlined = false) - : Eng(Eng), Pred(Pred), Changed(false), Location(Loc), NB(Builder), - wasInlined(WasInlined) { + : Eng(Eng), Pred(Pred), Changed(false), Location(Loc), + NB(Dst, Eng.getBuilderContext()), wasInlined(WasInlined) { assert(Pred->getState() && "We should not call the checkers on an empty state."); assert(Loc.getTag() && "The ProgramPoint associated with CheckerContext " diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 2a7264009b076..f9326f5b7855c 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -321,6 +321,10 @@ class NodeBuilder { return generateSink(L, St, Pred); } + // This is introduced temporarily to allow the gradual removal of + // NodeBuilders from the checker callback invocation logic. + ExplodedNodeSet &getFrontier() { return Frontier; } + const ExplodedNodeSet &getResults() const { return Frontier; } void takeNodes(const ExplodedNodeSet &S) { diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index 6314b5310fe59..c4c32d81099c2 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -167,7 +167,7 @@ std::string checkerScopeName(StringRef Name, const CheckerBackend *Checker) { ProgramPoint::PostStmtKind; const ProgramPoint &L = ProgramPoint::getProgramPoint( S, K, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L, WasInlined); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); checkFn(S, C); } }; @@ -227,7 +227,7 @@ namespace { } const ProgramPoint &L = Msg.getProgramPoint(IsPreVisit,checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L, WasInlined); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); checkFn(*Msg.cloneWithState<ObjCMethodCall>(Pred->getState()), C); } @@ -287,7 +287,7 @@ namespace { NodeBuilder &Bldr, ExplodedNode *Pred) { llvm::TimeTraceScope TimeScope(checkerScopeName("Call", checkFn.Checker)); const ProgramPoint &L = Call.getProgramPoint(IsPreVisit,checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L, WasInlined); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); checkFn(*Call.cloneWithState(Pred->getState()), C); } @@ -332,7 +332,7 @@ struct CheckLifetimeEndContext { NodeBuilder &Bldr, ExplodedNode *Pred) { assert(Pred->getLocation().getAs<LifetimeEnd>().has_value()); const ProgramPoint L = Pred->getLocation().withTag(checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); checkFn(Decl, C); } }; @@ -378,7 +378,7 @@ namespace { ProgramPoint::PreStoreKind; const ProgramPoint &L = ProgramPoint::getProgramPoint( NodeEx, K, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); checkFn(Loc, IsLoad, BoundEx, C); } }; @@ -427,7 +427,7 @@ namespace { NodeBuilder &Bldr, ExplodedNode *Pred) { llvm::TimeTraceScope TimeScope(checkerScopeName("Bind", checkFn.Checker)); const ProgramPoint &L = PP.withTag(checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); checkFn(Loc, Val, S, AtDeclInit, C); } @@ -476,7 +476,8 @@ struct CheckBlockEntranceContext { ExplodedNode *Pred) { llvm::TimeTraceScope TimeScope( checkerScopeName("BlockEntrance", CheckFn.Checker)); - CheckerContext C(Bldr, Eng, Pred, Entrance.withTag(CheckFn.Checker)); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), + Entrance.withTag(CheckFn.Checker)); CheckFn(Entrance, C); } }; @@ -519,7 +520,7 @@ struct CheckBeginFunctionContext { NodeBuilder &Bldr, ExplodedNode *Pred) { llvm::TimeTraceScope TimeScope(checkerScopeName("Begin", checkFn.Checker)); const ProgramPoint &L = PP.withTag(checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); checkFn(C); } @@ -552,7 +553,7 @@ void CheckerManager::runCheckersForEndFunction(ExplodedNodeSet &Dst, for (const auto &checkFn : EndFunctionCheckers) { const ProgramPoint &L = FunctionExitPoint(RS, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Dst, L); llvm::TimeTraceScope TimeScope(checkerScopeName("End", checkFn.Checker)); checkFn(RS, C); } @@ -580,7 +581,7 @@ namespace { checkerScopeName("BranchCond", checkFn.Checker)); ProgramPoint L = PostCondition(Condition, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); checkFn(Condition, C); } }; @@ -624,7 +625,7 @@ namespace { checkerScopeName("Allocator", checkFn.Checker)); ProgramPoint L = PostAllocatorCall( Call.getOriginExpr(), Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L, WasInlined); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); checkFn(cast<CXXAllocatorCall>(*Call.cloneWithState(Pred->getState())), C); } @@ -676,7 +677,7 @@ namespace { checkerScopeName("DeadSymbols", checkFn.Checker)); const ProgramPoint &L = ProgramPoint::getProgramPoint( S, ProgarmPointKind, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Bldr, Eng, Pred, L); + CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); // Note, do not pass the statement to the checkers without letting them // differentiate if we ran remove dead bindings before or after the @@ -775,7 +776,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind, Pred->getStackFrame(), EvalCallChecker.Checker); - CheckerContext C(B, Eng, Pred, L); + CheckerContext C(Eng, Pred, checkDst, L); bool evaluated = EvalCallChecker(*UpdatedCall, C); #ifndef NDEBUG if (evaluated && evaluatorChecker) { From bde6b4919a12dc9533e69438257a69e8d80c4eb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 18 Aug 2026 17:15:34 +0200 Subject: [PATCH 04/10] [side] Fix typo in name 'ProgarmPointKind' --- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index c4c32d81099c2..d0f2937141950 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -661,12 +661,12 @@ namespace { SymbolReaper &SR; const Stmt *S; ExprEngine &Eng; - ProgramPoint::Kind ProgarmPointKind; + ProgramPoint::Kind ProgramPointKind; CheckDeadSymbolsContext(const CheckersTy &checkers, SymbolReaper &sr, const Stmt *s, ExprEngine &eng, ProgramPoint::Kind K) - : Checkers(checkers), SR(sr), S(s), Eng(eng), ProgarmPointKind(K) {} + : Checkers(checkers), SR(sr), S(s), Eng(eng), ProgramPointKind(K) {} CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); } CheckersTy::const_iterator checkers_end() { return Checkers.end(); } @@ -676,7 +676,7 @@ namespace { llvm::TimeTraceScope TimeScope( checkerScopeName("DeadSymbols", checkFn.Checker)); const ProgramPoint &L = ProgramPoint::getProgramPoint( - S, ProgarmPointKind, Pred->getStackFrame(), checkFn.Checker); + S, ProgramPointKind, Pred->getStackFrame(), checkFn.Checker); CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); // Note, do not pass the statement to the checkers without letting them From 23ff297a9c8c3c18cce03fdbb9629b12880bea1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 18 Aug 2026 18:01:37 +0200 Subject: [PATCH 05/10] Remove NodeBuilders that became useless As the previous commit removed the `NodeBuilder` parameter from the constructor of `CheckerContext`, the `NodeBuilder` instances in `runCheckersForEndFunction` and `runCheckersForEvalCall` became unused, so this commit removes them -- but keeps the side effect of their constructor (inserting the source / pred argument into the destination). --- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index d0f2937141950..ebf8c360de818 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -549,7 +549,7 @@ void CheckerManager::runCheckersForEndFunction(ExplodedNodeSet &Dst, // We define the builder outside of the loop because if at least one checker // creates a successor for Pred, we do not need to generate an // autotransition for it. - NodeBuilder Bldr(Pred, Dst, Eng.getBuilderContext()); + Dst.insert(Pred); for (const auto &checkFn : EndFunctionCheckers) { const ProgramPoint &L = FunctionExitPoint(RS, Pred->getStackFrame(), checkFn.Checker); @@ -762,8 +762,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, for (auto *const Pred : Src) { std::optional<StringRef> evaluatorChecker; - ExplodedNodeSet checkDst; - NodeBuilder B(Pred, checkDst, Eng.getBuilderContext()); + ExplodedNodeSet checkDst{Pred}; ProgramStateRef State = Pred->getState(); CallEventRef<> UpdatedCall = Call.cloneWithState(State); From 036a27b070372b1457bba505438020532a42892a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 18 Aug 2026 18:06:31 +0200 Subject: [PATCH 06/10] [side] Improve comments on runCheckersForEndFunction `CheckerManager::runCheckersForEndFunction` has an unusual behavior (activates checkers in parallel, not sequentially), because it was originally an "end of path" callback and according to an old comment the old "end of path" checkers expected that their transition is the final one on the path. This change rewrites the doc-comment to highlight this difference and question whether it is still justified now that this callback is also activates at the end of inlined functions. --- .../StaticAnalyzer/Core/CheckerManager.cpp | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index ebf8c360de818..451945e4db554 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -539,17 +539,31 @@ void CheckerManager::runCheckersForBeginFunction(ExplodedNodeSet &Dst, expandGraphWithCheckers(C, Dst, Src); } -/// Run checkers for end of path. -// Note, We do not chain the checker output (like in expandGraphWithCheckers) -// for this callback since end of path nodes are expected to be final. +/// Run checkers for end of a function (either the entrypoint or another +/// function that was inlined). Note that this function places the +/// checker activations on separate execution paths: +/// /-[checker1]-> N1 ... +/// Pred --[checker2]-> N2 ... +/// \-[checker3]-> N3 ... +/// (If none of the checkers produce a transition, we continue with 'Pred'.) +/// +/// This differs from the handling of all the other checker callbacks, where +/// the checker activations are chained sequentially on a single path: +/// Pred --[checker1]-> N1 --[checker2]-> N2 --[checker3]-> N3 ... +/// +/// This difference has historical reasons: originally this callback was called +/// 'EndPath' and only activated at the end of an execution paths, and +/// (according to an old comment) those 'EndPath' checkers expected that they +/// create an "end of path" node which will be final. +/// TODO: Check whether this exceptional behavior is still justified. void CheckerManager::runCheckersForEndFunction(ExplodedNodeSet &Dst, ExplodedNode *Pred, ExprEngine &Eng, const ReturnStmt *RS) { - // We define the builder outside of the loop because if at least one checker - // creates a successor for Pred, we do not need to generate an - // autotransition for it. + // By default, continue from 'Pred' -- this will be removed from 'Dst' if any + // checker generates a transition from it. Dst.insert(Pred); + for (const auto &checkFn : EndFunctionCheckers) { const ProgramPoint &L = FunctionExitPoint(RS, Pred->getStackFrame(), checkFn.Checker); From 7b9e65fc6940863b0dae7c53634cb74659987dae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 18 Aug 2026 18:46:56 +0200 Subject: [PATCH 07/10] Remove NodeBuilder from expandGraphWithCheckers and runChecker Now that the constructor of `CheckerContext` no longer takes a `NodeBuilder` argument, the only role of this `NodeBuilder` was the side effect of its constructor. Note that the parameter order of `runChecker` is flipped to conform to the pattern that the source (or pred) parameter usually precedes the destination node set parameter. --- .../StaticAnalyzer/Core/CheckerManager.cpp | 61 +++++++++---------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index 451945e4db554..9146ecc0c8765 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -94,10 +94,8 @@ void CheckerManager::runCheckersOnASTBody(const Decl *D, AnalysisManager& mgr, //===----------------------------------------------------------------------===// template <typename CHECK_CTX> -static void expandGraphWithCheckers(CHECK_CTX checkCtx, - ExplodedNodeSet &Dst, +static void expandGraphWithCheckers(CHECK_CTX checkCtx, ExplodedNodeSet &Dst, const ExplodedNodeSet &Src) { - const NodeBuilderContext &BldrCtx = checkCtx.Eng.getBuilderContext(); if (Src.empty()) return; @@ -120,9 +118,9 @@ static void expandGraphWithCheckers(CHECK_CTX checkCtx, CurrSet->clear(); } - NodeBuilder B(*PrevSet, *CurrSet, BldrCtx); + CurrSet->insert(*PrevSet); for (const auto &NI : *PrevSet) - checkCtx.runChecker(*I, B, NI); + checkCtx.runChecker(*I, NI, *CurrSet); // If all the produced transitions are sinks, stop. if (CurrSet->empty()) @@ -159,15 +157,15 @@ std::string checkerScopeName(StringRef Name, const CheckerBackend *Checker) { CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); } CheckersTy::const_iterator checkers_end() { return Checkers.end(); } - void runChecker(CheckerManager::CheckStmtFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + void runChecker(CheckerManager::CheckStmtFunc checkFn, ExplodedNode *Pred, + ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope(checkerScopeName("Stmt", checkFn.Checker)); // FIXME: Remove respondsToCallback from CheckerContext; ProgramPoint::Kind K = IsPreVisit ? ProgramPoint::PreStmtKind : ProgramPoint::PostStmtKind; const ProgramPoint &L = ProgramPoint::getProgramPoint( S, K, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); + CheckerContext C(Eng, Pred, Dst, L, WasInlined); checkFn(S, C); } }; @@ -211,7 +209,7 @@ namespace { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckObjCMessageFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope( checkerScopeName("ObjCMsg", checkFn.Checker)); bool IsPreVisit; @@ -227,7 +225,7 @@ namespace { } const ProgramPoint &L = Msg.getProgramPoint(IsPreVisit,checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); + CheckerContext C(Eng, Pred, Dst, L, WasInlined); checkFn(*Msg.cloneWithState<ObjCMethodCall>(Pred->getState()), C); } @@ -283,11 +281,11 @@ namespace { CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); } CheckersTy::const_iterator checkers_end() { return Checkers.end(); } - void runChecker(CheckerManager::CheckCallFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + void runChecker(CheckerManager::CheckCallFunc checkFn, ExplodedNode *Pred, + ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope(checkerScopeName("Call", checkFn.Checker)); const ProgramPoint &L = Call.getProgramPoint(IsPreVisit,checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); + CheckerContext C(Eng, Pred, Dst, L, WasInlined); checkFn(*Call.cloneWithState(Pred->getState()), C); } @@ -329,10 +327,10 @@ struct CheckLifetimeEndContext { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckLifetimeEndFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { assert(Pred->getLocation().getAs<LifetimeEnd>().has_value()); const ProgramPoint L = Pred->getLocation().withTag(checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); + CheckerContext C(Eng, Pred, Dst, L); checkFn(Decl, C); } }; @@ -372,13 +370,13 @@ namespace { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckLocationFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope(checkerScopeName("Loc", checkFn.Checker)); ProgramPoint::Kind K = IsLoad ? ProgramPoint::PreLoadKind : ProgramPoint::PreStoreKind; const ProgramPoint &L = ProgramPoint::getProgramPoint( NodeEx, K, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); + CheckerContext C(Eng, Pred, Dst, L); checkFn(Loc, IsLoad, BoundEx, C); } }; @@ -423,11 +421,11 @@ namespace { CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); } CheckersTy::const_iterator checkers_end() { return Checkers.end(); } - void runChecker(CheckerManager::CheckBindFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + void runChecker(CheckerManager::CheckBindFunc checkFn, ExplodedNode *Pred, + ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope(checkerScopeName("Bind", checkFn.Checker)); const ProgramPoint &L = PP.withTag(checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); + CheckerContext C(Eng, Pred, Dst, L); checkFn(Loc, Val, S, AtDeclInit, C); } @@ -472,12 +470,11 @@ struct CheckBlockEntranceContext { auto checkers_begin() const { return Checkers.begin(); } auto checkers_end() const { return Checkers.end(); } - void runChecker(CheckBlockEntranceFunc CheckFn, NodeBuilder &Bldr, - ExplodedNode *Pred) { + void runChecker(CheckBlockEntranceFunc CheckFn, ExplodedNode *Pred, + ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope( checkerScopeName("BlockEntrance", CheckFn.Checker)); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), - Entrance.withTag(CheckFn.Checker)); + CheckerContext C(Eng, Pred, Dst, Entrance.withTag(CheckFn.Checker)); CheckFn(Entrance, C); } }; @@ -517,10 +514,10 @@ struct CheckBeginFunctionContext { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckBeginFunctionFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope(checkerScopeName("Begin", checkFn.Checker)); const ProgramPoint &L = PP.withTag(checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); + CheckerContext C(Eng, Pred, Dst, L); checkFn(C); } @@ -590,12 +587,12 @@ namespace { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckBranchConditionFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope( checkerScopeName("BranchCond", checkFn.Checker)); ProgramPoint L = PostCondition(Condition, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); + CheckerContext C(Eng, Pred, Dst, L); checkFn(Condition, C); } }; @@ -634,12 +631,12 @@ namespace { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckNewAllocatorFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope( checkerScopeName("Allocator", checkFn.Checker)); ProgramPoint L = PostAllocatorCall( Call.getOriginExpr(), Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L, WasInlined); + CheckerContext C(Eng, Pred, Dst, L, WasInlined); checkFn(cast<CXXAllocatorCall>(*Call.cloneWithState(Pred->getState())), C); } @@ -686,12 +683,12 @@ namespace { CheckersTy::const_iterator checkers_end() { return Checkers.end(); } void runChecker(CheckerManager::CheckDeadSymbolsFunc checkFn, - NodeBuilder &Bldr, ExplodedNode *Pred) { + ExplodedNode *Pred, ExplodedNodeSet &Dst) { llvm::TimeTraceScope TimeScope( checkerScopeName("DeadSymbols", checkFn.Checker)); const ProgramPoint &L = ProgramPoint::getProgramPoint( S, ProgramPointKind, Pred->getStackFrame(), checkFn.Checker); - CheckerContext C(Eng, Pred, Bldr.getFrontier(), L); + CheckerContext C(Eng, Pred, Dst, L); // Note, do not pass the statement to the checkers without letting them // differentiate if we ran remove dead bindings before or after the From 397131cc3c6e661f4b9e71aad37cebce37c34eb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 19 Aug 2026 13:11:05 +0200 Subject: [PATCH 08/10] Clarify end of CheckerContext::addTransitionImpl The old code was pointlessly complex, as `NodeBuilder::generateNode` takes `MarkAsSink` as an optional fourth parameter, which defaults to false -- and `generateSink` just calls `generateNode` with `true` passed to this fourth argument. I will inline `generateNode` at this point in a follow-up change. --- .../StaticAnalyzer/Core/PathSensitive/CheckerContext.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index de4be62b328ea..253021882ce6f 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -447,12 +447,7 @@ class CheckerContext { if (!P) P = Pred; - ExplodedNode *node; - if (MarkAsSink) - node = NB.generateSink(LocalLoc, State, P); - else - node = NB.generateNode(LocalLoc, State, P); - return node; + return NB.generateNode(LocalLoc, State, P, MarkAsSink); } }; From ab683541f20b9ad64e3f116107bc0c0486a13d4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 19 Aug 2026 13:45:44 +0200 Subject: [PATCH 09/10] Inline the NodeBuilder into CheckerContext After dozens of refactoring commits, the last use of the class `NodeBuilder` was in `CheckerContext`, where `addTransitionImpl` calls `NodeBuilder::generateNode` once. This commit replaces the data member`NodeBuilder NB` with its only significant data member, `ExplodedNodeSet &Frontier` and inlines the call of `NodeBuilder::generatNode`. After this, the class `NodeBuilder` can be finally removed. --- .../Core/PathSensitive/CheckerContext.h | 17 ++++++++++++++--- .../Core/PathSensitive/ExprEngine.h | 1 + 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h index 253021882ce6f..c344fd25ff3d3 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h @@ -31,7 +31,13 @@ class CheckerContext { bool Changed; /// The tagged location, which is used to generate all new nodes. const ProgramPoint Location; - NodeBuilder NB; + /// At the end of the checker evaluation, the analysis will continue from the + /// nodes in this set. When the checker adds a transition, freshly created + /// non-sink nodes are added to the `Frontier` and the node that was the + /// source of the transition is unconditionally removed from the `Frontier` + /// (it is superseded, even if the node creation fails or produces a sink). + /// At the beginning, the `Frontier` usually contains `Pred`. + ExplodedNodeSet &Frontier; public: /// If we are post visiting a call, this flag will be set if the @@ -41,7 +47,7 @@ class CheckerContext { CheckerContext(ExprEngine &Eng, ExplodedNode *Pred, ExplodedNodeSet &Dst, const ProgramPoint &Loc, bool WasInlined = false) : Eng(Eng), Pred(Pred), Changed(false), Location(Loc), - NB(Dst, Eng.getBuilderContext()), wasInlined(WasInlined) { + Frontier(Dst), wasInlined(WasInlined) { assert(Pred->getState() && "We should not call the checkers on an empty state."); assert(Loc.getTag() && "The ProgramPoint associated with CheckerContext " @@ -447,7 +453,12 @@ class CheckerContext { if (!P) P = Pred; - return NB.generateNode(LocalLoc, State, P, MarkAsSink); + Frontier.erase(P); + ExplodedNode *N = Eng.getCoreEngine().makeNode(LocalLoc, State, P, MarkAsSink); + + Frontier.insert(N); + + return N; } }; diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index 68d4362aca941..d0b667ccf9f99 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -505,6 +505,7 @@ class ExprEngine { bool hasEmptyWorkList() const { return !Engine.getWorkList()->hasWork(); } bool hasWorkRemaining() const { return Engine.hasWorkRemaining(); } + CoreEngine &getCoreEngine() { return Engine; } const CoreEngine &getCoreEngine() const { return Engine; } public: From f96350db1eb53201aef29c206613cf32da763ae1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 19 Aug 2026 13:52:41 +0200 Subject: [PATCH 10/10] Remove class NodeBuilder --- .../Core/PathSensitive/CoreEngine.h | 96 ------------------- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 11 --- 2 files changed, 107 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index f9326f5b7855c..71443e4434462 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -49,7 +49,6 @@ class ExprEngine; /// It traverses the CFG and generates the ExplodedGraph. class CoreEngine { friend class ExprEngine; - friend class NodeBuilder; friend class NodeBuilderContext; public: @@ -242,101 +241,6 @@ class NodeBuilderContext { } }; -/// \class NodeBuilder -/// This is the simplest builder which generates nodes in the -/// ExplodedGraph. -/// -/// The main benefit of the builder is that it automatically tracks the -/// frontier nodes (or destination set). This is the set of nodes which should -/// be propagated to the next step / builder. They are the nodes which have been -/// added to the builder (either as the input node set or as the newly -/// constructed nodes) but did not have any outgoing transitions added. -/// -/// TODO: This "main benefit" is often useless, in fact the only significant -/// use is within `CheckerManager::ExpandGraphWithCheckers`. There this logic -/// ensures that if a checker performs multiple transitions on the same path, -/// then only the last of them is "built upon" by other checkers or the engine. -/// -/// However, there are also many short-lived temporary `NodeBuilder` instances -/// where the `generateNode` is called in a very predictable manner (once, or -/// once for each source node) and the frontier management is overkill. -/// These locations should be gradually simplified by using the method -/// `CoreEngine::makeNode()` instead of the temporary `NodeBuilder`s. -class NodeBuilder { -protected: - const NodeBuilderContext &C; - - /// The frontier set - a set of nodes which need to be propagated after - /// the builder dies. - ExplodedNodeSet &Frontier; - -public: - NodeBuilder(ExplodedNodeSet &DstSet, const NodeBuilderContext &Ctx) - : C(Ctx), Frontier(DstSet) {} - - NodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet, - const NodeBuilderContext &Ctx) - : NodeBuilder(DstSet, Ctx) { - Frontier.insert(SrcNode); - } - - NodeBuilder(const ExplodedNodeSet &SrcSet, ExplodedNodeSet &DstSet, - const NodeBuilderContext &Ctx) - : NodeBuilder(DstSet, Ctx) { - Frontier.insert(SrcSet); - } - - /// Generates a node in the ExplodedGraph. - ExplodedNode *generateNode(const ProgramPoint &PP, ProgramStateRef State, - ExplodedNode *Pred, bool MarkAsSink = false); - - /// Generates a sink in the ExplodedGraph. - /// - /// When a node is marked as sink, the exploration from the node is stopped - - /// the node becomes the last node on the path and certain kinds of bugs are - /// suppressed. - ExplodedNode *generateSink(const ProgramPoint &PP, - ProgramStateRef State, - ExplodedNode *Pred) { - return generateNode(PP, State, Pred, true); - } - - ExplodedNode *generateNode(const Stmt *S, - ExplodedNode *Pred, - ProgramStateRef St, - const ProgramPointTag *tag = nullptr, - ProgramPoint::Kind K = ProgramPoint::PostStmtKind){ - const ProgramPoint &L = - ProgramPoint::getProgramPoint(S, K, Pred->getStackFrame(), tag); - return generateNode(L, St, Pred); - } - - ExplodedNode *generateSink(const Stmt *S, - ExplodedNode *Pred, - ProgramStateRef St, - const ProgramPointTag *tag = nullptr, - ProgramPoint::Kind K = ProgramPoint::PostStmtKind){ - const ProgramPoint &L = - ProgramPoint::getProgramPoint(S, K, Pred->getStackFrame(), tag); - return generateSink(L, St, Pred); - } - - // This is introduced temporarily to allow the gradual removal of - // NodeBuilders from the checker callback invocation logic. - ExplodedNodeSet &getFrontier() { return Frontier; } - - const ExplodedNodeSet &getResults() const { return Frontier; } - - void takeNodes(const ExplodedNodeSet &S) { - for (const auto I : S) - Frontier.erase(I); - } - - void takeNodes(ExplodedNode *N) { Frontier.erase(N); } - void addNodes(const ExplodedNodeSet &S) { Frontier.insert(S); } - void addNodes(ExplodedNode *N) { Frontier.insert(N); } -}; - } // namespace ento } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index 6ae711af33bed..45087198da27d 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -670,14 +670,3 @@ void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet &Set, const ReturnStmt *RS } } } - -ExplodedNode *NodeBuilder::generateNode(const ProgramPoint &Loc, - ProgramStateRef State, - ExplodedNode *FromN, bool MarkAsSink) { - Frontier.erase(FromN); - ExplodedNode *N = C.getEngine().makeNode(Loc, State, FromN, MarkAsSink); - - Frontier.insert(N); - - return N; -} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
