https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/202709
This change removes `NodeBuilders` from `VisitLvalObjCIvarRefExpr` and `VisitObjCForCollectionStmt` as a step of my project to gradually replace the class `NodeBuilder` with more straightforward tools. These `NodeBuilder`s were both used in very simple patterns, but in `VisitObjCForCollectionStmt` the surrounding code was overcomplicated, and I decided to refactor and simplify it (instead of making it uglier). This change turns `populateObjCForDestinationSet` into a method of `ExprEngine` because it needs access to many parts of the `ExprEngine` singleton (before this change it was a static helper function that took 9 arguments). ------ Note for reviewers: this PR consists of six commits, and the individual commit messages explain why each change is NFC. From 24395d6d68cd6a70e5a6dd3d7ab017fc2106cf73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 9 Jun 2026 16:38:58 +0200 Subject: [PATCH 1/6] Eliminate NodeBuilder from 'VisitObjCIvarRefExpr' This node builder was used in a very simple pattern: - A destination set was default initialized (= empty). - The constructor of the `NodeBuilder` placed `Pred` in this empty set. - The `generateNode` call removed `Pred` from the set, then inserted a fresh node with a standard "bind value to expression" transition. This commit uses `makeNodeWithBinding` to create the node and passes it directly to `runCheckersForPostStmt`. This works because `ExplodedNodeSet` has a constructor that implicitly converts an `ExplodedNode *` to a set (which usually contains that one node, but may be empty if the node was null or sink). There is lots of precedent for using this implicit conversion to avoid the boilerplate code for creating ta node set. --- clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 15f0275691e92..c8a188c0a1083 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, From a3d3916e2f358d1e050ec43587678f917ae90312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 9 Jun 2026 16:51:28 +0200 Subject: [PATCH 2/6] Eliminate DstLocationSingleton Previously `visitObjCForCollectonStmt` took an `ExplodedNode *` (`dstLocation`), wrapped it in an one-element set `DstLocationSingleton` then passed it to the function `populateObjCForDestinationSet`, which "unwrapped" this one-element set by iterating over it. This commit removes this back-and-forth logic by turning the first argument of `populateObjCForDestinationSet` into an `ExplodedNode *`. In general, this would not be an equivalent transformation, because the constructor of `ExplodedNodeSet` produces an empty set if its `ExplodedNode *` argument is null or sink, but in this particular case `dstLocation` is an element of the `ExplodedNodeSet` `DstLocation`, so it cannot be null or sink. --- .../StaticAnalyzer/Core/ExprEngineObjC.cpp | 56 +++++++++---------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index c8a188c0a1083..fa10c36904336 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -41,7 +41,7 @@ void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S, /// Generate a node in \p Bldr for an iteration statement using ObjC /// for-loop iterator. -static void populateObjCForDestinationSet(ExplodedNodeSet &dstLocation, +static void populateObjCForDestinationSet(ExplodedNode *Pred, SValBuilder &svalBuilder, const ObjCForCollectionStmt *S, ConstCFGElementRef elem, @@ -49,34 +49,32 @@ static void populateObjCForDestinationSet(ExplodedNodeSet &dstLocation, 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); - } + ProgramStateRef state = Pred->getState(); + const StackFrame *SF = Pred->getStackFrame(); - nextState = nextState->bindLoc(elementV, V, SF); + 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); } - Bldr.generateNode(S, Pred, nextState); - } + nextState = nextState->bindLoc(elementV, V, SF); + } + + Bldr.generateNode(S, Pred, nextState); } void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, @@ -129,16 +127,16 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, evalLocation(DstLocation, S, elem, Pred, state, elementV, false); for (ExplodedNode *dstLocation : DstLocation) { - ExplodedNodeSet DstLocationSingleton{dstLocation}, Tmp; + ExplodedNodeSet Tmp; NodeBuilder Bldr(dstLocation, Tmp, *currBldrCtx); if (!isContainerNull) - populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, + populateObjCForDestinationSet(dstLocation, svalBuilder, S, elemRef, elementV, SymMgr, getNumVisitedCurrent(), Bldr, /*hasElements=*/true); - populateObjCForDestinationSet(DstLocationSingleton, svalBuilder, S, elemRef, + populateObjCForDestinationSet(dstLocation, svalBuilder, S, elemRef, elementV, SymMgr, getNumVisitedCurrent(), Bldr, /*hasElements=*/false); From 8b612e170a44b851b347a5cb3a6bb1e1280b7880 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 9 Jun 2026 17:05:12 +0200 Subject: [PATCH 3/6] Remove NodeBuilder from VisitObjCForCollectionStmt The constructor of this node builder placed the node `dstLocation` into the previously empty destination set `Tmp`. After this, the node builder was passed to the static function `populateObjCForDestinationSet` once or twice, with each call unconditionally using one `generateNode` call at the very end. These calls guaranteed that `dstLocation` was removed from `Tmp` and did not interfere with each other because the node created by the first call was a fresh node, so the second `generateNode` couldn't remove it. In the parameter list of `populateObjCForDestinationSet` this commit replaces the `NodeBuilder` object with its destination set and a reference to the `CoreEngine` singleton (which is available for the `NodeBuilder` through its `NodeBuilderContext`). This makes the parameter list of `populateObjCForDestinationSet` even more awkward, but I will refactor and simplify it in a follow-up commit. --- .../StaticAnalyzer/Core/ExprEngineObjC.cpp | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index fa10c36904336..9131b667d90c2 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -39,15 +39,13 @@ void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S, getCheckerManager().runCheckersForPreStmt(Dst, Pred, S, *this); } -/// Generate a node in \p Bldr for an iteration statement using ObjC +/// Generate a node into \p Dst for an iteration statement using ObjC /// for-loop iterator. -static void populateObjCForDestinationSet(ExplodedNode *Pred, - SValBuilder &svalBuilder, - const ObjCForCollectionStmt *S, - ConstCFGElementRef elem, - SVal elementV, SymbolManager &SymMgr, - unsigned NumVisitedCurrent, - NodeBuilder &Bldr, bool hasElements) { +static void populateObjCForDestinationSet( + ExplodedNode *Pred, SValBuilder &svalBuilder, + const ObjCForCollectionStmt *S, ConstCFGElementRef elem, SVal elementV, + SymbolManager &SymMgr, unsigned NumVisitedCurrent, ExplodedNodeSet &Dst, + CoreEngine &Engine, bool hasElements) { ProgramStateRef state = Pred->getState(); const StackFrame *SF = Pred->getStackFrame(); @@ -74,7 +72,7 @@ static void populateObjCForDestinationSet(ExplodedNode *Pred, nextState = nextState->bindLoc(elementV, V, SF); } - Bldr.generateNode(S, Pred, nextState); + Dst.insert(Engine.makePostStmtNode(S, nextState, Pred)); } void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, @@ -128,17 +126,16 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, for (ExplodedNode *dstLocation : DstLocation) { ExplodedNodeSet Tmp; - NodeBuilder Bldr(dstLocation, Tmp, *currBldrCtx); if (!isContainerNull) - populateObjCForDestinationSet(dstLocation, svalBuilder, S, - elemRef, elementV, SymMgr, - getNumVisitedCurrent(), Bldr, + populateObjCForDestinationSet(dstLocation, svalBuilder, S, elemRef, + elementV, SymMgr, getNumVisitedCurrent(), + Tmp, Engine, /*hasElements=*/true); populateObjCForDestinationSet(dstLocation, svalBuilder, S, elemRef, - elementV, SymMgr, getNumVisitedCurrent(), - Bldr, + elementV, SymMgr, getNumVisitedCurrent(), Tmp, + Engine, /*hasElements=*/false); // Finally, run any custom checkers. From 6e6d147d7c9077e3f659498577bbb55a437977f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 9 Jun 2026 17:54:13 +0200 Subject: [PATCH 4/6] Turn populateObjCFroDestinationSet into a method of ExprEngine Previously this was a standalone static function, but in addition to the five 'real' arguments it had to take five other arguments to access the state of the `ExprEngine` singletion. I reordered the remaining arguments to follow the "expression, Pred, Dst, others" pattern that is typical in the various transfer functions. --- .../Core/PathSensitive/ExprEngine.h | 8 ++++++ .../StaticAnalyzer/Core/ExprEngineObjC.cpp | 25 +++++++------------ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h index bb2de45cec92a..422ca023a692c 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 9131b667d90c2..7454bb29116b3 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -39,14 +39,11 @@ void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S, getCheckerManager().runCheckersForPreStmt(Dst, Pred, S, *this); } -/// Generate a node into \p Dst for an iteration statement using ObjC -/// for-loop iterator. -static void populateObjCForDestinationSet( - ExplodedNode *Pred, SValBuilder &svalBuilder, - const ObjCForCollectionStmt *S, ConstCFGElementRef elem, SVal elementV, - SymbolManager &SymMgr, unsigned NumVisitedCurrent, ExplodedNodeSet &Dst, - CoreEngine &Engine, bool hasElements) { - +void ExprEngine::populateObjCForDestinationSet(const ObjCForCollectionStmt *S, + ExplodedNode *Pred, + ExplodedNodeSet &Dst, + SVal elementV, + bool hasElements) { ProgramStateRef state = Pred->getState(); const StackFrame *SF = Pred->getStackFrame(); @@ -63,7 +60,8 @@ static void populateObjCForDestinationSet( SVal V; if (hasElements) { - SymbolRef Sym = SymMgr.conjureSymbol(elem, SF, T, NumVisitedCurrent); + SymbolRef Sym = SymMgr.conjureSymbol(getCFGElementRef(), SF, T, + getNumVisitedCurrent()); V = svalBuilder.makeLoc(Sym); } else { V = svalBuilder.makeIntVal(0, T); @@ -105,7 +103,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()); @@ -128,14 +125,10 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, ExplodedNodeSet Tmp; if (!isContainerNull) - populateObjCForDestinationSet(dstLocation, svalBuilder, S, elemRef, - elementV, SymMgr, getNumVisitedCurrent(), - Tmp, Engine, + populateObjCForDestinationSet(S, dstLocation, Tmp, elementV, /*hasElements=*/true); - populateObjCForDestinationSet(dstLocation, svalBuilder, S, elemRef, - elementV, SymMgr, getNumVisitedCurrent(), Tmp, - Engine, + populateObjCForDestinationSet(S, dstLocation, Tmp, elementV, /*hasElements=*/false); // Finally, run any custom checkers. From 7ab94b50e230a795ecb1ada7a033550c3b09eeba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 9 Jun 2026 18:05:24 +0200 Subject: [PATCH 5/6] Capitalize variable names in populateObjCForDestinationSet Its body will be blamed to me anyway because I removed an unneeded layer of indentation, so I take this opportunity to update the variable names (except for `svalBuilder` which is a data member of `ExprEngine`). --- .../lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 7454bb29116b3..caa2357273c44 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -42,15 +42,14 @@ void ExprEngine::VisitObjCAtSynchronizedStmt(const ObjCAtSynchronizedStmt *S, void ExprEngine::populateObjCForDestinationSet(const ObjCForCollectionStmt *S, ExplodedNode *Pred, ExplodedNodeSet &Dst, - SVal elementV, - bool hasElements) { - ProgramStateRef state = Pred->getState(); + SVal ElementV, + bool HasElements) { + ProgramStateRef State = Pred->getState(); const StackFrame *SF = Pred->getStackFrame(); - ProgramStateRef nextState = - ExprEngine::setWhetherHasMoreIteration(state, S, SF, hasElements); + State = ExprEngine::setWhetherHasMoreIteration(State, S, SF, HasElements); - if (auto MV = elementV.getAs<loc::MemRegionVal>()) + 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. @@ -59,7 +58,7 @@ void ExprEngine::populateObjCForDestinationSet(const ObjCForCollectionStmt *S, assert(Loc::isLocType(T)); SVal V; - if (hasElements) { + if (HasElements) { SymbolRef Sym = SymMgr.conjureSymbol(getCFGElementRef(), SF, T, getNumVisitedCurrent()); V = svalBuilder.makeLoc(Sym); @@ -67,10 +66,10 @@ void ExprEngine::populateObjCForDestinationSet(const ObjCForCollectionStmt *S, V = svalBuilder.makeIntVal(0, T); } - nextState = nextState->bindLoc(elementV, V, SF); + State = State->bindLoc(ElementV, V, SF); } - Dst.insert(Engine.makePostStmtNode(S, nextState, Pred)); + Dst.insert(Engine.makePostStmtNode(S, State, Pred)); } void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, From 6da96bda20d261afb388b5e39780956004fe1fde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 9 Jun 2026 18:08:29 +0200 Subject: [PATCH 6/6] Rename loop variable 'dstLocation' to 'N' Because the old name was too long, non-capitalized, non-idiomatic and easy to confuse with the capitalized 'DstLocation'. 'N' is a customary name in these relatively small loops that iterate over a set of exploded nodes. --- clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index caa2357273c44..738f693af9f81 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -120,15 +120,13 @@ void ExprEngine::VisitObjCForCollectionStmt(const ObjCForCollectionStmt *S, ExplodedNodeSet DstLocation; // states in `DstLocation` may differ from `Pred` evalLocation(DstLocation, S, elem, Pred, state, elementV, false); - for (ExplodedNode *dstLocation : DstLocation) { + for (ExplodedNode *N : DstLocation) { ExplodedNodeSet Tmp; if (!isContainerNull) - populateObjCForDestinationSet(S, dstLocation, Tmp, elementV, - /*hasElements=*/true); + populateObjCForDestinationSet(S, N, Tmp, elementV, /*hasElements=*/true); - populateObjCForDestinationSet(S, dstLocation, Tmp, elementV, - /*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. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
