Re: [PATCH bpf-next v6 08/10] selftests/bpf: Add C tests for 16-byte returns in R0:R2

On 8/16/26 9:45 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The kfuncs `bpf_kfunc_call_test_i128` and `bpf_kfunc_call_test_ret_pair` are conditionally compiled out of `bpf_testmod.ko` on architectures other than x86_64 and arm64, but are unconditionally compiled and declared as strong `__ksym` externs in the selftest object `verifier_aggregate_ret.c`. This will cause libbpf to fail loading the entire object on unsupported architectures (when compiled with LLVM 23+), failing the entire test suite even for programs that don't use the kfuncs.
> --
>
> --- Patch [8]: [PATCH bpf-next v6 08/10] selftests/bpf: Add C tests for 16-byte returns in R0:R2 ---
> commit 00f68ada7c423dbadb8514b3c5f6c309f446bc99
> Author: Yonghong Song <[email protected]>
>
> selftests/bpf: Add C tests for 16-byte returns in R0:R2
>
> Add selftests that exercise a 16-byte return value passed in the R0:R2
> register pair, written in C so that they depend on the compiler lowering
> the register-pair return.
>
> The R0:R2 convention is only emitted by LLVM 23 and newer, and a by-value
> aggregate return does not compile at all before that, so the programs sit
> behind a __clang_major__ guard. An older compiler builds the dummy test in
> the #else branch instead, which keeps the object non-empty and says in its
> description why nothing was exercised.
>
> The kfunc tests are tagged __arch_x86_64/__arch_arm64 and skip elsewhere.
> Those are the architectures whose JIT advertises
> bpf_jit_supports_kfunc_ret_reg_pair(), which bpf_add_kfunc_call() requires
> before it accepts a kfunc returning more than 8 bytes, and they are also
> the only ones building the kfuncs.
>
> Acked-by: Eduard Zingerman <[email protected]>
> Signed-off-by: Yonghong Song <[email protected]>
>
>> diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> index 7d81070eefe7f..426000f9a14fa 100644
>> --- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
>> +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_kfunc.h
> [ ... ]
>> @@ -131,6 +136,10 @@ int bpf_kfunc_call_test2(struct sock *sk, __u32 a, __u32 b) __ksym;
>>   struct sock *bpf_kfunc_call_test3(struct sock *sk) __ksym;
>>   long bpf_kfunc_call_test4(signed char a, short b, int c, long d) __ksym;
>>   int bpf_kfunc_call_test5(__u8 a, __u16 b, __u32 c) __ksym;
>> +#ifdef __SIZEOF_INT128__
>> +__int128 bpf_kfunc_call_test_i128(__u64 a, __u64 b) __ksym;
>> +#endif
>> +struct prog_test_ret_pair bpf_kfunc_call_test_ret_pair(__u64 a, __u64 b) __ksym;
> [Severity: Medium]
> Should these kfuncs be declared with the __weak attribute?
>
> While the test programs in verifier_aggregate_ret.c use the __arch_x86_64 and
> __arch_arm64 tags, they still unconditionally reference these kfuncs in the
> C code. Without the __weak attribute, Clang emits them as strong externs in
> the BPF object file.
>
> When libbpf attempts to load the verifier_aggregate_ret object on unsupported
> architectures (when compiled with LLVM 23+), won't it fail to resolve these
> strong externs because they are conditionally compiled out of bpf_testmod.ko?
> This could cause the entire object load to fail, preventing even the
> non-kfunc tests in the same object from running.

I think we are fine here. We can revisit this if newer architecture (e.g., riscv, s390x)
needs __weak attribute in the future.