[clang] [analyzer] Clean up evalBind, fix bad logic (PR #196313)
Donát Nagy via cfe-commits
cfe-commits at lists.llvm.org
Thu May 7 06:23:38 PDT 2026
https://github.com/NagyDonat created https://github.com/llvm/llvm-project/pull/196313
This commit refactors `ExprEngine::evalBind` to eliminate the use of a `NodeBuilder` and fix incorrect logic that was apparently introduced because the `NodeBuilder` had obfuscated the underlying set operations.
In the special case when the engine is binding to an `Unknown` or `Undefined` (memory) location, the old code invoked _either_ the `check::Bind` checkers _or_ the pointer escape checkers on each execution path. This commit ensures that on each execution path _both_ the `check::Bind` checkers _and then_ the pointer escape checkers get a chance to activate.
I'm pretty sure that the bad logic did not cause incorrect behavior of the analyzer, because there are no `checkBind` checkers that generate non-sink transitions when the location is `Unknown` or `Undefined`.
I also added an assertion that the location argument of `evalBind` cannot be a `NonLoc`, because this is a common sense precondition, seems to be actually true and makes it easier to reason about the behavior of this function.
>From db6edd64e64d914c24d57de4bd34e7287e8d0e2f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 4 May 2026 17:00:31 +0200
Subject: [PATCH 1/9] [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?= <donat.nagy at ericsson.com>
Date: Tue, 5 May 2026 18:43:37 +0200
Subject: [PATCH 2/9] [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?= <donat.nagy at ericsson.com>
Date: Tue, 5 May 2026 18:46:15 +0200
Subject: [PATCH 3/9] [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?= <donat.nagy at ericsson.com>
Date: Wed, 6 May 2026 15:44:26 +0200
Subject: [PATCH 4/9] 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?= <donat.nagy at ericsson.com>
Date: Thu, 7 May 2026 14:12:12 +0200
Subject: [PATCH 5/9] 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?= <donat.nagy at ericsson.com>
Date: Thu, 7 May 2026 14:52:50 +0200
Subject: [PATCH 6/9] [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?= <donat.nagy at ericsson.com>
Date: Thu, 7 May 2026 14:57:59 +0200
Subject: [PATCH 7/9] [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?= <donat.nagy at ericsson.com>
Date: Thu, 7 May 2026 15:01:46 +0200
Subject: [PATCH 8/9] [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?= <donat.nagy at ericsson.com>
Date: Thu, 7 May 2026 15:02:48 +0200
Subject: [PATCH 9/9] [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));
More information about the cfe-commits
mailing list