https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/179711
As I was trying to understand the class `NodeBuilder` and its subclasses, I wasted lots of time on studying dead or needlessly complicated code. I'm creating this patch to ensure that others in the future won't need to struggle with this cruft. This commit eliminates three deficiencies: - (Small change:) In a constructor of `StmtNodeBuilder` I switched to using the `takeNodes()` overload which accepts an `ExplodedNodeSet` (instead of manually iterating). - The `Finalized` attribute of NodeBuilder was completely irrelevant (it was always initialized to `true`). - The "main" feature of `NodeBuilderWithSinks` was that it gathered the generated sink nodes into a set, but this was never actually used. As the only other feature (storing a `ProgramPoint` in a data member) was very trivial, I replaced this class with a plain `NodeBuilder` in the only location that used it. From 8427692e68653d2520e89e0c3ac2394f726759ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 4 Feb 2026 17:42:35 +0100 Subject: [PATCH 1/3] [NFC][analyzer] Simplify constructor of StmtNodeBuilder Use the proper overload of `takeNodes(). --- .../clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index f4b4263901e6a..cefe518c9de7d 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -401,8 +401,7 @@ class StmtNodeBuilder: public NodeBuilder { NodeBuilder *Enclosing = nullptr) : NodeBuilder(SrcSet, DstSet, Ctx), EnclosingBldr(Enclosing) { if (EnclosingBldr) - for (const auto I : SrcSet) - EnclosingBldr->takeNodes(I); + EnclosingBldr->takeNodes(SrcSet); } ~StmtNodeBuilder() override; From 7907b24af7457db3701abe83ecf5e76e827f0c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 4 Feb 2026 16:51:52 +0100 Subject: [PATCH 2/3] [NFC][analyzer] Remove dead code from NodeBuilder This commit removes the logic related to the data member `bool Finalized` from the class `NodeBuilder` because it was always set to true by default in the constructor of `NodeBuilder`. --- .../Core/PathSensitive/CoreEngine.h | 37 ++++--------------- 1 file changed, 7 insertions(+), 30 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index cefe518c9de7d..f885392f359bd 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -243,21 +243,12 @@ class NodeBuilder { protected: const NodeBuilderContext &C; - /// Specifies if the builder results have been finalized. For example, if it - /// is set to false, autotransitions are yet to be generated. - bool Finalized; - bool HasGeneratedNodes = false; /// The frontier set - a set of nodes which need to be propagated after /// the builder dies. ExplodedNodeSet &Frontier; - /// Checks if the results are ready. - virtual bool checkResults() { - return Finalized; - } - bool hasNoSinksInFrontier() { for (const auto I : Frontier) if (I->isSink()) @@ -265,9 +256,6 @@ class NodeBuilder { return true; } - /// Allow subclasses to finalize results before result_begin() is executed. - virtual void finalizeResults() {} - ExplodedNode *generateNodeImpl(const ProgramPoint &PP, ProgramStateRef State, ExplodedNode *Pred, @@ -275,14 +263,14 @@ class NodeBuilder { public: NodeBuilder(ExplodedNode *SrcNode, ExplodedNodeSet &DstSet, - const NodeBuilderContext &Ctx, bool F = true) - : C(Ctx), Finalized(F), Frontier(DstSet) { + const NodeBuilderContext &Ctx) + : C(Ctx), Frontier(DstSet) { Frontier.Add(SrcNode); } NodeBuilder(const ExplodedNodeSet &SrcSet, ExplodedNodeSet &DstSet, - const NodeBuilderContext &Ctx, bool F = true) - : C(Ctx), Finalized(F), Frontier(DstSet) { + const NodeBuilderContext &Ctx) + : C(Ctx), Frontier(DstSet) { Frontier.insert(SrcSet); assert(hasNoSinksInFrontier()); } @@ -309,25 +297,14 @@ class NodeBuilder { return generateNodeImpl(PP, State, Pred, true); } - const ExplodedNodeSet &getResults() { - finalizeResults(); - assert(checkResults()); - return Frontier; - } + const ExplodedNodeSet &getResults() { return Frontier; } using iterator = ExplodedNodeSet::iterator; /// Iterators through the results frontier. - iterator begin() { - finalizeResults(); - assert(checkResults()); - return Frontier.begin(); - } + iterator begin() { return Frontier.begin(); } - iterator end() { - finalizeResults(); - return Frontier.end(); - } + iterator end() { return Frontier.end(); } const NodeBuilderContext &getContext() { return C; } bool hasGeneratedNodes() { return HasGeneratedNodes; } From 4094819dfa0b2b23710e9db928393b1382cde9c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 4 Feb 2026 17:17:24 +0100 Subject: [PATCH 3/3] [NFC][analyzer] Remove useless NodeBuilderWithSinks The class `NodeBuilderWithSinks` provided two features compared to its base class `NodeBuilder`: 1. It provided an accessor `getSinks()` to retrieve the list of sink nodes generated by this builder. (This was the motivation behind the class name.) 2. It provided `generateNode` and `generateSink` methods that took the location (the ProgramPoint instance) from a "saved" parameter of the constructor `NodeBuilderWithSinks()` (while the analogous methods of `NodeBuilder` take the location as their own argument). Among these features 1. was completely unused, and 2. was so simple that I decied to this class and use a plain `NodeBuilder` + a separately passed location at the only location where this class was used. --- .../Core/PathSensitive/CoreEngine.h | 35 ------------------- .../Core/PathSensitive/ExprEngine.h | 6 ++-- clang/lib/StaticAnalyzer/Core/CoreEngine.cpp | 10 +++--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 23 +++++++----- 4 files changed, 20 insertions(+), 54 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index f885392f359bd..7c1225d23daef 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h @@ -319,41 +319,6 @@ class NodeBuilder { void addNodes(ExplodedNode *N) { Frontier.Add(N); } }; -/// \class NodeBuilderWithSinks -/// This node builder keeps track of the generated sink nodes. -class NodeBuilderWithSinks: public NodeBuilder { - void anchor() override; - -protected: - SmallVector<ExplodedNode*, 2> sinksGenerated; - ProgramPoint &Location; - -public: - NodeBuilderWithSinks(ExplodedNode *Pred, ExplodedNodeSet &DstSet, - const NodeBuilderContext &Ctx, ProgramPoint &L) - : NodeBuilder(Pred, DstSet, Ctx), Location(L) {} - - ExplodedNode *generateNode(ProgramStateRef State, - ExplodedNode *Pred, - const ProgramPointTag *Tag = nullptr) { - const ProgramPoint &LocalLoc = (Tag ? Location.withTag(Tag) : Location); - return NodeBuilder::generateNode(LocalLoc, State, Pred); - } - - ExplodedNode *generateSink(ProgramStateRef State, ExplodedNode *Pred, - const ProgramPointTag *Tag = nullptr) { - const ProgramPoint &LocalLoc = (Tag ? Location.withTag(Tag) : Location); - ExplodedNode *N = NodeBuilder::generateSink(LocalLoc, State, Pred); - if (N && N->isSink()) - sinksGenerated.push_back(N); - return N; - } - - const SmallVectorImpl<ExplodedNode*> &getSinks() const { - return sinksGenerated; - } -}; - /// \class StmtNodeBuilder /// This builder class is useful for generating nodes that resulted from /// visiting a statement. The main difference from its parent NodeBuilder is diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index e5980fa2e2a4d..6705716932bbc 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -86,7 +86,6 @@ class ExplodedNode; class IndirectGotoNodeBuilder; class MemRegion; class NodeBuilderContext; -class NodeBuilderWithSinks; class ProgramState; class ProgramStateManager; class RegionAndSymbolInvalidationTraits; @@ -320,9 +319,8 @@ class ExprEngine { ExplodedNode *Pred, ExplodedNodeSet &Dst); /// Called by CoreEngine when processing the entrance of a CFGBlock. - void processCFGBlockEntrance(const BlockEdge &L, - NodeBuilderWithSinks &nodeBuilder, - ExplodedNode *Pred); + void processCFGBlockEntrance(const BlockEdge &L, const BlockEntrance &BE, + NodeBuilder &Builder, ExplodedNode *Pred); void runCheckersForBlockEntrance(const NodeBuilderContext &BldCtx, const BlockEntrance &Entrance, diff --git a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp index 857b2f0689e05..18a009abfb6a4 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -320,12 +320,12 @@ void CoreEngine::HandleBlockEdge(const BlockEdge &L, ExplodedNode *Pred) { // Call into the ExprEngine to process entering the CFGBlock. BlockEntrance BE(L.getSrc(), L.getDst(), Pred->getLocationContext()); ExplodedNodeSet DstNodes; - NodeBuilderWithSinks NodeBuilder(Pred, DstNodes, BuilderCtx, BE); - ExprEng.processCFGBlockEntrance(L, NodeBuilder, Pred); + NodeBuilder Builder(Pred, DstNodes, BuilderCtx); + ExprEng.processCFGBlockEntrance(L, BE, Builder, Pred); // Auto-generate a node. - if (!NodeBuilder.hasGeneratedNodes()) { - NodeBuilder.generateNode(Pred->State, Pred); + if (!Builder.hasGeneratedNodes()) { + Builder.generateNode(BE, Pred->State, Pred); } ExplodedNodeSet CheckerNodes; @@ -702,8 +702,6 @@ ExplodedNode* NodeBuilder::generateNodeImpl(const ProgramPoint &Loc, return N; } -void NodeBuilderWithSinks::anchor() {} - StmtNodeBuilder::~StmtNodeBuilder() { if (EnclosingBldr) for (const auto I : Frontier) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 545d37764eca4..a90b37cdf88d8 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -2557,19 +2557,23 @@ static const LocationContext *getInlinedLocationContext(ExplodedNode *Node, } /// Block entrance. (Update counters). +/// FIXME: `BlockEdge &L` is only used for debug statistics, consider removing +/// it and using `BlockEntrance &BE` (where `BlockEntrance` is a subtype of +/// `ProgramPoint`) for statistical purposes. void ExprEngine::processCFGBlockEntrance(const BlockEdge &L, - NodeBuilderWithSinks &nodeBuilder, + const BlockEntrance &BE, + NodeBuilder &Builder, ExplodedNode *Pred) { // If we reach a loop which has a known bound (and meets // other constraints) then consider completely unrolling it. if(AMgr.options.ShouldUnrollLoops) { unsigned maxBlockVisitOnPath = AMgr.options.maxBlockVisitOnPath; - const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminatorStmt(); + const Stmt *Term = Builder.getContext().getBlock()->getTerminatorStmt(); if (Term) { ProgramStateRef NewState = updateLoopStack(Term, AMgr.getASTContext(), Pred, maxBlockVisitOnPath); if (NewState != Pred->getState()) { - ExplodedNode *UpdatedNode = nodeBuilder.generateNode(NewState, Pred); + ExplodedNode *UpdatedNode = Builder.generateNode(BE, NewState, Pred); if (!UpdatedNode) return; Pred = UpdatedNode; @@ -2582,10 +2586,10 @@ void ExprEngine::processCFGBlockEntrance(const BlockEdge &L, // If this block is terminated by a loop and it has already been visited the // maximum number of times, widen the loop. - unsigned int BlockCount = nodeBuilder.getContext().blockCount(); + unsigned int BlockCount = Builder.getContext().blockCount(); if (BlockCount == AMgr.options.maxBlockVisitOnPath - 1 && AMgr.options.ShouldWidenLoops) { - const Stmt *Term = nodeBuilder.getContext().getBlock()->getTerminatorStmt(); + const Stmt *Term = Builder.getContext().getBlock()->getTerminatorStmt(); if (!isa_and_nonnull<ForStmt, WhileStmt, DoStmt, CXXForRangeStmt>(Term)) return; @@ -2600,16 +2604,17 @@ void ExprEngine::processCFGBlockEntrance(const BlockEdge &L, // Here we just pass the the first CFG element in the block. ProgramStateRef WidenedState = getWidenedLoopState(Pred->getState(), LCtx, BlockCount, - *nodeBuilder.getContext().getBlock()->ref_begin()); - nodeBuilder.generateNode(WidenedState, Pred); + *Builder.getContext().getBlock()->ref_begin()); + Builder.generateNode(BE, WidenedState, Pred); return; } // FIXME: Refactor this into a checker. if (BlockCount >= AMgr.options.maxBlockVisitOnPath) { - static SimpleProgramPointTag tag(TagProviderName, "Block count exceeded"); + static SimpleProgramPointTag Tag(TagProviderName, "Block count exceeded"); + const ProgramPoint TaggedLoc = BE.withTag(&Tag); const ExplodedNode *Sink = - nodeBuilder.generateSink(Pred->getState(), Pred, &tag); + Builder.generateSink(TaggedLoc, Pred->getState(), Pred); if (const LocationContext *LC = getInlinedLocationContext(Pred, G)) { // FIXME: This will unconditionally prevent inlining this function (even _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
