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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/29/23 12:12 PM, James Hilliard wrote:
On Wed, Mar 29, 2023 at 11:03 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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.





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux