Re: [PATCH 7/7] bpf: Add tests for new BPF atomic operations

[Brendan Jackman <jackmanb@google.com>]

On Mon, Nov 23, 2020 at 04:26:45PM -0800, Yonghong Song wrote:
> On 11/23/20 9:32 AM, Brendan Jackman wrote:
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 3d5940cd110d..4e28640ca2d8 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -250,7 +250,7 @@ define CLANG_BPF_BUILD_RULE
> >   	$(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2)
> >   	$(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm			\
> >   		-c $1 -o - || echo "BPF obj compilation failed") | 	\
> > -	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2
> > +	$(LLC) -mattr=dwarfris -march=bpf -mcpu=v4 $4 -filetype=obj -o $2
> 
> We have an issue here. If we change -mcpu=v4 here, we will force
> people to use trunk llvm to run selftests which is not a good idea.
> 
> I am wondering whether we can single out progs/atomics_test.c, which will be
> compiled with -mcpu=v4 and run with test_progs.
> 
> test_progs-no_alu32 runs tests without alu32. Since -mcpu=v4 implies
> alu32, atomic tests should be skipped in test_progs-no-alu32.
> 
> In bpf_helpers.h, we already use __clang_major__ macro to compare
> to clang version,
> 
> #if __clang_major__ >= 8 && defined(__bpf__)
> static __always_inline void
> bpf_tail_call_static(void *ctx, const void *map, const __u32 slot)
> {
>         if (!__builtin_constant_p(slot))
>                 __bpf_unreachable();
> ...
> 
> I think we could also use __clang_major__ in progs/atomics_test.c
> to enable tested intrinsics only if __clang_major__ >= 12? This
> way, the same code can be compiled with -mcpu=v2/v3.
> 
> Alternatively, you can also use a macro at clang command line like
>    clang -mcpu=v4 -DENABLE_ATOMIC ...
>    clang -mcpu=v3/v2 ...
> 
> progs/atomics_test.c:
>    #ifdef ENABLE_ATOMIC
>      ... atomic_intrinsics ...
>    #endif

Ah - all good points, thanks. Looks like tools/build/feature/ might
offer a solution here, I'll investigate.

> > diff --git a/tools/testing/selftests/bpf/progs/atomics_test.c b/tools/testing/selftests/bpf/progs/atomics_test.c
> > new file mode 100644
> > index 000000000000..d81f45eb6c45
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/atomics_test.c
> > @@ -0,0 +1,61 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +__u64 add64_value = 1;
> > +__u64 add64_result;
> > +__u32 add32_value = 1;
> > +__u32 add32_result;
> > +__u64 add_stack_value_copy;
> > +__u64 add_stack_result;
> 
> To please llvm10, let us initialize all unitialized globals explicitly like
>    __u64 add64_result = 0;
>    __u32 add32_result = 0;
>    ...
> 
> llvm11 and above are okay but llvm10 put those uninitialized globals
> into COM section (not .bss or .data sections) which BTF did not
> handle them.

Thanks, will initialise everything explicitly.