On 4/2/26 13:44, Vineet Gupta wrote:
> On 4/2/26 10:02 AM, David Faust wrote:
>>>> And again, what precisely is problematic about generating rN = rM
>>>> for this set?
>>> Verifier trips up, #12 attach _probe
>>> (tools/testing/selftests/bpf/progs/test_attach_probe.c)
>>>
>>>> If %r1 is subsequently used as the int argument to a function call,
>>>> then it is valid as is.
>>>> If it would be passed as a 'unsigned long' or something else requiring
>>>> a conversion then we would rather have e.g. a zero_extend:DI to do that,
>>>> not a simple set.
>>> The rN based move copies the full 64-bits, but the subsequent arg is
>>> int: verifier wants to make sure top bits are cleared, which is not
>>> guaranteed given the current codegen.
>> Ahhhhhh, so it is a verifier issue! Not a code correctness issue.
>> OK, that makes more sense.
>
> Yeah and my bad for not calling it in the changelog, but it was a
> logistics issue as I'd seperated it out of ABI change but needed ABI
> changes for the exact context.
>
>>> Full log below, relevant snippets extracted:
>>>
>>> 22: (85) call bpf_copy_from_user_str#85440 ; R0=scalar()
>>> ...
>>> 24: (bf) r7 = r0 ; R0=scalar(id=1) R7=scalar(id=1)
>>> ...
>>> 29: (85) call bpf_strncmp#182 ; R0=scalar()
>>> 30: (55) if r0 != 0x0 goto pc+1 ; R0=0
>>> 31: (16) if w7 == 0x4 goto pc+2 34: R0=0
>>> R6=map_value(map=test_att.bss,ks=4,vs=56)
>>> R7=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
>>> 0xffffffff00000000)) R10=fp0 fp-16=???????m fp-24=mmmmmmmm fp-88=????mmmm
>>> ...
>>>
>>> 36: (bf) r2 = r7 ;
>>> R2=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
>>> 0xffffffff00000000))
>>> R7=scalar(id=1,smin=0x8000000000000004,smax=0x7fffffff00000004,umin=smin32=umin32=4,umax=0xffffffff00000004,smax32=umax32=4,var_off=(0x4;
>>> 0xffffffff00000000))
>>> ...
>>> 39: (85) call bpf_copy_from_user_str#85440
>>> R2 min value is negative, either use unsigned or 'var &= const'
>>>
>>> Now, thinking about it, part of the problem could also be #31 using w7
>>> reg for comparison. If it had used the rN reg too, maybe verifier would
>>> be confident about the top bits too and won't trip up. But that again
>>> would point to impedance mismatch between movsi (only using rN regs) and
>>> branch_on_si, which would use wN regs.
>> Hmm, looking at these verifier logs (thanks for providing), I think
>> the issue may stem from the bpf_copy_from_user_str call.
>> Ultimately the return value propagates r0 -> r7 -> r2.
>
> Exactly.
>
>> BUT, I am confused. This test passes on my system compiled with
>> gcc trunk. After the call, we emit a movs32 when moving the return value
>> to a different register, which is enough for the verifier to know the
>> value is restricted to signed integer range.
>>
>> Are you seeing this with trunk or with the PROMOTE_MODE or some other
>> change applied?
>
> Yeah it's specifically with PROMOTE_MODE part of the ABI change, w/o
> touching target hooks.
I see.
At this point it seems clear that there is no correctness issue with the
moves currently generated by gcc trunk.
Rather, the question is about the verifiability of the resulting code.
But verifier issues raised here also do not reproduce with gcc trunk.
Can you please send a reduced example showing precisely what code is
being rejected by the verifier (despite being semantically correct)
_as generated by gcc trunk_ ?
If these issues only appear with a patched gcc, then this patch to
address those issues should be part of a patchset including all other
changes which would necessitate this fix.
You are introducing a lot of confusion by sending a patch against
issues that do not seem to exist in trunk.
>
> Thx,
> -Vineet
