Re: [PATCH bpf-next 11/12] selftests/bpf: Add inline-asm tests for by-value arguments

[Yonghong Song <yonghong.song@linux.dev>]

On 9/3/26 11:09 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/testing/selftests/bpf/prog_tests/aggregate_arg.c b/tools/testing/selftests/bpf/prog_tests/aggregate_arg.c
>> new file mode 100644
>> index 000000000000..b230f3bd3b2a
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/prog_tests/aggregate_arg.c
>> @@ -0,0 +1,9 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
>> +#include <test_progs.h>
>> +#include "aggregate_arg_func.skel.h"
>> +
>> +void test_aggregate_arg(void)
>> +{
>> +	RUN_TESTS(aggregate_arg_func);
>> +}
>> diff --git a/tools/testing/selftests/bpf/progs/aggregate_arg_func.c b/tools/testing/selftests/bpf/progs/aggregate_arg_func.c
>> new file mode 100644
>> index 000000000000..61dd5f86c3a4
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/aggregate_arg_func.c
>> @@ -0,0 +1,155 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
>> +#include <vmlinux.h>
>> +#include <bpf/bpf_helpers.h>
>> +#include "bpf_misc.h"
>> +
>> +typedef unsigned __int128 u128;
>> +
>> +struct pair {
>> +	__u64 lo;
>> +	__u64 hi;
>> +};
> [ ... ]
>
>> +#if defined(__BPF_FEATURE_STACK_ARGUMENT)
>> +
>> +__noinline __u64 global_arg_split(int a, int b, int c, int d, struct pair p)
>> +{
>> +	return (__u64)a + b + c + d + p.lo + p.hi;
>> +}
>> +
>> +SEC("tc")
>> +__failure __msg("global function global_arg_split() needs 6 > 5 argument slots")
>> +__naked int aggregate_arg_split_fail(void)
> [ ... ]
>
>> +SEC("tc")
>> +__failure __msg("global function global_arg_past_regs() needs 7 > 5 argument slots")
>> +__naked int aggregate_arg_past_regs_fail(void)
> [ ... ]
>
>> +SEC("tc")
>> +__failure __msg("global function global_arg_i128_slots() needs 6 > 5 argument slots")
>> +__naked int aggregate_arg_i128_slots_fail(void)
> [ ... ]
>
> A subsystem pattern flags this as potentially concerning: this commit
> creates a new BPF prog file (progs/aggregate_arg_func.c) plus a new
> test_progs entry point (prog_tests/aggregate_arg.c) for by-value
> argument tests, while the immediately preceding commit in the same series
> (7bdde6e0b83d) added by-value aggregate argument tests to the existing
> progs/verifier_int128_arg.c, which is already run by
> prog_tests/verifier.c.
>
> This means coverage for one feature is now split across two test runners
> in the same directory, and struct pair is defined twice
> (progs/verifier_int128_arg.c and progs/aggregate_arg_func.c).
>
> Three of the six new subtests (aggregate_arg_split_fail,
> aggregate_arg_past_regs_fail, aggregate_arg_i128_slots_fail) are pure
> __failure/__msg verifier tests that need no hand-written register layout,
> so they could fit as cases in the existing file. The same is true of
> aggregate_arg_pair_ptr_fail.
>
> Is there a reason to create a separate test file rather than adding these
> tests to the existing progs/verifier_int128_arg.c, which already covers
> by-value aggregate arguments and would consolidate the feature's tests in
> one place?
>
> (For context: the pattern is motivated by the fact that aggregate_arg_pair_asm
> and aggregate_arg_pair_ptr_fail need hand-written register placement that
> C cannot express, and this layout mirrors the existing aggregate_ret
> structure with prog_tests/aggregate_ret.c + progs/aggregate_ret_func.c.
> A following commit extends the test with kfunc cases, so the new entry
> point is reused rather than single-purpose. The question is whether the
> C tests in verifier_int128_arg.c should move here too, so the feature's
> tests live in one place and struct pair is defined once.)

I think it is okay.

>
>
> ---
> 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/33840714551