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

[Ian Rogers]

On Fri, Jan 6, 2023 at 1:58 AM Mike Leach <mike.leach@xxxxxxxxxx> wrote:
>
> 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 Mike,

With "apt remove libbpf-devel" I was able to repro this and I've sent
out a v2 [1] adding the guards and moving the helper functions under
HAVE_BPF_SKEL as you say. The patch was sufficient to fix the build
for me, could you double check it? The builtin-trace fix addresses a
missing guard not present since November 2018 in '744fafc787de perf
trace: See if there is a map named "filtered_pids"' but builtin-trace
has some optionality around whether it is included in a build so I
didn't include the Fixes tag.

Thanks,
Ian

[1] https://lore.kernel.org/lkml/20230106142537.607399-2-irogers@xxxxxxxxxx/

> > >
> > > > 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