=?utf-8?q?Donát?= Nagy <[email protected]>, =?utf-8?q?Donát?= Nagy <[email protected]>, =?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 @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/183540.diff 2 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h (-7) - (modified) clang/lib/StaticAnalyzer/Core/CoreEngine.cpp (+19-49) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index a8b33f22fcc33..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); @@ -121,9 +117,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 29bd103e57d2c..f0f2a7619de0b 100644 --- a/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp @@ -391,11 +391,11 @@ 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) { - generateNode(BlockEdge(B, succ, Pred->getLocationContext()), - Pred->State, Pred); + 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); } } return; @@ -479,8 +479,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,24 +578,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. -/// 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. - - // Only add 'Node' to the worklist if it was freshly generated. - if (IsNew) WList->enqueue(Node); -} - void CoreEngine::enqueueStmtNode(ExplodedNode *N, const CFGBlock *Block, unsigned Idx) { assert(Block); @@ -637,28 +620,12 @@ 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); } -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); - - bool isNew; - ExplodedNode *Node = G.getNode(Loc, N->getState(), false, &isNew); - Node->addPredecessor(N, G); - return isNew ? Node : nullptr; -} - std::optional<unsigned> CoreEngine::getCompletedIterationCount(const CFGBlock *B, ExplodedNode *Pred) const { @@ -695,15 +662,18 @@ void CoreEngine::enqueue(ExplodedNodeSet &Set, } void CoreEngine::enqueueEndOfFunction(ExplodedNodeSet &Set, const ReturnStmt *RS) { - for (auto *I : Set) { + for (ExplodedNode *Node : Set) { + const LocationContext *LocCtx = Node->getLocationContext(); + // If we are in an inlined call, generate CallExitBegin node. - if (I->getLocationContext()->getParent()) { - I = generateCallExitBeginNode(I, RS); - if (I) - WList->enqueue(I); + if (LocCtx->getParent()) { + // Use the callee location context. + 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++; } } `````````` </details> https://github.com/llvm/llvm-project/pull/183540 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
