https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/202709
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/9] 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/9] 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/9] 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/9] 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/9] 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/9] 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. From 1f14c763f6abab47504836b3bbde303f808ed42e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 10 Jun 2026 14:03:57 +0200 Subject: [PATCH 7/9] Eliminate trivial NodeBuilders in VisitObjCMessage The removal of these `NodeBuilder`s was unusually easy because these parts of the code ignore the `Frontier` set of the node builder (which is populated with back-and-forth set manipulations) and only use the return value of `generateNode` which is always the same as the return value of the appropriate `makeNode` call. (In the `dstNonNil` I ended up using `makePostStmtNode` because that call matches that frequently occurring node making pattern.) --- clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 738f693af9f81..5d232465fad81 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -196,11 +196,9 @@ 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); + PreStmt PS(ME, Pred->getStackFrame(), nullptr); + Pred = Engine.makeNode(PS, nilState, Pred); assert((Pred || HasTag) && "Should have cached out already!"); (void)HasTag; if (!Pred) @@ -214,13 +212,11 @@ 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); + Pred = Engine.makePostStmtNode(ME, notNilState, Pred); assert((Pred || HasTag) && "Should have cached out already!"); (void)HasTag; if (!Pred) From 8103de354cf7ae9056fe6d317911e93eed7e0c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 10 Jun 2026 14:21:27 +0200 Subject: [PATCH 8/9] Remove some pointless assertions The ancestor of these assertions was originally introduced in 2012 by commit 5481cfefa63624c4a91da0e05a1140e29ce6f65a, to rule out regressions of a bug where incorrect management of the analyzer state led to caching out in a logically inconsistent place. That original assertion was a simple and strong ``` Pred = Bldr.generateNode(currStmt, Pred, notNilState); assert(Pred && "Should have cached out already!"); ``` which provided the useful invariant that "Pred cannot be null here". As the code evolved, the assertion was moved to the engine from `ObjCSelfInitChecker`, duplicated and most importantly, weakened by allowing that the new `Pred` may be null if the old `Pred` had a tag. As the old `Pred` can easily have a tag (and this fact is independent of everything else that happens around here), the new assertion does not provide any useful invariant -- we still need to handle the case when the new `Pred` is nullptr. --- clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 5d232465fad81..76ce9c977884f 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -196,11 +196,8 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, // Receiver is definitely nil, so run ObjCMessageNil callbacks and return. if (nilState && !notNilState) { - bool HasTag = Pred->getLocation().getTag(); PreStmt PS(ME, Pred->getStackFrame(), nullptr); Pred = Engine.makeNode(PS, nilState, Pred); - assert((Pred || HasTag) && "Should have cached out already!"); - (void)HasTag; if (!Pred) return; @@ -215,10 +212,7 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, // Generate a transition to the non-nil state, dropping any potential // nil flow. if (notNilState != State) { - bool HasTag = Pred->getLocation().getTag(); Pred = Engine.makePostStmtNode(ME, notNilState, Pred); - assert((Pred || HasTag) && "Should have cached out already!"); - (void)HasTag; if (!Pred) return; } From 0decffa807bddcafd23a512ce9e3daceef1725c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 10 Jun 2026 14:51:02 +0200 Subject: [PATCH 9/9] Simplify an old-style loop --- clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index 76ce9c977884f..c8f1babc0ccea 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -231,9 +231,7 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, ExplodedNodeSet dstEval; NodeBuilder Bldr(dstGenericPrevisit, dstEval, *currBldrCtx); - for (ExplodedNodeSet::iterator DI = dstGenericPrevisit.begin(), - DE = dstGenericPrevisit.end(); DI != DE; ++DI) { - ExplodedNode *Pred = *DI; + for (ExplodedNode *Pred : dstGenericPrevisit) { ProgramStateRef State = Pred->getState(); CallEventRef<ObjCMethodCall> UpdatedMsg = Msg.cloneWithState(State); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
