Re: [PATCH v3 bpf-next 5/5] selftests/bpf: Add bench for task storage creation

[Martin KaFai Lau <martin.lau@linux.dev>]

On 3/29/23 12:12 PM, James Hilliard wrote:
> On Wed, Mar 29, 2023 at 11:03 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 3/27/23 8:51 PM, James Hilliard wrote:
>>>> diff --git a/tools/testing/selftests/bpf/progs/bench_local_storage_create.c b/tools/testing/selftests/bpf/progs/bench_local_storage_create.c
>>>> index 2814bab54d28..7c851c9d5e47 100644
>>>> --- a/tools/testing/selftests/bpf/progs/bench_local_storage_create.c
>>>> +++ b/tools/testing/selftests/bpf/progs/bench_local_storage_create.c
>>>> @@ -22,6 +22,13 @@ struct {
>>>>           __type(value, struct storage);
>>>>    } sk_storage_map SEC(".maps");
>>>>
>>>> +struct {
>>>> +       __uint(type, BPF_MAP_TYPE_TASK_STORAGE);
>>>> +       __uint(map_flags, BPF_F_NO_PREALLOC);
>>>> +       __type(key, int);
>>>> +       __type(value, struct storage);
>>>> +} task_storage_map SEC(".maps");
>>>> +
>>>>    SEC("raw_tp/kmalloc")
>>>>    int BPF_PROG(kmalloc, unsigned long call_site, const void *ptr,
>>>>                size_t bytes_req, size_t bytes_alloc, gfp_t gfp_flags,
>>>> @@ -32,6 +39,24 @@ int BPF_PROG(kmalloc, unsigned long call_site, const void *ptr,
>>>>           return 0;
>>>>    }
>>>>
>>>> +SEC("tp_btf/sched_process_fork")
>>>> +int BPF_PROG(fork, struct task_struct *parent, struct task_struct *child)
>>>
>>> Apparently fork is a built-in function in bpf-gcc:
>>
>> It is also failing in a plain C program
>>
>> #>  gcc -Werror=builtin-declaration-mismatch -o test test.c
>> test.c:14:35: error: conflicting types for built-in function ‘fork’; expected
>> ‘int(void)’ [-Werror=builtin-declaration-mismatch]
>>      14 | int __attribute__((__noinline__)) fork(long x, long y)
>>         |                                   ^~~~
>> cc1: some warnings being treated as errors
>>
>> #> clang -o test test.c
>> succeed
>>
>> I am not too attached to the name but it seems something should be addressed in
>> the gcc instead.
> 
> Hmm, so it looks like it's marked as a builtin here:
> https://github.com/gcc-mirror/gcc/blob/releases/gcc-12.1.0/gcc/builtins.def#L875
> 
> The macro for that is here:
> https://github.com/gcc-mirror/gcc/blob/releases/gcc-12.1.0/gcc/builtins.def#L104-L111
> 
> Which has this comment:
> /* Like DEF_LIB_BUILTIN, except that the function is not one that is
> specified by ANSI/ISO C. So, when we're being fully conformant we
> ignore the version of these builtins that does not begin with
> __builtin. */
> 
> Looks like this builtin was originally added here:
> https://github.com/gcc-mirror/gcc/commit/d1c38823924506d389ca58d02926ace21bdf82fa
> 
> Based on this issue it looks like fork is treated as a builtin for
> libgcov support:
> https://gcc.gnu.org/bugzilla//show_bug.cgi?id=82457
> 
> So from my understanding fork is a gcc builtin when building with -std=gnu11
> but is not a builtin when building with -std=c11.

That sounds like there is a knob to turn this behavior on and off. Do the same 
for the bpf target?

> 
> So it looks like fork is translated to __gcov_fork when -std=gnu* is set which
> is why we get this error.
> 
> As this appears to be intended behavior for gcc I think the best option is
> to just rename the function so that we don't run into issues when building
> with gnu extensions like -std=gnu11.

Is it sure 'fork' is the only culprit? If not, it is better to address it 
properly because this unnecessary name change is annoying when switching bpf 
prog from clang to gcc. Like changing the name in this .c here has to make 
another change to the .c in the prog_tests/ directory.

> 
>>
>>>
>>> In file included from progs/bench_local_storage_create.c:6:
>>> progs/bench_local_storage_create.c:43:14: error: conflicting types for
>>> built-in function 'fork'; expected 'int(void)'
>>> [-Werror=builtin-declaration-mismatch]
>>>      43 | int BPF_PROG(fork, struct task_struct *parent, struct
>>> task_struct *child)
>>>         |              ^~~~
>>>
>>> I haven't been able to find this documented anywhere however.
>>