BPF Archive on lore.kernel.org
 help / color / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Jiri Olsa <jolsa@kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andriin@fb.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Brendan Jackman <jackmanb@google.com>,
	KP Singh <kpsingh@google.com>
Subject: Re: [PATCH bpf-next] selftests/bpf: Fix selftest compilation on clang 11
Date: Wed, 9 Dec 2020 12:24:23 -0800
Message-ID: <CAEf4BzYBddPaEzRUs=jaWSo5kbf=LZdb7geAUVj85GxLQztuAQ@mail.gmail.com> (raw)
In-Reply-To: <20201209142912.99145-1-jolsa@kernel.org>

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
>

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-09 14:29 Jiri Olsa
2020-12-09 20:24 ` Andrii Nakryiko [this message]
2020-12-10 16:15   ` Jiri Olsa
     [not found]     ` <CANA3-0dvt3FH8=ZYO7CfW0YKeQemNcsP76j441wW31-WE1_o4A@mail.gmail.com>
2020-12-10 16:36       ` Daniel Borkmann
2020-12-10 16:00 ` patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEf4BzYBddPaEzRUs=jaWSo5kbf=LZdb7geAUVj85GxLQztuAQ@mail.gmail.com' \
    --to=andrii.nakryiko@gmail.com \
    --cc=andriin@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jackmanb@google.com \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kafai@fb.com \
    --cc=kpsingh@chromium.org \
    --cc=kpsingh@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git