Re: [PATCH bpf-next] selftests/bpf: Fix selftest compilation on clang 11

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Wed, Dec 9, 2020 at 7:16 AM Jiri Olsa <jolsa@kernel.org> 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.

So given minimal additions code-wise, but also considering all the above:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  .../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
>