Author: Donát Nagy Date: 2026-08-25T11:37:45+02:00 New Revision: 1b695cee6810e7a8409181fcc4b6086a91072d80
URL: https://github.com/llvm/llvm-project/commit/1b695cee6810e7a8409181fcc4b6086a91072d80 DIFF: https://github.com/llvm/llvm-project/commit/1b695cee6810e7a8409181fcc4b6086a91072d80.diff LOG: [NFC][analyzer] Remove class 'NodeBuilderContext' (#218442) The class `NodeBuilderContext` was heavily distorted during the ad hoc development of the analyzer: it was not actually that useful for building nodes (it could have been replaced by a single pointer to the `CoreEngine`), but it gained a second unrelated role that it was involved in the only way to query the current `CFGBlock`. This class had no actual advantage, but until the start of this year it was widely used in low quality parts of the engine code. After dozens of cleanup commits I was finally able to remove `NodeBuilder` in 3a8697fab84c8d61e7fc4370c19bdd5023391716, so now I can remove `NodeBuilderContext` in this commit. Added: Modified: clang/include/clang/StaticAnalyzer/Core/CheckerManager.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h clang/lib/StaticAnalyzer/Core/ExprEngine.cpp clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp clang/test/Analysis/stack-frame-context-revision.cpp Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h index 3311774f069ea..46e648c6b7d90 100644 --- a/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ b/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -49,7 +49,6 @@ class ExplodedNodeSet; class ExprEngine; struct EvalCallOptions; class MemRegion; -class NodeBuilderContext; class ObjCMethodCall; class RegionAndSymbolInvalidationTraits; class SVal; diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h index 71443e4434462..d26c0d9257b0f 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 NodeBuilderContext; public: using BlocksExhausted = @@ -210,37 +209,6 @@ class CoreEngine { DataTag::Factory &getDataTags() { return DataTags; } }; -class NodeBuilderContext { - const CoreEngine &Eng; - const CFGBlock *Block; - const StackFrame *SF; - -public: - NodeBuilderContext(const CoreEngine &E, const CFGBlock *B, - const StackFrame *S) - : Eng(E), Block(B), SF(S) { - assert(B); - } - - NodeBuilderContext(const CoreEngine &E, const CFGBlock *B, ExplodedNode *N) - : NodeBuilderContext(E, B, N->getStackFrame()) {} - - /// Return the CoreEngine associated with this builder. - const CoreEngine &getEngine() const { return Eng; } - - /// Return the CFGBlock associated with this builder. - const CFGBlock *getBlock() const { return Block; } - - /// Return the stack frame associated with this builder. - const StackFrame *getStackFrame() const { return SF; } - - /// Returns the number of times the current basic block has been - /// visited on the exploded graph path. - unsigned blockCount() const { - return Eng.WList->getBlockCounter().getNumVisited(SF, Block->getBlockID()); - } -}; - } // namespace ento } // namespace clang diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index 68d4362aca941..64a2ebe5149e4 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -83,7 +83,6 @@ class ConstraintManager; class ExplodedNodeSet; class ExplodedNode; class MemRegion; -class NodeBuilderContext; class ProgramState; class ProgramStateManager; class RegionAndSymbolInvalidationTraits; @@ -156,29 +155,8 @@ class ExprEngine { SValBuilder &svalBuilder; unsigned int currStmtIdx = 0; - - /// Pointer to a (so-called, somewhat misnamed) NodeBuilderContext object - /// which has three independent roles: - /// - It holds a pointer to the CFGBlock that is currently under analysis. - /// (This is the primary way to get the current block.) - /// - It holds a pointer to the current StackFrame. (This is rarely - /// used, the stack frame is usually queried from a recent - /// ExplodedNode. Unfortunately it seems that these two sources of truth - /// are not always consistent.) - /// - It can be used for constructing `NodeBuilder`s. Practically all - /// `NodeBuilder` objects are useless complications in the code, so I - /// intend to replace them with direct use of `CoreEngine::makeNode`. - /// TODO: Eventually `currBldrCtx` should be replaced by two separate fields: - /// `const CFGBlock *CurrBlock` & `const StackFrame *CurrStackFrame` - /// that are kept up-to-date and are almost always non-null during the - /// analysis. I will switch to this more natural representation when - /// `NodeBuilder`s are eliminated from the code. - const NodeBuilderContext *currBldrCtx = nullptr; - /// Historically `currBldrCtx` pointed to a local variable in some stack - /// frame. This field is introduced as a temporary measure to allow a gradual - /// transition. Only use this in {re,}setCurrStackFrameAndBlock! - /// TODO: Remove this temporary hack. - std::optional<NodeBuilderContext> OwnedCurrBldrCtx; + const StackFrame *CurrStackFrame = nullptr; + const CFGBlock *CurrBlock = nullptr; /// Helper object to determine if an Objective-C message expression /// implicitly never returns. @@ -235,12 +213,6 @@ class ExprEngine { return &CTU; } - // FIXME: Ideally the body of this method should look like - // CurrStackFrame = SF; - // CurrBlock = B; - // where CurrStackFrame and CurrBlock are new member variables that - // fulfill the roles of `currBldrCtx` in a more natural way. - // This implementation is a temporary measure to allow a gradual transition. void setCurrStackFrameAndBlock(const StackFrame *SF, const CFGBlock *B) { // The current StackFrame and Block is reset at the beginning of // dispatchWorkItem. Ideally, this method should be called only once per @@ -249,20 +221,16 @@ class ExprEngine { // StackFrame and Block needs to change in the middle of a single step // (which currently happens only once, in processCallExit), use an explicit // call to resetCurrStackFrameAndBlock. - assert(!currBldrCtx && !OwnedCurrBldrCtx && + assert(!CurrBlock && !CurrStackFrame && "The current StackFrame and Block is already set"); - OwnedCurrBldrCtx.emplace(Engine, B, SF); - currBldrCtx = &*OwnedCurrBldrCtx; + assert(SF && B && "The StackFrame and Block must be non-null"); + CurrStackFrame = SF; + CurrBlock = B; } void resetCurrStackFrameAndBlock() { - currBldrCtx = nullptr; - OwnedCurrBldrCtx = std::nullopt; - } - - const NodeBuilderContext &getBuilderContext() const { - assert(currBldrCtx); - return *currBldrCtx; + CurrStackFrame = nullptr; + CurrBlock = nullptr; } const StackFrame *getRootStackFrame() const { @@ -277,15 +245,11 @@ class ExprEngine { /// (e.g. a recent `ExplodedNode`). Traditionally this stack frame is /// only used for block count calculations (`getNumVisited`); it is probably /// wise to follow this tradition until the discrepancies are resolved. - const StackFrame *getCurrStackFrame() const { - return currBldrCtx ? currBldrCtx->getStackFrame() : nullptr; - } + const StackFrame *getCurrStackFrame() const { return CurrStackFrame; } /// Get the 'current' CFGBlock corresponding to the current work item /// (elementary analysis step handled by `dispatchWorkItem`). - const CFGBlock *getCurrBlock() const { - return currBldrCtx ? currBldrCtx->getBlock() : nullptr; - } + const CFGBlock *getCurrBlock() const { return CurrBlock; } ConstCFGElementRef getCFGElementRef() const { return {getCurrBlock(), currStmtIdx}; @@ -815,12 +779,11 @@ class ExprEngine { /// and updateObjectsUnderConstruction. std::pair<ProgramStateRef, SVal> handleConstructionContext(const Expr *E, ProgramStateRef State, - const NodeBuilderContext *BldrCtx, const StackFrame *SF, const ConstructionContext *CC, EvalCallOptions &CallOpts, unsigned Idx = 0) { - SVal V = computeObjectUnderConstruction(E, State, BldrCtx->blockCount(), SF, - CC, CallOpts, Idx); + SVal V = computeObjectUnderConstruction(E, State, getNumVisitedCurrent(), + SF, CC, CallOpts, Idx); State = updateObjectsUnderConstruction(V, E, State, SF, CC, CallOpts); return std::make_pair(State, V); diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index e6349eb4eba2a..a8fa11b078993 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -1153,7 +1153,7 @@ void ExprEngine::ProcessLifetimeEnd(const Stmt *S, const VarDecl *D, ExplodedNodeSet Dst; getCheckerManager().runCheckersForLifetimeEnd(Dst, Src, D, *this); - Engine.enqueueStmtNodes(Dst, currBldrCtx->getBlock(), currStmtIdx); + Engine.enqueueStmtNodes(Dst, getCurrBlock(), currStmtIdx); } void ExprEngine::ProcessInitializer(const CFGInitializer CFGInit, diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index 3df2d3d9e3674..9fb167ee2ea4a 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -602,8 +602,8 @@ void ExprEngine::handleConstructor(const Expr *E, ExplodedNode *Pred, } // The target region is found from construction context. - std::tie(State, Target) = handleConstructionContext(CE, State, currBldrCtx, - SF, CC, CallOpts, Idx); + std::tie(State, Target) = + handleConstructionContext(CE, State, SF, CC, CallOpts, Idx); break; } case CXXConstructionKind::VirtualBase: { diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index e0139484b2b48..66fdb6d117a96 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -773,7 +773,7 @@ ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call, assert(RTC->getStmt() == Call.getOriginExpr()); EvalCallOptions CallOpts; // FIXME: We won't really need those. std::tie(State, Target) = - handleConstructionContext(Call.getOriginExpr(), State, currBldrCtx, SF, + handleConstructionContext(Call.getOriginExpr(), State, SF, RTC->getConstructionContext(), CallOpts); const MemRegion *TargetR = Target.getAsRegion(); assert(TargetR); diff --git a/clang/test/Analysis/stack-frame-context-revision.cpp b/clang/test/Analysis/stack-frame-context-revision.cpp index 3ed85e4cb20b8..5137b60289e38 100644 --- a/clang/test/Analysis/stack-frame-context-revision.cpp +++ b/clang/test/Analysis/stack-frame-context-revision.cpp @@ -1,10 +1,10 @@ // RUN: %clang_analyze_cc1 -std=c++14 -analyzer-checker=core,cplusplus.NewDelete -verify %s // expected-no-diagnostics: -// From now the profile of the 'StackFrame' also contains the -// 'NodeBuilderContext::blockCount()'. With this addition we can distinguish -// between the 'StackArgumentsSpaceRegion' of the 'P' arguments being diff erent -// on every iteration. +// From now the profile of the 'StackFrame' also contains the 'BlockCount' +// ('ExprEngine::getNumVisitedCurrent()') value at the call site. With this +// addition we can distinguish between the 'StackArgumentsSpaceRegion' of the +// 'P' arguments being diff erent on every iteration. typedef __INTPTR_TYPE__ intptr_t; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
