Author: Donát Nagy Date: 2026-06-26T14:52:38+02:00 New Revision: 4cc7c70be0132826fdb3cd6a6c90c2610cc1da9d
URL: https://github.com/llvm/llvm-project/commit/4cc7c70be0132826fdb3cd6a6c90c2610cc1da9d DIFF: https://github.com/llvm/llvm-project/commit/4cc7c70be0132826fdb3cd6a6c90c2610cc1da9d.diff LOG: [NFC][analyzer] Remove NodeBuilders in ObjC logic (#202709) This change removes the remaining `NodeBuilder`s from `ExprEngineObjC.cpp` as a step of my project to gradually replace the class `NodeBuilder` with more straightforward tools. The `NodeBuilder`s that I remove were all used in very simple patterns, especially the two `NodeBuilder`s in `VisitObjCMessage` where the `Frontier` set is thrown away and the code continues with the return value of `generateNode` (which is exactly the same as the return value of the appropriate `makeNode` call). In addition to the removal of the `NodeBuilder`s, this change also includes two additional NFC improvements: 1. `populateObjCForDestinationSet` was previously a static helper function with nine (!) awkward arguments; this change turns it into a method of `ExprEngine` with only five arguments (and the first three arguments are customary: _expression_, `Pred`, `Dst` as in the transfer functions). This change was necessary for the `NodeBuilder` removal: without this cleanup I would have had to add a tenth argument. 2. In `VisitObjCMessage` I remove two complex and pointless assertions. The ancestor of these was a simple and useful ```c++ Pred = Bldr.generateNode(currStmt, Pred, notNilState); assert(Pred && "Should have cached out already!"); ``` introduced in 2012 by 5481cfefa63624c4a91da0e05a1140e29ce6f65a in a checker, but since then it was relocated to the engine, duplicated and weakened to `assert(Pred || HasTag)`. As `HasTag` can easily be true and is independent of everything else in this function, the current assertions (which are removed in this change) don't provide any helpful invariant, they just state that "`Pred` is not null ... except when it is null". Added: Modified: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index ce9b20185444e..327d5858e9c6b 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h @@ -594,6 +594,14 @@ class ExprEngine { void VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, ExplodedNode *Pred, ExplodedNodeSet &Dst); + /// Implementation detail of VisitObjCForCollectionStmt, which contains the + /// logic that needs to be executed both in the "container is empty" case + /// (HasElements=false) and the "container is not empty" case + /// (HasElements=true). + void populateObjCForDestinationSet(const ObjCForCollectionStmt *S, + ExplodedNode *Pred, ExplodedNodeSet &Dst, + SVal ElementV, bool HasElements); + void VisitObjCMessage(const ObjCMessageExpr *ME, ExplodedNode *Pred, ExplodedNodeSet &Dst); diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 26b8ca3e1f2bc..bc214f4aa7054 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -26,13 +26,11 @@ void ExprEngine::VisitLvalObjCIvarRefExpr(const ObjCIvarRefExpr *Ex, SVal baseVal = state->getSVal(Ex->getBase(), SF); SVal location = state->getLValue(Ex->getDecl(), baseVal); - ExplodedNodeSet dstIvar; - NodeBuilder Bldr(Pred, dstIvar, *currBldrCtx); - Bldr.generateNode(Ex, Pred, state->BindExpr(Ex, SF, location)); + ExplodedNode *N = Engine.makeNodeWithBinding(Pred, Ex, location); // Perform the post-condition check of the ObjCIvarRefExpr and store // the created nodes in 'Dst'. - getCheckerManager().runCheckersForPostStmt(Dst, dstIvar, Ex, *this); + getCheckerManager().runCheckersForPostStmt(Dst, N, Ex, *this); } void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S, @@ -41,44 +39,37 @@ void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S, getCheckerManager().runCheckersForPreStmt(Dst, Pred, S, *this); } -/// Generate a node in \p Bldr for an iteration statement using ObjC -/// for-loop iterator. -static void populateObjCForDestinationSet(ExplodedNodeSet &dstLocation, - SValBuilder &svalBuilder, - const ObjCForCollectionStmt *S, - ConstCFGElementRef elem, - SVal elementV, SymbolManager &SymMgr, - unsigned NumVisitedCurrent, - NodeBuilder &Bldr, bool hasElements) { - - for (ExplodedNode *Pred : dstLocation) { - ProgramStateRef state = Pred->getState(); - const StackFrame *SF = Pred->getStackFrame(); - - ProgramStateRef nextState = - ExprEngine::setWhetherHasMoreIteration(state, S, SF, hasElements); - - if (auto MV = elementV.getAs<loc::MemRegionVal>()) - if (const auto *R = dyn_cast<TypedValueRegion>(MV->getRegion())) { - // FIXME: The proper thing to do is to really iterate over the - // container. We will do this with dispatch logic to the store. - // For now, just 'conjure' up a symbolic value. - QualType T = R->getValueType(); - assert(Loc::isLocType(T)); - - SVal V; - if (hasElements) { - SymbolRef Sym = SymMgr.conjureSymbol(elem, SF, T, NumVisitedCurrent); - V = svalBuilder.makeLoc(Sym); - } else { - V = svalBuilder.makeIntVal(0, T); - } - - nextState = nextState->bindLoc(elementV, V, SF); +void ExprEngine::populateObjCForDestinationSet(const ObjCForCollectionStmt *S, + ExplodedNode *Pred, + ExplodedNodeSet &Dst, + SVal ElementV, + bool HasElements) { + ProgramStateRef State = Pred->getState(); + const StackFrame *SF = Pred->getStackFrame(); + + State = ExprEngine::setWhetherHasMoreIteration(State, S, SF, HasElements); + + if (auto MV = ElementV.getAs<loc::MemRegionVal>()) + if (const auto *R = dyn_cast<TypedValueRegion>(MV->getRegion())) { + // FIXME: The proper thing to do is to really iterate over the + // container. We will do this with dispatch logic to the store. + // For now, just 'conjure' up a symbolic value. + QualType T = R->getValueType(); + assert(Loc::isLocType(T)); + + SVal V; + if (HasElements) { + SymbolRef Sym = SymMgr.conjureSymbol(getCFGElementRef(), SF, T, + getNumVisitedCurrent()); + V = svalBuilder.makeLoc(Sym); + } else { + V = svalBuilder.makeIntVal(0, T); } - Bldr.generateNode(S, Pred, nextState); - } + State = State->bindLoc(ElementV, V, SF); + } + + Dst.insert(Engine.makePostStmtNode(S, State, Pred)); } void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, @@ -111,7 +102,6 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, const Stmt *elem = S->getElement(); const Expr *collection = S->getCollection(); - const ConstCFGElementRef &elemRef = getCFGElementRef(); ProgramStateRef state = Pred->getState(); SVal collectionV = state->getSVal(collection, Pred->getStackFrame()); @@ -130,20 +120,13 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, ExplodedNodeSet DstLocation; // states in `DstLocation` may diff er from `Pred` evalLocation(DstLocation, S, elem, Pred, state, elementV, false); - for (ExplodedNode *dstLocation : DstLocation) { - ExplodedNodeSet DstLocationSingleton{dstLocation}, Tmp; - NodeBuilder Bldr(dstLocation, Tmp, *currBldrCtx); + for (ExplodedNode *N : DstLocation) { + ExplodedNodeSet Tmp; if (!isContainerNull) - populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, - elemRef, elementV, SymMgr, - getNumVisitedCurrent(), Bldr, - /*hasElements=*/true); + populateObjCForDestinationSet(S, N, Tmp, elementV, /*hasElements=*/true); - populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, elemRef, - elementV, SymMgr, getNumVisitedCurrent(), - Bldr, - /*hasElements=*/false); + populateObjCForDestinationSet(S, N, Tmp, elementV, /*hasElements=*/false); // Finally, run any custom checkers. // FIXME: Eventually all pre- and post-checks should live in VisitStmt. @@ -213,13 +196,8 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, // Receiver is definitely nil, so run ObjCMessageNil callbacks and return. if (nilState && !notNilState) { - ExplodedNodeSet dstNil; - NodeBuilder Bldr(Pred, dstNil, *currBldrCtx); - bool HasTag = Pred->getLocation().getTag(); - Pred = Bldr.generateNode(ME, Pred, nilState, nullptr, - ProgramPoint::PreStmtKind); - assert((Pred || HasTag) && "Should have cached out already!"); - (void)HasTag; + PreStmt PS(ME, Pred->getStackFrame(), nullptr); + Pred = Engine.makeNode(PS, nilState, Pred); if (!Pred) return; @@ -231,15 +209,10 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, return; } - ExplodedNodeSet dstNonNil; - NodeBuilder Bldr(Pred, dstNonNil, *currBldrCtx); // Generate a transition to the non-nil state, dropping any potential // nil flow. if (notNilState != State) { - bool HasTag = Pred->getLocation().getTag(); - Pred = Bldr.generateNode(ME, Pred, notNilState); - assert((Pred || HasTag) && "Should have cached out already!"); - (void)HasTag; + Pred = Engine.makePostStmtNode(ME, notNilState, Pred); if (!Pred) return; } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
