Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1

[Ian Rogers]

On Thu, Jan 5, 2023 at 3:40 PM Mike Leach <mike.leach@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Thu, 5 Jan 2023 at 19:03, Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > On Thu, Jan 5, 2023 at 9:22 AM Mike Leach <mike.leach@xxxxxxxxxx> wrote:
> > >
> > > Recent updates to perf build result in the following output when cross
> > > compiling to aarch64, with libelf unavailable, and therefore
> > > NO_LIBBPF=1 set.
> > >
> > > ```
> > >   $make -C tools/perf
> > >
> > >   <cut>
> > >
> > >   Makefile.config:428: No libelf found. Disables 'probe' tool, jvmti
> > >   and BPF support in 'perf record'. Please install libelf-dev,
> > >   libelf-devel or elfutils-libelf-devel
> > >
> > >   <cut>
> > >
> > >   libbpf.c:46:10: fatal error: libelf.h: No such file or directory
> > >       46 | #include <libelf.h>
> > >          |          ^~~~~~~~~~
> > >   compilation terminated.
> > >
> > >   ./tools/build/Makefile.build:96: recipe for target
> > >   '.tools/perf/libbpf/staticobjs/libbpf.o' failed
> > >
> > > ```
> > >
> > > plus one other include error for <gelf.h>
> >
> > Ouch, apologies for the breakage. You wouldn't happen to have
> > something like a way with say a docker image to repro the problem? The
> > make line above is somewhat minimal.
> >
>
> Unfortunately not - I was cross compiling on my main workstation.
> However, in theory
> $make -C tools/perf NO_LIBBPF=1
> should explicitly exclude the library from the build - which without
> the fix it does not.
>
> > > The issue is that the commit noted below adds libbpf to the prepare:
> > > target but no longer accounts for the NO_LIBBPF define. Additionally
> > > changing the include directories means that even if the libbpf target
> > > build is prevented, bpf headers are missing in other parts of the build.
> > >
> > > This patch ensures that in the case of NO_LIBBPF=1, the build target is
> > > changed to a header only target, and the headers are installed, without
> > > attempting to build the libbpf.a target.
> > >
> > > Applies to perf/core
> > >
> > > Fixes: 746bd29e348f ("perf build: Use tools/lib headers from install path")
> > > Signed-off-by: Mike Leach <mike.leach@xxxxxxxxxx>
> > > ---
> > >  tools/perf/Makefile.perf | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> > > index 13e7d26e77f0..ee08ecf469f6 100644
> > > --- a/tools/perf/Makefile.perf
> > > +++ b/tools/perf/Makefile.perf
> > > @@ -305,7 +305,11 @@ else
> > >  endif
> > >  LIBBPF_DESTDIR = $(LIBBPF_OUTPUT)
> > >  LIBBPF_INCLUDE = $(LIBBPF_DESTDIR)/include
> > > +ifndef NO_LIBBPF
> > >  LIBBPF = $(LIBBPF_OUTPUT)/libbpf.a
> > > +else
> > > +LIBBPF = $(LIBBPF_INCLUDE)/bpf/bpf.h
> >
> > This seems strange, don't we want to avoid libbpf targets?
> >
>
> This is a header only target - see my continuation comment below....
>
> > > +endif
> > >  CFLAGS += -I$(LIBBPF_OUTPUT)/include
> > >
> > >  ifneq ($(OUTPUT),)
> > > @@ -826,10 +830,16 @@ $(LIBAPI)-clean:
> > >         $(call QUIET_CLEAN, libapi)
> > >         $(Q)$(RM) -r -- $(LIBAPI_OUTPUT)
> > >
> > > +ifndef NO_LIBBPF
> > >  $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
> > >         $(Q)$(MAKE) -C $(LIBBPF_DIR) FEATURES_DUMP=$(FEATURE_DUMP_EXPORT) \
> > >                 O= OUTPUT=$(LIBBPF_OUTPUT)/ DESTDIR=$(LIBBPF_DESTDIR) prefix= \
> > >                 $@ install_headers
> > > +else
> > > +$(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
> > > +       $(Q)$(MAKE) -C $(LIBBPF_DIR) OUTPUT=$(LIBBPF_OUTPUT)/ \
> > > +               DESTDIR=$(LIBBPF_DESTDIR) prefix= install_headers
> > > +endif
> >
> > Shouldn't we just be able to conditionalize having $(LIBBPF) as a
> > dependency for the perf binary? If there is no dependency then the
> > targets won't be built and we shouldn't need to conditionalize here.
> >
>
> I did try doing just that, but the build process does two things when
> building libbpf
> a) builds the library
> b) installs the bpf headers in the libbpf output location.
>
> Now what the original patch  - "perf build: Use tools/lib headers from
> install path"  - does is to also change the include paths to the
> compiler to pick up the headers,
> removing the line:
>
> INC_FLAGS += -I$(srctree)/tools/lib/
>
> from tools/perf/Makefile.config and adding the line
>
> CFLAGS += -I$(LIBBPF_OUTPUT)/include
>
> in tools/perf/Makefile.perf (along with similar lines for libperf, libapi etc)
>
> The result of this is that if you only remove the library build, the
> headers are not installed and other compilation units fail as the
> headers are still included even if the library is not in use.
> These were originally satisfied by the now removed INC_FLAGS +=
> -I$(srctree)/tools/lib.
>
> Thus when NO_LIBBPF=1 even though we do not build the library - we
> still need to install the headers to retain the consistency - hence a
> "header only" target, that only installs the headers without building
> the library.
>
> This avoids restoring the original -I$(srctree)/tools/lib/, which
> would potentially mess up the oher library builds that have changed
> their header include paths.
>
> Regards
>
> Mike


Thanks Mike,

The -I is needed for the libbpf headers but if NO_LIBBPF is enabled
then the C define HAVE_LIBBPF_SUPPORT isn't and we shouldn't include
any of these headers. This means updating the CFLAGS for libbpf should
only be done if we actually build the static  libbpf.a, the dynamic
version's headers should already be on the include path. I sent out a
variant of this fix doing that here:
https://lore.kernel.org/lkml/20230106061631.571659-1-irogers@xxxxxxxxxx/

Apologies again for the breakage, I can buy you a beer the next time
I'm home in Manchester.
Ian

>
> > Thanks!
> > Ian
> >
> > >  $(LIBBPF)-clean:
> > >         $(call QUIET_CLEAN, libbpf)
> > > --
> > > 2.17.1
> > >
>
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK