| Message ID | 20201209142912.99145-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 41003dd0241c2ceb2461a88a18ff461795f2af57 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Fix selftest compilation on clang 11 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 32 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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
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; }
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(+)