On Wed, Dec 09, 2020 at 12:24:23PM -0800, Andrii Nakryiko wrote:
> On Wed, Dec 9, 2020 at 7:16 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > We can't compile test_core_reloc_module.c selftest with clang 11,
> > compile fails with:
> >
> > CLNG-LLC [test_maps] test_core_reloc_module.o
> > progs/test_core_reloc_module.c:57:21: error: use of unknown builtin \
> > '__builtin_preserve_type_info' [-Wimplicit-function-declaration]
> > out->read_ctx_sz = bpf_core_type_size(struct bpf_testmod_test_read_ctx);
> >
> > Skipping these tests if __builtin_preserve_type_info() is not
> > supported by compiler.
> >
> > Fixes: 6bcd39d366b6 ("selftests/bpf: Add CO-RE relocs selftest relying on kernel module BTF")
> > Fixes: bc9ed69c79ae ("selftests/bpf: Add tp_btf CO-RE reloc test for modules")
>
> The test isn't really broken, so "Fixes: " tags seem wrong here.
>
> Given core_relo tests have established `data.skip = true` mechanism,
> I'm fine with this patch. But moving forward I think we should
> minimize the amount of feature-detection and tests skipping in
> selftests. The point of selftests is to test the functionality at the
> intersection of 4 projects: kernel, libbpf, pahole and clang. We've
> stated before and I think it remains true that the expectation for
> anyone that wants to develop and run selftests is to track latests
> versions of all 4 of those, sometimes meaning nightly builds or
> building from sources. For clang, which is arguably the hardest of the
> 4 to build from sources, LLVM project publishes nightly builds for
> Ubuntu and Debian, which are very easy to use to get recent enough
> versions for selftests. That's exactly what libbpf CI is doing, BTW.
>
> It's hard and time-consuming enough to develop these features, I'd
> rather keep selftests simpler, more manageable, and less brittle by
> not having excessive amount of feature detection and skipped
> selftests. I think that's the case for BPF atomics as well, btw (cc'ed
> Yonghong and Brendan).
>
> To alleviate some of the pain of setting up the environment, one way
> would be to provide script and/or image to help bring up qemu VM for
> easier testing. To that end, KP Singh (cc'ed) was able to re-use
> libbpf CI's VM setup and make it easier for local development. I hope
> he can share this soon.
ok, that'd be great, thanks for taking this one
jirka
>
> So given minimal additions code-wise, but also considering all the above:
>
> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> > .../testing/selftests/bpf/progs/test_core_reloc_module.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/progs/test_core_reloc_module.c b/tools/testing/selftests/bpf/progs/test_core_reloc_module.c
> > index 56363959f7b0..f59f175c7baf 100644
> > --- a/tools/testing/selftests/bpf/progs/test_core_reloc_module.c
> > +++ b/tools/testing/selftests/bpf/progs/test_core_reloc_module.c
> > @@ -40,6 +40,7 @@ int BPF_PROG(test_core_module_probed,
> > struct task_struct *task,
> > struct bpf_testmod_test_read_ctx *read_ctx)
> > {
> > +#if __has_builtin(__builtin_preserve_enum_value)
> > struct core_reloc_module_output *out = (void *)&data.out;
> > __u64 pid_tgid = bpf_get_current_pid_tgid();
> > __u32 real_tgid = (__u32)(pid_tgid >> 32);
> > @@ -61,6 +62,9 @@ int BPF_PROG(test_core_module_probed,
> > out->len_exists = bpf_core_field_exists(read_ctx->len);
> >
> > out->comm_len = BPF_CORE_READ_STR_INTO(&out->comm, task, comm);
> > +#else
> > + data.skip = true;
> > +#endif
> >
> > return 0;
> > }
> > @@ -70,6 +74,7 @@ int BPF_PROG(test_core_module_direct,
> > struct task_struct *task,
> > struct bpf_testmod_test_read_ctx *read_ctx)
> > {
> > +#if __has_builtin(__builtin_preserve_enum_value)
> > struct core_reloc_module_output *out = (void *)&data.out;
> > __u64 pid_tgid = bpf_get_current_pid_tgid();
> > __u32 real_tgid = (__u32)(pid_tgid >> 32);
> > @@ -91,6 +96,9 @@ int BPF_PROG(test_core_module_direct,
> > out->len_exists = bpf_core_field_exists(read_ctx->len);
> >
> > out->comm_len = BPF_CORE_READ_STR_INTO(&out->comm, task, comm);
> > +#else
> > + data.skip = true;
> > +#endif
> >
> > return 0;
> > }
> > --
> > 2.26.2
> >
>
