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.
