Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230105172243.7238-1-mike.leach@linaro.org> <CAP-5=fVbPVE4rxJ2s8phhJ5RRH4EnKaWrF2kaT0oCmK6kvhP2g@mail.gmail.com>
In-Reply-To: <CAP-5=fVbPVE4rxJ2s8phhJ5RRH4EnKaWrF2kaT0oCmK6kvhP2g@mail.gmail.com>
From:   Mike Leach <mike.leach@linaro.org>
Date:   Thu, 5 Jan 2023 23:40:42 +0000
Message-ID: <CAJ9a7Vi7cB2t3wao9d78j3G9v4zpO8_hHf8DB4vKOzgT9O+j6g@mail.gmail.com>
Subject: Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1
To:     Ian Rogers <irogers@google.com>
Cc:     linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        bpf@vger.kernel.org, acme@kernel.org, peterz@infradead.org,
        mingo@redhat.com, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, jolsa@kernel.org,
        namhyung@kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Thu, 5 Jan 2023 at 19:03, Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Jan 5, 2023 at 9:22 AM Mike Leach <mike.leach@linaro.org> 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@linaro.org>
> > ---
> >  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!
> Ian
>
> >  $(LIBBPF)-clean:
> >         $(call QUIET_CLEAN, libbpf)
> > --
> > 2.17.1
> >


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