=?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/[email protected]>
llvmbot wrote: <!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> 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 bother 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. --- Full diff: https://github.com/llvm/llvm-project/pull/179711.diff 4 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h (+8-67) - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h (+2-4) - (modified) clang/lib/StaticAnalyzer/Core/CoreEngine.cpp (+4-6) - (modified) clang/lib/StaticAnalyzer/Core/ExprEngine.cpp (+14-9) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index f4b4263901e6a..7c1225d23daef 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; } @@ -342,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 @@ -401,8 +343,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; 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 `````````` </details> https://github.com/llvm/llvm-project/pull/179711 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
