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

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")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../testing/selftests/bpf/progs/test_core_reloc_module.c  | 8 ++++++++
 1 file changed, 8 insertions(+)

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
>

Hello:

This patch was applied to bpf/bpf-next.git (refs/heads/master):

On Wed,  9 Dec 2020 15:29:12 +0100 you 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);
> 
> [...]

Here is the summary with links:
  - [bpf-next] selftests/bpf: Fix selftest compilation on clang 11
    https://git.kernel.org/bpf/bpf-next/c/41003dd0241c

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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@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.

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@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
> >
>

On 12/10/20 5:28 PM, KP Singh wrote:
> On Thu, Dec 10, 2020 at 5:18 PM Jiri Olsa <jolsa@redhat.com> wrote:
[...]
>>> 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.
> 
> I will clean it up and share it asap and send it as an RFC which
> adds it to tools/testing/selftests/bpf

Thanks!

> We can discuss on the RFC as to where the script would finally end up
> but I think it would save a lot of time/back-and-forth if developers could
> simply check:
> 
>    "Does my change break the BPF CI?"

I'd love to have a Dockerfile under tools/testing/selftests/bpf/ that
replicates the CI env (e.g. busybox, nightly llvm, pahole git, etc) where
we could have quay.io job auto-build this for bpf / bpf-next tree e.g. from a
GH mirror. This would then allow to mount the local kernel tree as a volume
into the container for easy compilation & test access for everyone where we
then don't need all these workarounds like in this patch anymore.

Thanks,
Daniel