Re: [PATCH bpf-next v2 03/13] bpf: track low-32 scalar equality across zero-extending movs
From: Vineet Gupta
Date: Wed Sep 16 2026 - 20:08:37 EST
On 9/15/26 9:23 PM, Alexei Starovoitov wrote:
On Tue, Sep 15, 2026 at 1:31 PM Vineet Gupta <vineet.gupta@xxxxxxxxx> wrote:
On 9/12/26 11:59 AM, Alexei Starovoitov wrote:I think that's a problem with reconstruct_zext32().
On Thu Sep 10, 2026 at 9:46 AM PDT, Vineet Gupta wrote:The 2 examples below both imply ADD_CONST32 + delta=0, with different
Linked-scalar equality is full-64-bit only. A 32-bit mov from a sourceat the first glance SUBREG_ZEXT is exactly the same as ADD_CONST32 delta == 0.
with unknown high bits therefore has to drop the relationship, and a later
narrowing of the source never reaches the destination:
r6 = ... /* full 64-bit unknown */
w7 = w6 /* 32-bit zero-extending mov */
if w6 != 0 goto ... /* not taken: r6's low 32 bits are 0 */
if w7 == 0 goto ... /* not deduced today */
Record a low-32-only link instead: dst shares src's low 32 bits and its
high half is zero. On a later narrowing, sync_linked_regs() rebuilds such
a register from the base rather than copying it, by re-applying the same
zext_32_to_64() the mov used. The reverse direction is skipped: a ->subreg
base knows nothing about a full register's high half.
no?
Both are unidirectional:
w6->id == 1
w7->id == 1, add_const == 32, delta == 0
will zero extend w7.
This new SUBREG_ZEXT will do the same.
What am I missing?
semantics and need special casing.
w3 = w2; w3 += 0
w7 = w6; (r6 was wide)
With code hacks it could in fact me made to work, but its less cleaner.
Current approach we have something like below in sync_linked_regs()
/*
* A ->subreg register shares only the base's low 32
bits, so it
* is rebuilt rather than copied. Not modelled together
with a
* delta, so skip if either side has one (sound, less
precise).
*/
if (reg->subreg) {
if (reg->add_const || known_reg->add_const)
continue;
if (reg->subreg == SUBREG_ZEXT)
reconstruct_zext32(reg, known_reg);
else
reconstruct_sext32(reg, known_reg);
if (e->is_reg)
mark_reg_scratched(env, e->regno);
else
mark_stack_slot_scratched(env, e->spi);
continue;
}
/*
* The reverse: known_reg knows only its low 32 bits,
which say
* nothing about reg's high half.
*/
if (known_reg->subreg)
continue;
With the suggested approach it looks something like below.
ADD_CONST_32, delta == 0 has to be disambiguated from a real += 0 by
inspecting another register's bounds.
/*
* A low-32 link shares only the base's low 32 bits, so it is
* rebuilt rather than copied.
*
* SIGN_EXTEND_32 says so outright. ADD_CONST_32 with
delta == 0
* does not: it is also what "w3 = w2; w3 += 0" records,
which is
* a plain equality. The two are separable only by the base's
* width
*/
if (reg->add_const == SIGN_EXTEND_32 &&
!known_reg->add_const) {
reconstruct_sext32(reg, known_reg);
goto scratched;
}
if (reg->add_const == ADD_CONST_32 && reg->delta == 0 &&
!known_reg->add_const) {
reconstruct_zext32(reg, known_reg);
goto ...;
}
It shouldn't be necessary.
The existing code that adds a constant should
already handle it.
If not, we have a bug in add_const_32.
Not really a bug, but a deliberate design choice in the past.
if (alu32 && (dst_umax > U32_MAX))
goto clear_id;
So the core change is ADD_CONST_32 needs to be allowed on wider regs.
So treating it as
reg = (u32)(base + delta)
which simplifies to zero-extend for delta==0 and base can be either wide or narrow.
Instead of losing link at wx=wy time
we can keep it with delta == 0
and sync_linked_regs() shouldn't need any new code.
I toyed with a prototype which essentially lifts the restriction above (and ensuing adjustments)
FWIW it *does* require a reverse guard in sync_linked_regs() - so some additional code there (at least in my version).
It ended up with full testsuite run parity - after 4 incremental patches.
But the pattern of all those patches was adding some predicate / special-casing to reg->add_const
hunk 1
- if (src_reg->add_const)
+ if (src_reg->add_const && src_reg->delta)
hunk 2
- if (dst_reg->add_const) {
+ if (dst_reg->add_const && !dst_reg->delta &&
+ dst_reg->add_const == (alu32 ? ADD_CONST_32 : ADD_CONST_64)) {
+ dst_reg->delta = off;
+ } else if (dst_reg->add_const) {
hunk 3
- if (subreg_link && reg->id)
- state->regs[dst_regno].subreg = is_ldsx ? SUBREG_SEXT : SUBREG_ZEXT;
+ if (subreg_link && reg->id) {
+ if (is_ldsx) {
+ state->regs[dst_regno].subreg_sext = true;
+ } else {
+ state->regs[dst_regno].add_const = ADD_CONST_32;
+ state->regs[dst_regno].delta = 0;
+ }
+ }
hunk 4
- if (known_reg->subreg)
+ if (known_reg->subreg_sext)
+ continue;
+ /*
+ * Same for a zero-extending link, now spelled ADD_CONST_32:
+ * reg == (u32)(base + delta) does not invert once the base is
+ * wider than 32 bits.
+ */
+ if (known_reg->add_const == ADD_CONST_32 &&
+ get_reg_width(reg) > 32)
continue;
Yes it does allow removal of reconstruct_zext32()
- if (reg->subreg) {
+ if (reg->subreg_sext) {
if (reg->add_const || known_reg->add_const)
continue;
- if (reg->subreg == SUBREG_ZEXT)
- reconstruct_zext32(reg, known_reg);
- else
- reconstruct_sext32(reg, known_reg);
+ reconstruct_sext32(reg, known_reg);
but personally speaking reading the code feels a bit more harder now.
I can send over the full patch which converts the v2 SUBREG_ZEXT into ADD_CONST32+delta=0 to give a feel for all the special casings, were it to be subsumed into the orig zext support work, but I think you understand what I'm getting at ;-)
Happy to pursue whatever your maintainer hat tells you.
Thx,
-Vineet