Re: [PATCH bpf-next v2 09/10] selftests/bpf: Test global functions returning arena pointers by value

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

On 8/25/26 2:59 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/testing/selftests/bpf/progs/aggregate_ret_func.c b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> index 6f66fc822cede..237adb8e5ee1e 100644
>> --- a/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> +++ b/tools/testing/selftests/bpf/progs/aggregate_ret_func.c
>> @@ -2,6 +2,7 @@
>>   /* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
>>   #include <linux/bpf.h>
>>   #include <bpf/bpf_helpers.h>
>> +#include <bpf_arena_common.h>
>>   #include "bpf_misc.h"
> [ ... ]
>
>> @@ -234,4 +235,121 @@ __naked int aggregate_ret_global_union_ptr_fail(void)
>>
>>   #endif
>>
>> +/*
>> + * gcc returns a by-value struct through a hidden pointer, and emits the
>> + * 'r0 = r1' returning it after the __naked body's exit, leaving the
>> + * subprogram falling through. Build these with clang only.
>> + */
>> +#if defined(__clang__)
> Should this use the same clang version check as the structurally identical
> block at line 178? That block checks '#if defined(__clang_major__) &&
> __clang_major__ >= 23' for __naked functions returning by-value structs:
>
> aggregate_ret_func.c:178
> #if defined(__clang_major__) && __clang_major__ >= 23
> ...
> __naked struct with_ptr global_ret_struct_ptr(void)
> {
> 	asm volatile (
> 	"r0 = 0;"
> 	"r2 = 0;"
> 	"exit;"
> 	);
> }
>
> SEC("tc")
> __load_if_JITed()
> __failure __msg("has unsupported return type")
> __naked int aggregate_ret_global_struct_ptr_fail(void)
> {
> 	asm volatile (
> 	"call %[global_ret_struct_ptr];"
> 	"r0 = 0;"
> 	"exit;"
> 	:
> 	: __imm(global_ret_struct_ptr)
> 	: __clobber_all);
> }
>
> The verifier_arena.c hunk added in this same commit also uses the '>= 23'
> guard:
>
> verifier_arena.c:737
> #if defined(__clang_major__) && __clang_major__ >= 23
>
> struct arena_page_pair {
> 	u32 __arena *first;
> 	u32 __arena *second;
> };
>
> __weak struct arena_page_pair split_arena_page(u32 __arena *page)
> {
> 	...
> }
>
> By-value struct returns in registers are an LLVM 23 BPF-ABI feature. With
> an older clang, the return is lowered through a hidden pointer (sret), which
> is the gcc behavior described in your new comment.
>
> With clang < 23, the four new '__success __retval(0)' tests would be
> compiled against a different signature than intended. The callers never
> initialize r1, so they would be checked against a prototype that no longer
> matches 'returns struct arena_pair' in the BTF.
>
> Can the two adjacent blocks be consistent about which toolchains can build
> the same construct?

The condition "defined(__clang_major__) && __clang_major__ >= 23"
is needed since returning two registers is only supported at >= llvm23.

Another case is for __clang__ guard. For naked function, clang compilation
will maintain naked asm code. But for gcc, even for naked function, it
still check the return type during lowering and at that point, gcc will
change e.g. 16-byte return type to a sret, which is not what we want.

I will make sure this is explained in commit message.

>
>> +
>> +struct arena_pair {
>> +	void __arena *lo;
>> +	void __arena *hi;
>> +};
>> +
>> +struct arena_and_scalar {
>> +	void __arena *p;
>> +	__u64 x;
>> +};
>> +
>> +struct arena_array {
>> +	void __arena *p[2];
>> +};
>> +
>> +struct arena_single {
>> +	void __arena *p;
>> +};
>> +
>> +__naked struct arena_pair global_ret_arena_pair(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0;"
>> +	"r2 = 0;"
>> +	"exit;"
>> +	);
>> +}
>> +
>> +SEC("tc")
>> +__load_if_JITed()
>> +__success __retval(0)
>> +__naked int aggregate_ret_global_arena_pair(void)
>> +{
>> +	asm volatile (
>> +	"call %[global_ret_arena_pair];"
>> +	"r0 = 0;"
>> +	"exit;"
>> +	:
>> +	: __imm(global_ret_arena_pair)
>> +	: __clobber_all);
>> +}
>> +
>> +__naked struct arena_and_scalar global_ret_arena_and_scalar(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0;"
>> +	"r2 = 0;"
>> +	"exit;"
>> +	);
>> +}
>> +
>> +SEC("tc")
>> +__load_if_JITed()
>> +__success __retval(0)
>> +__naked int aggregate_ret_global_arena_and_scalar(void)
>> +{
>> +	asm volatile (
>> +	"call %[global_ret_arena_and_scalar];"
>> +	"r0 = 0;"
>> +	"exit;"
>> +	:
>> +	: __imm(global_ret_arena_and_scalar)
>> +	: __clobber_all);
>> +}
>> +
>> +__naked struct arena_array global_ret_arena_array(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0;"
>> +	"r2 = 0;"
>> +	"exit;"
>> +	);
>> +}
>> +
>> +SEC("tc")
>> +__load_if_JITed()
>> +__success __retval(0)
>> +__naked int aggregate_ret_global_arena_array(void)
>> +{
>> +	asm volatile (
>> +	"call %[global_ret_arena_array];"
>> +	"r0 = 0;"
>> +	"exit;"
>> +	:
>> +	: __imm(global_ret_arena_array)
>> +	: __clobber_all);
>> +}
>> +
>> +__naked struct arena_single global_ret_arena_single(void)
>> +{
>> +	asm volatile (
>> +	"r0 = 0;"
>> +	"exit;"
>> +	);
>> +}
>> +
>> +SEC("tc")
>> +__success __retval(0)
>> +__naked int aggregate_ret_global_arena_single(void)
>> +{
>> +	asm volatile (
>> +	"call %[global_ret_arena_single];"
>> +	"r0 = 0;"
>> +	"exit;"
>> +	:
>> +	: __imm(global_ret_arena_single)
>> +	: __clobber_all);
>> +}
>> +
>> +#endif
>> +
>>   char _license[] SEC("license") = "GPL";
> [ ... ]
>
>
> ---
> 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/32899532405