Message-ID: <CAP-5=fX4-kmkm+qn9m22O_4A2_8j=uAm=vcXh9x2RqqDKEdnBg@mail.gmail.com>
Date:   Fri, 7 Jan 2022 11:26:50 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Song Liu <songliubraving@...com>, Jiri Olsa <jolsa@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>, bpf <bpf@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Quentin Monnet <quentin@...valent.com>
Subject: Re: perf build broken seemingly due to libbpf changes, checking...

On Fri, Jan 7, 2022 at 10:32 AM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> Em Thu, Jan 06, 2022 at 07:30:34PM -0800, Ian Rogers escreveu:
> > On Thu, Jan 6, 2022 at 2:04 PM Ian Rogers <irogers@...gle.com> wrote:
> > >
> > > On Thu, Jan 6, 2022 at 1:44 PM Andrii Nakryiko
> > > <andrii.nakryiko@...il.com> wrote:
> > > >
> > > > On Thu, Jan 6, 2022 at 1:42 PM Ian Rogers <irogers@...gle.com> wrote:
> > > > >
> > > > > On Thu, Jan 6, 2022 at 11:48 AM Arnaldo Carvalho de Melo
> > > > > <acme@...nel.org> wrote:
> > > > > >
> > > > > > Em Thu, Jan 06, 2022 at 04:44:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > > Em Thu, Jan 06, 2022 at 04:34:46PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > > > > After merging torvalds/master to perf/urgent I'm getting this:
> > > > > > > >
> > > > > > > > util/bpf-event.c:25:21: error: no previous prototype for ‘btf__load_from_kernel_by_id’ [-Werror=missing-prototypes]
> > > > > > > >    25 | struct btf * __weak btf__load_from_kernel_by_id(__u32 id)
> > > > > > > >       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > > util/bpf-event.c:37:1: error: no previous prototype for ‘bpf_object__next_program’ [-Werror=missing-prototypes]
> > > > > > > >    37 | bpf_object__next_program(const struct bpf_object *obj, struct bpf_program *prev)
> > > > > > > >       | ^~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > > util/bpf-event.c:46:1: error: no previous prototype for ‘bpf_object__next_map’ [-Werror=missing-prototypes]
> > > > > > > >    46 | bpf_object__next_map(const struct bpf_object *obj, const struct bpf_map *prev)
> > > > > > > >       | ^~~~~~~~~~~~~~~~~~~~
> > > > > > > > util/bpf-event.c:55:1: error: no previous prototype for ‘btf__raw_data’ [-Werror=missing-prototypes]
> > > > > > > >    55 | btf__raw_data(const struct btf *btf_ro, __u32 *size)
> > > > > > > >       | ^~~~~~~~~~~~~
> > > > > > > > cc1: all warnings being treated as errors
> > > > > > > > make[4]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/bpf-event.o] Error 1
> > > > > > > > make[4]: *** Waiting for unfinished jobs....
> > > > > > > > util/bpf_counter.c: In function ‘bpf_target_prog_name’:
> > > > > > > > util/bpf_counter.c:82:15: error: implicit declaration of function ‘btf__load_from_kernel_by_id’ [-Werror=implicit-function-declaration]
> > > > > > > >    82 |         btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
> > > > > > > >       |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > > util/bpf_counter.c:82:13: error: assignment to ‘struct btf *’ from ‘int’ makes pointer from integer without a cast [-Werror=int-conversion]
> > > > > > > >    82 |         btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
> > > > > > > >       |             ^
> > > > > > > > cc1: all warnings being treated as errors
> > > > > > > > make[4]: *** [/var/home/acme/git/perf/tools/build/Makefile.build:96: /tmp/build/perf/util/bpf_counter.o] Error 1
> > > > > > > >
> > > > > > > > I'm checking now...
> > > > > > > >
> > > > > > > > BTW I test perf builds with:
> > > > > > > >
> > > > > > > > make -k BUILD_BPF_SKEL=1 CORESIGHT=1 PYTHON=python3 O=/tmp/build/perf -C tools/perf install-bin && git status && perf test python
> > > > > > >
> > > > > > > Nevermind, this was due to a patch by Ian Rogers I was testing,
> > > > > > > bisecting get up to the last patch, since I had merged torvalds/master
> > > > > > > today it got me to a wrong correlation, sorry for the disturbance.
> > > > > > >
> > > > > > > For reference, this is the patch:
> > > > > > >
> > > > > > > http://lore.kernel.org/lkml/20220106072627.476524-1-irogers@google.com
> > > > > >
> > > > > > Ian, I have libbpf-devel installed:
> > > > > >
> > > > > > ⬢[acme@...lbox perf]$ rpm -qa | grep libbpf
> > > > > > libbpf-0.4.0-1.fc34.x86_64
> > > > > > libbpf-devel-0.4.0-1.fc34.x86_64
> > > > > > ⬢[acme@...lbox perf]$
> > > > > >
> > > > > > But I'm not using LIBBPF_DYNAMIC=1, so you can't just give precedence to
> > > > > > system headers for all of the homies in tools/lib/.
> > > > > >
> > > > > > I bet that if I remove the libbpf-devel package it works, yeah, just
> > > > > > tested. So we need to make those overrides dependent on using
> > > > > > LIBBPF_DYNAMIC=1, LIBTRACEEVENT_DYNAMIC=1, etc and avoid the big hammer
> > > > > > that is -Itools/lib/, using a more finegrained approach, right?
> > > > >
> > > > > Ugh, this is messy. The -I for tools/lib is overloaded and being used
> > > > > in tools/perf/util/bpf-event.c so that bpf/bpf.h, bpf/btf.h and
> > > >
> > > > can you do `make install` for libbpf instead and have it install
> > > > headers into a dedicated target directory which can be added into -I
> > > > search path. Quentin did this for all the other libbpf users in kernel
> > > > tree (bpftool, resolve_btfids, etc) and it works great.
> > >
> > > This sounds good to me, and being able to borrow code from bpftool
> > > should make writing it is straightforward. I'll try to find time to do
> > > a patch, but I don't mind someone getting there before me :-)
> >
> > So tools/lib also provides subcmd, symbol and api. These will need
> > Makefiles to allow an install and likely the header file structure
> > altering. This seems like too big a fix for the next 5.16rc, wdyt?
>
> Right, I think the best thing is to revert the patch Jiri pointed out,
> right?

Your call. There is a latent bug that with LIBTRACEEVENT_DYNAMIC we
are using tools/lib/traceevent header files. Reverting the change
means we don't break because of this, but it means that people
building with LIBTRACEEVENT_DYNAMIC and newer libtraceevent (at least
my employer :-) ) lose logging. I can carry the change locally, so not
a big loss :-) There are a few issues stemming from this:

1) we've identified the current build is wrong for xxx_DYNAMIC options
as tools/lib versions headers always override
2) to address this we should make the tools/lib things proper
libraries like libbpf, libtraceevent, etc.
3) once we have proper libraries, we need to update the perf build to
build non-dynamic libraries then depend on the built/installed header
files

I expect at least some of this is going to break when testing on many
distributions as that just seems to be what always happens, and
changing the build in this significant way is going to have
implications. Doing this means that the code base is in better shape
and logging works. To counter some of the many distribution pain, do
you have a way to reproduce your testing? My OpenSuSE recipe is:

```
# Get the image
docker pull opensuse/tumbleweed
# Start it with an interactive bash shell and mounting the current
directory as /kernel-src
sudo docker run --privileged -it --net=host --env="DISPLAY" --mount
type=bind,source="$(pwd)",target=/kernel-src opensuse/tumbleweed
/bin/bash
# Install missing rpms
zypper install make gcc diffutils flex bison kernel-devel findutils
libelf-devel python3 kernel-kvmsmall-devel glibc-devel
# Go to /kernel-src and build into /tmp, etc.
```
But finding every distribution, every rpm, etc. is quite laborious.

Thanks,
Ian

> - Arnaldo

Powered by blists - more mailing lists