Re: [PATCH bpf-next v5 10/11] selftests/bpf: Add inline-asm and subprog tests for R0:R2 returns

On 8/13/26 2:11 PM, [email protected] wrote:
>> diff --git a/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c b/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c
>> index 979536f3c89c..c295adedbae8 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/aggregate_ret.c
>> @@ -4,10 +4,14 @@
>>   #include "aggregate_ret_int128_c.skel.h"
>>   #include "aggregate_ret_struct_c.skel.h"
>>   #include "aggregate_ret_kfunc_c.skel.h"
>> +#include "aggregate_ret_func.skel.h"
>> +#include "aggregate_ret_kfunc.skel.h"
>>
>>   void test_aggregate_ret(void)
>>   {
>>   	RUN_TESTS(aggregate_ret_int128_c);
>>   	RUN_TESTS(aggregate_ret_struct_c);
>>   	RUN_TESTS(aggregate_ret_kfunc_c);
>> +	RUN_TESTS(aggregate_ret_func);
>> +	RUN_TESTS(aggregate_ret_kfunc);
>>   }
> [ ... ]
>
>> diff --git a/tools/testing/selftests/bpf/progs/aggregate_ret_func.c b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> new file mode 100644
>> index 000000000000..cfb21bcf704b
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> @@ -0,0 +1,260 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
>> +#include <linux/bpf.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include "bpf_misc.h"
> [ ... ]
>
>> +SEC("tc")
>> +__load_if_JITed()
>> +__success __retval(0)
>> +__log_level(2)
>> +__msg("mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 4: (07) r1 += -8")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 3: (bf) r1 = r10")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 2: (57) r6 &= 7")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 1: (bf) r6 = r2")
>> +__msg("mark_precise: frame0: regs=r2 stack= before 12: (95) exit")
>> +__msg("mark_precise: frame1: regs=r2 stack= before 11: (b7) r2 = 4")
>> +__naked int aggregate_ret_static_precise(void)
> [ ... ]
>
>> +SEC("tc")
>> +__load_if_JITed()
>> +__success __retval(0)
>> +__log_level(2)
>> +__msg("mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 4: (07) r1 += -8")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 3: (bf) r1 = r10")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 2: (57) r6 &= 7")
>> +__msg("mark_precise: frame0: regs=r6 stack= before 1: (bf) r6 = r2")
>> +__msg("mark_precise: frame0: regs=r2 stack= before 0: (85) call pc+9")
>> +__naked int aggregate_ret_global_precise(void)
> A subsystem pattern flags these two tests as potentially concerning.
> The aggregate_ret_static_precise() and aggregate_ret_global_precise()
> tests in the new aggregate_ret_func.c file are the R2 variant of
> behavior that tools/testing/selftests/bpf/progs/verifier_subprog_precision.c
> already covers: subprog_result_precise() and global_subprog_result_precise()
> in that file assert the same mark_precise log shape across the call/exit
> boundary for static and global subprogs. The two new tests differ only in
> which register (R2 vs R0) is being tracked, and in the final one or two
> __msg lines.
>
> This same commit did extend an existing file for its liveness test case
> rather than adding a new one (it added aux2() and subprog_ret_reg_pair()
> to compute_live_registers.c).
>
> Would it be more natural to add these two precision tests as new cases
> in verifier_subprog_precision.c, where a reader looks for precision-
> backtracking coverage? The rest of aggregate_ret_func.c (the __failure
> cases and the accepted pointer-pair case) is genuinely new-feature
> coverage with no existing home, so perhaps that part stays separate.

I don't have a strong opinion. But current file aggregate_ret_func.c
seems okay as well since there are some other returning R0:R1 tests.

>
> [ ... ]
>
>> diff --git a/tools/testing/selftests/bpf/progs/compute_live_registers.c b/tools/testing/selftests/bpf/progs/compute_live_registers.c
>> index d055fc7b3b95..0be9441ec273 100644
>> --- a/tools/testing/selftests/bpf/progs/compute_live_registers.c
>> +++ b/tools/testing/selftests/bpf/progs/compute_live_registers.c
>> @@ -431,6 +431,36 @@ __naked void subprog1(void)
>>   		::: __clobber_all);
>>   }
>>
>> +static __used __naked unsigned __int128 aux2(void)
>> +{
>> +	asm volatile (
>> +		"r0 = 1;"
>> +		"r2 = 2;"
>> +		"exit;"
>> +		::: __clobber_all);
>> +}
>> +
>> +SEC("socket")
>> +/* A program observing the pair needs the JIT; see bpf_compute_subprog_ret_regs(). */
>> +__load_if_JITed()
>> +__log_level(2)
>> +__msg("0: .12345.... (85) call pc+2")
>> +__msg("1: ..2....... (bf) r0 = r2")
>> +/* R2 is not read at the exit of this program, which returns an int, ... */
>> +__msg("2: 0......... (95) exit")
>> +__msg("3: .......... (b7) r0 = 1")
>> +__msg("4: 0......... (b7) r2 = 2")
>> +/* ... but it is at the exit of aux2(), which returns a register pair. */
>> +__msg("5: 0.2....... (95) exit")
>> +__naked void subprog_ret_reg_pair(void)
> The changelog describes aggregate_ret_func.c covering "liveness at the exit
> of a subprogram returning a pair," but that test is not in that file.
> aggregate_ret_func.c has no liveness test at all - its only __log_level(2)
> tests match "mark_precise: ..." lines.
>
> The liveness-at-exit test is here, in compute_live_registers.c: aux2() and
> subprog_ret_reg_pair(), with the register-mask __msg expectations like
> "0: .12345.... (85) call pc+2" and "5: 0.2....... (95) exit".
>
> Conversely, the changelog does not mention the accepted-case test
> aggregate_ret_static_ptr_pair() in aggregate_ret_func.c (a static subprog
> legitimately returning a pointer in R2, whose result is then dereferenced),
> and it never mentions the aggregate_ret_kfunc.c / aggregate_ret_kfunc_c.c
> additions that make up roughly 180 of the commit's added lines. New kfuncs
> in the test module are the kind of addition a reviewer looks for in the
> changelog.
>
> Could the message be adjusted so that aggregate_ret_func.c's description
> matches what the file actually contains, the liveness test is attributed to
> compute_live_registers.c, and the kfunc side of the patch gets a sentence?

I think this is unnecessary. The code itself will show which files have
which 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/31740414277