https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/196313
From db6edd64e64d914c24d57de4bd34e7287e8d0e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 4 May 2026 17:00:31 +0200 Subject: [PATCH 01/11] [NFC] Eliminate NodeBuilder from evalBind Mechanically inline the constructor and `generateNode` calls of the `NodeBuilder` instance in `ExprEngine::evalBind`. This is bug-for-bug equivalent with the old code and exposes the set operations that happen "under the hood". --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index a7a503cd00ab0..4f03678896650 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3735,7 +3735,7 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, getCheckerManager().runCheckersForBind(CheckedSet, Pred, location, Val, StoreE, AtDeclInit, *this, *PP); - NodeBuilder Bldr(CheckedSet, Dst, *currBldrCtx); + Dst.insert(CheckedSet); // If the location is not a 'Loc', it will already be handled by // the checkers. There is nothing left to do. @@ -3744,7 +3744,8 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, /*tag*/nullptr); ProgramStateRef state = Pred->getState(); state = processPointerEscapedOnBind(state, location, Val, LC); - Bldr.generateNode(L, state, Pred); + Dst.erase(Pred); + Dst.insert(Engine.makeNode(L, state, Pred)); return; } @@ -3766,7 +3767,8 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, } const ProgramPoint L = PostStore(StoreE, LC, LocReg, nullptr); - Bldr.generateNode(L, state, PredI); + Dst.erase(PredI); + Dst.insert(Engine.makeNode(L, state, PredI)); } } From 928edf9b5f3e12b3d5db47f571d9d0649a195262 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 5 May 2026 18:43:37 +0200 Subject: [PATCH 02/11] [NFC] Move insertion into `if (!isa<Loc>(location))` ...because on the path that doesn't enter this `if`, the effects of the call `Dst.insert(CheckedSet)` are undone by the `Dst.erase(PredI)` calls in the for loop that remove each element of `CheckedSet` from `Dst`. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 4f03678896650..5a0cffe751a9c 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3735,11 +3735,11 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, getCheckerManager().runCheckersForBind(CheckedSet, Pred, location, Val, StoreE, AtDeclInit, *this, *PP); - Dst.insert(CheckedSet); - // If the location is not a 'Loc', it will already be handled by // the checkers. There is nothing left to do. if (!isa<Loc>(location)) { + Dst.insert(CheckedSet); + const ProgramPoint L = PostStore(StoreE, LC, /*Loc*/nullptr, /*tag*/nullptr); ProgramStateRef state = Pred->getState(); From dc5f9185ae743021cdabf5ffd381fbb0388feb5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Tue, 5 May 2026 18:46:15 +0200 Subject: [PATCH 03/11] [NFC] Remove erase calls that are no longer relevant Remove the calls `Dst.erase(PredI)` (where `PredI` is an element of `CheckedSet`) because they are irrelevant: after the previous commit, `Dst` cannot contain any of the nodes `PredI` (on this branch). A node `PredI` is either a node created by the `check::Bind` callbacks (so it is fresh and distinct from any other node) or the node `Pred` (the incoming argument of `evalBind`, which is "returned without changes" when the checkers don't do anything). The nodes freshly created in the `for` loop of `evalBind` are distinct from `Pred`, so to justify our change, we only need to check that `Dst` cannot contain `Pred` at the beginning of `evalBind`. `Dst` starts as an empty set in most calls of `evalBind` (5 of the 6 direct calls and all 4 calls through `evalStore`). The only non-empty `Dst` appears in `ExprEngine::performTrivialCopy` which does ``` for (ExplodedNode *N : Tmp) evalBind(Dst, CallExpr, N, ThisVal, V, !AlwaysReturnsLValue); ``` where in subsequent iterations `Dst` contains nodes added by the `evalBind` calls in the earlier iterations. However those earlier iterations can only add fresh nodes (that do not appear in the set `Tmp`) and _other, earlier_ elements of `Tmp`, so they also cannot collide with the current element `N` which is passed to `evalBind` as its `Pred` argument. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 5a0cffe751a9c..07c73ecee13ac 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3767,7 +3767,6 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, } const ProgramPoint L = PostStore(StoreE, LC, LocReg, nullptr); - Dst.erase(PredI); Dst.insert(Engine.makeNode(L, state, PredI)); } } From aeeffcb5a249572b9bc8b9a136bbdb5ff75eddca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Wed, 6 May 2026 15:44:26 +0200 Subject: [PATCH 04/11] Assert that location is not a NonLoc in evalBind ... it can be a `Loc` (obviously), can be `UnknownVal` (there are examples in the LIT tests), perhaps it may be `UndefinedVal`, but common sense dictates that it definitely shouldn't be a `NonLoc`. I tried to examine code that calls `evalBind`, but this argument can come from many sources and I wasn't able to exhaustively verify that there isn't some ugly corner case where it is a `Loc`. The LIT suite passes with this assertion and I will also check it on a set of open source projects. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 07c73ecee13ac..15c88b14fb401 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3725,6 +3725,7 @@ ExprEngine::notifyCheckersOfPointerEscape(ProgramStateRef State, void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, ExplodedNode *Pred, SVal location, SVal Val, bool AtDeclInit, const ProgramPoint *PP) { + assert(!isa<NonLoc>(location) && "evalBind location should not be NonLoc!"); const LocationContext *LC = Pred->getLocationContext(); PostStmt PS(StoreE, LC); if (!PP) From 05cc5d015e75bf53fe39c5508d75b4166498fffd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 7 May 2026 14:12:12 +0200 Subject: [PATCH 05/11] Fix logic in case when !isa<Loc>(location) in evalBind When commit dc15415da4cefd8494b8805906d0004abc66ae0a introduced the pointer escape callback in 2012, it inserted two calls to `processPointerEscapeOnBind`. On the early return branch `!isa<Loc>(location)` this call was inserted in a logically incorrect way, probably because the `NodeBuilder` obfuscated the set operations (which were revealed by the previous commits in this PR). On this branch (`!isa<Loc>(location)` e.g. `location` is Unknown), the old logic either applied the `check::Bind` transitions and skipped the PointerEscape transitions or skipped the `check::Bind` transitions before applying the PointerEscape transitions. This didn't cause actual errors because no `check::Bind` checker generates non-sink transitions in the case when the `location` is Unknown or Undefined. (Many checkers use `getAsRegion`, so they would generate transitions for `nonloc::LocAsInteger` -- but I strongly hope that `location` cannot be a `NonLoc` and added an assertion that verifies this.) This commit reunifies the early return with the "normal" branch to ensure that on the `check::Bind` and PointerEscape checkers are both appiled (after each other) on each execution path. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 25 +++++--------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 15c88b14fb401..6d1934011590d 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3736,30 +3736,17 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, getCheckerManager().runCheckersForBind(CheckedSet, Pred, location, Val, StoreE, AtDeclInit, *this, *PP); - // If the location is not a 'Loc', it will already be handled by - // the checkers. There is nothing left to do. - if (!isa<Loc>(location)) { - Dst.insert(CheckedSet); - - const ProgramPoint L = PostStore(StoreE, LC, /*Loc*/nullptr, - /*tag*/nullptr); - ProgramStateRef state = Pred->getState(); - state = processPointerEscapedOnBind(state, location, Val, LC); - Dst.erase(Pred); - Dst.insert(Engine.makeNode(L, state, Pred)); - return; - } - for (const auto PredI : CheckedSet) { ProgramStateRef state = PredI->getState(); state = processPointerEscapedOnBind(state, location, Val, LC); - // When binding the value, pass on the hint that this is a initialization. - // For initializations, we do not need to inform clients of region - // changes. - state = state->bindLoc(location.castAs<Loc>(), Val, LC, - /* notifyChanges = */ !AtDeclInit); + if (std::optional<Loc> AsLoc = location.getAs<Loc>()) { + // When binding the value, pass on the hint that this is a + // initialization. For initializations, we do not need to inform clients + // of region changes. + state = state->bindLoc(*AsLoc, Val, LC, /*notifyChanges=*/!AtDeclInit); + } const MemRegion *LocReg = nullptr; if (std::optional<loc::MemRegionVal> LocRegVal = From e4156c8454a874944e149957bdf5e1f96d6360cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 7 May 2026 14:52:50 +0200 Subject: [PATCH 06/11] [NFC][format] Rename local program point variables Slight readability improvements. I replaced `L` because it is an abbreviation of "location" and we already have other totally unrelated "location"s in this method. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 6d1934011590d..32a8113d0b7b8 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3727,9 +3727,9 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, bool AtDeclInit, const ProgramPoint *PP) { assert(!isa<NonLoc>(location) && "evalBind location should not be NonLoc!"); const LocationContext *LC = Pred->getLocationContext(); - PostStmt PS(StoreE, LC); + PostStmt DefaultPP(StoreE, LC); if (!PP) - PP = &PS; + PP = &DefaultPP; // Do a previsit of the bind. ExplodedNodeSet CheckedSet; @@ -3754,8 +3754,8 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, LocReg = LocRegVal->getRegion(); } - const ProgramPoint L = PostStore(StoreE, LC, LocReg, nullptr); - Dst.insert(Engine.makeNode(L, state, PredI)); + PostStore PS(LC, LocReg, nullptr); + Dst.insert(Engine.makeNode(PS, state, PredI)); } } From 8223d83db86b2792daaf84f7c472bd7c99e363a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 7 May 2026 14:57:59 +0200 Subject: [PATCH 07/11] [NFC][format] Capitalize "location" and "state" To follow the coding standard, now that there are already lots of changes in this method. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 21 ++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 32a8113d0b7b8..29057d121dbdc 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3723,9 +3723,10 @@ ExprEngine::notifyCheckersOfPointerEscape(ProgramStateRef State, /// evalBind - Handle the semantics of binding a value to a specific location. /// This method is used by evalStore and (soon) VisitDeclStmt, and others. void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, - ExplodedNode *Pred, SVal location, SVal Val, + ExplodedNode *Pred, SVal Location, SVal Val, bool AtDeclInit, const ProgramPoint *PP) { - assert(!isa<NonLoc>(location) && "evalBind location should not be NonLoc!"); + assert(!isa<NonLoc>(Location) && "evalBind location should not be NonLoc!"); + const LocationContext *LC = Pred->getLocationContext(); PostStmt DefaultPP(StoreE, LC); if (!PP) @@ -3733,29 +3734,29 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, // Do a previsit of the bind. ExplodedNodeSet CheckedSet; - getCheckerManager().runCheckersForBind(CheckedSet, Pred, location, Val, + getCheckerManager().runCheckersForBind(CheckedSet, Pred, Location, Val, StoreE, AtDeclInit, *this, *PP); for (const auto PredI : CheckedSet) { - ProgramStateRef state = PredI->getState(); + ProgramStateRef State = PredI->getState(); - state = processPointerEscapedOnBind(state, location, Val, LC); + State = processPointerEscapedOnBind(State, Location, Val, LC); - if (std::optional<Loc> AsLoc = location.getAs<Loc>()) { + if (std::optional<Loc> AsLoc = Location.getAs<Loc>()) { // When binding the value, pass on the hint that this is a // initialization. For initializations, we do not need to inform clients // of region changes. - state = state->bindLoc(*AsLoc, Val, LC, /*notifyChanges=*/!AtDeclInit); + State = State->bindLoc(*AsLoc, Val, LC, /*notifyChanges=*/!AtDeclInit); } const MemRegion *LocReg = nullptr; if (std::optional<loc::MemRegionVal> LocRegVal = - location.getAs<loc::MemRegionVal>()) { + Location.getAs<loc::MemRegionVal>()) { LocReg = LocRegVal->getRegion(); } - PostStore PS(LC, LocReg, nullptr); - Dst.insert(Engine.makeNode(PS, state, PredI)); + PostStore PS(StoreE, LC, LocReg, /*tag=*/nullptr); + Dst.insert(Engine.makeNode(PS, State, PredI)); } } From d00b5e705cff3a208dafefa2229fbc7356fb399c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 7 May 2026 15:01:46 +0200 Subject: [PATCH 08/11] [NFC][format] Update doc-comment, add a comment line --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 29057d121dbdc..48ab118d1add4 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3721,10 +3721,11 @@ ExprEngine::notifyCheckersOfPointerEscape(ProgramStateRef State, } /// evalBind - Handle the semantics of binding a value to a specific location. -/// This method is used by evalStore and (soon) VisitDeclStmt, and others. +/// This method is used by evalStore, VisitDeclStmt, and others. void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, ExplodedNode *Pred, SVal Location, SVal Val, bool AtDeclInit, const ProgramPoint *PP) { + // It may be a Loc, UnknownVal or perhaps UndefinedVal. assert(!isa<NonLoc>(Location) && "evalBind location should not be NonLoc!"); const LocationContext *LC = Pred->getLocationContext(); From 34d0a3ec7dec75a918078916523fd973ccb1c34c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Thu, 7 May 2026 15:02:48 +0200 Subject: [PATCH 09/11] [NFC][format] Regularize use of 'auto' In the `for` loop do not use 'auto' because the type was not spelled out in the immediate vicincy. In the two `if` statements use 'auto' because the `getAs<TYPE>()` calls already specify the type. --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 48ab118d1add4..fefef1cb06074 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3738,12 +3738,12 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, getCheckerManager().runCheckersForBind(CheckedSet, Pred, Location, Val, StoreE, AtDeclInit, *this, *PP); - for (const auto PredI : CheckedSet) { + for (ExplodedNode *PredI : CheckedSet) { ProgramStateRef State = PredI->getState(); State = processPointerEscapedOnBind(State, Location, Val, LC); - if (std::optional<Loc> AsLoc = Location.getAs<Loc>()) { + if (auto AsLoc = Location.getAs<Loc>()) { // When binding the value, pass on the hint that this is a // initialization. For initializations, we do not need to inform clients // of region changes. @@ -3751,10 +3751,8 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, } const MemRegion *LocReg = nullptr; - if (std::optional<loc::MemRegionVal> LocRegVal = - Location.getAs<loc::MemRegionVal>()) { + if (auto LocRegVal = Location.getAs<loc::MemRegionVal>()) LocReg = LocRegVal->getRegion(); - } PostStore PS(StoreE, LC, LocReg, /*tag=*/nullptr); Dst.insert(Engine.makeNode(PS, State, PredI)); From 0ec0cc31b676f04c4dd5233bd9ba2e24e816d39f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 11 May 2026 13:08:04 +0200 Subject: [PATCH 10/11] [NFC] Simplify by introducing `getAsRegion()` --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index fefef1cb06074..158777cb0fbf0 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3750,11 +3750,7 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, State = State->bindLoc(*AsLoc, Val, LC, /*notifyChanges=*/!AtDeclInit); } - const MemRegion *LocReg = nullptr; - if (auto LocRegVal = Location.getAs<loc::MemRegionVal>()) - LocReg = LocRegVal->getRegion(); - - PostStore PS(StoreE, LC, LocReg, /*tag=*/nullptr); + PostStore PS(StoreE, LC, Location.getAsRegion(), /*tag=*/nullptr); Dst.insert(Engine.makeNode(PS, State, PredI)); } } From ee3db217b1649a764edd6f7edc60b48960ace54f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <[email protected]> Date: Mon, 11 May 2026 13:10:03 +0200 Subject: [PATCH 11/11] [NFC] Add an explanatory comment --- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp index 158777cb0fbf0..583fddf121145 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp @@ -3741,6 +3741,7 @@ void ExprEngine::evalBind(ExplodedNodeSet &Dst, const Stmt *StoreE, for (ExplodedNode *PredI : CheckedSet) { ProgramStateRef State = PredI->getState(); + // Check and record that 'Val' may escape: State = processPointerEscapedOnBind(State, Location, Val, LC); if (auto AsLoc = Location.getAs<Loc>()) { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
