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

[Mike Leach]

Hi Ian,

On Fri, 6 Jan 2023 at 06:24, Ian Rogers <irogers@xxxxxxxxxx> wrote:
>
> 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
>

Applying your new patch to perf/core and building I get:-

  CC      builtin-stat.o
In file included from builtin-stat.c:71:
util/bpf_counter.h:7:10: fatal error: bpf/bpf.h: No such file or directory
    7 | #include <bpf/bpf.h>
      |          ^~~~~~~~~~~
compilation terminated.
/datadisk/mike/work/kernel-ups/tools/build/Makefile.build:96: recipe
for target 'builtin-stat.o' failed
make[3]: *** [builtin-stat.o] Error 1
make[3]: *** Waiting for unfinished jobs....
  LD      pmu-events/pmu-events-in.o
Makefile.perf:673: recipe for target 'perf-in.o' failed
make[2]: *** [perf-in.o] Error 2
Makefile.perf:235: recipe for target 'sub-make' failed
make[1]: *** [sub-make] Error 2
Makefile:69: recipe for target 'all' failed
make: *** [all] Error 2

which is a result of the bpf headers not being installed in their new
location and the removal of the -I from the old location as mentioned
in my last.
So perhaps the issue is less about the build operations and more about
the lack of #ifdef HAVE_LIBBPF_SUPPORT in certain other source files.

However, if I put the #ifdef HAVE_LIBBPF_SUPPORT around the #include
of util/bpf_counter.h, then compilation fails with multiple

builtin-stat.c: In function ‘read_bpf_map_counters’:
builtin-stat.c:463:9: error: implicit declaration of function
‘bpf_counter__read’; did you mean ‘refcount_read’?
[-Werror=implicit-function-declaration]
  463 |   err = bpf_counter__read(counter);
      |         ^~~~~~~~~~~~~~~~~
      |         refcount_read

type errors.

Turns out that bpf_counter.h has inline stubs for these functions
bracketed by #ifdef HAVE_BPF_SKEL / #else / #endif, which I presume
are used in the non-bpf case.

I can get a clean build with your patch if I adjust the HAVE_BFP_SKEL
bracketing to encompass everything (including header includes, struct
defines and other functions) other than the stubs in the #ifdef case
and only the stubs in the #else case  - but I have no idea if this
will have an adverse effect on other tools which may use the same
header.

Thanks and Regards

Mike

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



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK