Re: [PATCH bpf-next v4 5/8] selftests/bpf: write the mixed arena/scalar ALU tests in asm

[bot+bpf-ci]

> selftests/bpf: write the mixed arena/scalar ALU tests in asm

TL;DR: Commit message only, no code issues: it refers to a follow-up patch as
"6/8" and says an arena-capable GCC is needed; both should name the commit by
subject, since series numbers vanish from git history.

> selftests/bpf: write the mixed arena/scalar ALU tests in asm
>
> They fail to build with BPF_GCC:
>
>   progs/verifier_arena.c:566:5: error: optimization may eliminate reads and/or writes to register variables [-Werror=volatile-register-var]
>
> for
>
>   volatile register __u64 reg asm("r3");
>
> and GCC means it. With the warning silenced it drops both scalar
> accesses, leaving nothing that uses r3 as a scalar:
>
>   call	7
>   if w0 == 0 goto .L136
>   r9 = arena ll;r3 = 0;r3 = addr_space_cast(r3, 0, 1);
>   .L136:
>   r0 = 0
>   exit
>
> `reg = 1ULL << 32' and `reg += 1' are both gone. The tests need one
> instruction reached with r3 live as a scalar on one path and as an
> arena pointer on the other; what is left gives the verifier no reason
> to reject anything, so the __failure expectation would not hold. A
> local register variable is not guaranteed to survive a helper call
> either, which GCC is also entitled to act on.
>
> Nothing here needs a compiler to allocate registers, so write the
> whole body in asm like mixed_arena_stack_atomic below, which tests the
> same shape. Both compilers then emit the same sequence, with the
> branch landing on the shared ALU op:
>
>   call 7
>   r6=r0
>   r3=1
>   r3<<=32
>   if w6==0 goto 4          <- lands on r3+=1
>   r9=0 ll
>   r3=0
>   r3 = addr_space_cast(r3, 0, 1)
>   r3+=1                    <- scalar on one path, arena pointer on the other
>   r0=0
>   exit
>
> And while here also bring in __load_if_JITed(). A program binding an
> arena map is rejected in check_map_prog_compatibility() with "JIT is
> required to use arena" during map resolution, before do_check() can
> reach the shared instruction, so without a JIT these would fail on the
> wrong message rather than skip.
>
> Both tests become visible once 6/8 stops hiding them, and pass:
>
>   verifier_arena/mixed_arena_scalar_alu64_scalar_first   -> OK
>   verifier_arena/mixed_arena_scalar_alu64_arena_first    -> OK
>
> Notes:
>  - This does need an __arena capable BPF_GCC.
>  - Under clang the rewrite is not a no-op: the existing version spilled r3
>    to the stack and did the shared ALU op on r1 reloaded from it, so the
>    test was not exercising one register live as both a scalar and an arena
>    pointer.
>
> Signed-off-by: Vineet Gupta <vineet.gupta@xxxxxxxxx>
>

this isn't a bug, but could the changelog name the follow-up commit by its
subject instead of "6/8"?  The series number is not kept once the patches
are applied, so the reference no longer identifies anything in git history.

The same applies to the "Notes:" line "This does need an __arena capable
BPF_GCC", which only holds once that later commit has dropped the
__BPF_FEATURE_ADDR_SPACE_CAST guard around these tests.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/41694978246