Re: [PATCH bpf-next 10/12] selftests/bpf: Add C tests for by-value arguments up to 16 bytes

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

On 9/3/26 10:19 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] `take_with_ptr_global` and its test are missing the `#if defined(__clang__)` guard used for other global functions taking structs by value, which will cause the test to fail when built with GCC.
> - [Medium] `take_two_pairs_global` lacks the surrounding `int` arguments explicitly promised in the commit message to catch slot counting bugs.
> --
>
> commit a3823ad45ae612d86e218eeec89944cc894c7843
> Author: Yonghong Song <yonghong.song@linux.dev>
>
> selftests/bpf: Add C tests for by-value arguments up to 16 bytes
>
> This commit extends the by-value argument tests with aggregate cases written
> in C. It relies on the compiler to lower arguments into register pairs instead
> of using hand-written register layouts.
>
>> diff --git a/tools/testing/selftests/bpf/progs/verifier_int128_arg.c b/tools/testing/selftests/bpf/progs/verifier_int128_arg.c
>> index 044fc4a800554..b10fb4aeb4056 100644
>> --- a/tools/testing/selftests/bpf/progs/verifier_int128_arg.c
>> +++ b/tools/testing/selftests/bpf/progs/verifier_int128_arg.c
> [ ... ]
>> +__noinline __u64 take_two_pairs_global(struct pair p, struct pair q)
>> +{
>> +	return p.lo + p.hi + q.lo + q.hi;
>> +}
>> +
>> +SEC("tc")
>> +__success __retval(0)
>> +int aggregate_arg_two_structs_c_test(struct __sk_buff *skb)
>> +{
>> +	__u64 a = skb->len ^ MIX_A;
>> +	__u64 b = skb->len ^ MIX_B;
>> +	struct pair p = { .lo = a, .hi = b };
>> +	struct pair q = { .lo = b, .hi = a };
>> +
>> +	if (take_two_pairs_global(p, q) != 2 * (a + b))
>> +		return 1;
>> +
>> +	return 0;
>> +}
> [Severity: Medium]
> Does this function signature and test implementation match the strategy
> described in the commit message?
>
> The commit message explicitly states a defensive testing strategy of having
> "two struct arguments in a row, each with an int argument on either side so
> that a wrong slot count shows up as a wrong value in the parameters around
> it".
>
> However, take_two_pairs_global completely omits these surrounding int
> arguments. Can these be added to properly validate register slot counting?

I will change commit message. With previous kernel support, it won't have wrong
slot count.

>
> [ ... ]
>> +__noinline __u64 take_with_ptr_global(struct with_ptr s)
>> +{
>> +	return s.x;
>> +}
>> +
>> +SEC("tc")
>> +__failure __msg("type STRUCT in take_with_ptr_global() is not composed of scalars")
>> +int aggregate_arg_ptr_member_fail(struct __sk_buff *skb)
>> +{
>> +	struct with_ptr s = { .p = skb, .x = skb->len };
>> +
>> +	return take_with_ptr_global(s);
>> +}
> [Severity: Medium]
> Should take_with_ptr_global and its test aggregate_arg_ptr_member_fail
> be enclosed within the #if defined(__clang__) guard?
>
> Other global functions taking structs by value in this file are guarded.
> Since GCC passes aggregates by invisible reference, it will produce a
> different BTF signature (a pointer instead of a struct).
>
> Will this cause the test to fail when compiled with GCC, as the verifier
> might not emit the specific "type STRUCT ... is not composed of scalars"
> failure message?

There is no need to guard with __clang__. This is a negative test
where both gcc and clang will fail.

>