* [PATCH] perf build: Fix build error when NO_LIBBPF=1
@ 2023-01-05 17:22 Mike Leach
2023-01-05 19:02 ` Ian Rogers
0 siblings, 1 reply; 6+ messages in thread
From: Mike Leach @ 2023-01-05 17:22 UTC (permalink / raw)
To: linux-perf-users, linux-kernel, bpf, acme, irogers
Cc: peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung,
Mike Leach
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>
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
+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
$(LIBBPF)-clean:
$(call QUIET_CLEAN, libbpf)
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1 2023-01-05 17:22 [PATCH] perf build: Fix build error when NO_LIBBPF=1 Mike Leach @ 2023-01-05 19:02 ` Ian Rogers 2023-01-05 23:40 ` Mike Leach 0 siblings, 1 reply; 6+ messages in thread From: Ian Rogers @ 2023-01-05 19:02 UTC (permalink / raw) To: Mike Leach Cc: linux-perf-users, linux-kernel, bpf, acme, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung 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. > 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? > +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. Thanks! Ian > $(LIBBPF)-clean: > $(call QUIET_CLEAN, libbpf) > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1 2023-01-05 19:02 ` Ian Rogers @ 2023-01-05 23:40 ` Mike Leach 2023-01-06 6:24 ` Ian Rogers 0 siblings, 1 reply; 6+ messages in thread From: Mike Leach @ 2023-01-05 23:40 UTC (permalink / raw) To: Ian Rogers Cc: linux-perf-users, linux-kernel, bpf, acme, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1 2023-01-05 23:40 ` Mike Leach @ 2023-01-06 6:24 ` Ian Rogers 2023-01-06 9:58 ` Mike Leach 0 siblings, 1 reply; 6+ messages in thread From: Ian Rogers @ 2023-01-06 6:24 UTC (permalink / raw) To: Mike Leach Cc: linux-perf-users, linux-kernel, bpf, acme, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung On Thu, Jan 5, 2023 at 3:40 PM Mike Leach <mike.leach@linaro.org> wrote: > > 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 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@google.com/ 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1 2023-01-06 6:24 ` Ian Rogers @ 2023-01-06 9:58 ` Mike Leach 2023-01-06 14:36 ` Ian Rogers 0 siblings, 1 reply; 6+ messages in thread From: Mike Leach @ 2023-01-06 9:58 UTC (permalink / raw) To: Ian Rogers Cc: linux-perf-users, linux-kernel, bpf, acme, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung Hi Ian, On Fri, 6 Jan 2023 at 06:24, Ian Rogers <irogers@google.com> wrote: > > On Thu, Jan 5, 2023 at 3:40 PM Mike Leach <mike.leach@linaro.org> wrote: > > > > 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 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@google.com/ > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] perf build: Fix build error when NO_LIBBPF=1 2023-01-06 9:58 ` Mike Leach @ 2023-01-06 14:36 ` Ian Rogers 0 siblings, 0 replies; 6+ messages in thread From: Ian Rogers @ 2023-01-06 14:36 UTC (permalink / raw) To: Mike Leach Cc: linux-perf-users, linux-kernel, bpf, acme, peterz, mingo, mark.rutland, alexander.shishkin, jolsa, namhyung On Fri, Jan 6, 2023 at 1:58 AM Mike Leach <mike.leach@linaro.org> wrote: > > Hi Ian, > > On Fri, 6 Jan 2023 at 06:24, Ian Rogers <irogers@google.com> wrote: > > > > On Thu, Jan 5, 2023 at 3:40 PM Mike Leach <mike.leach@linaro.org> wrote: > > > > > > 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 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@google.com/ > > > > 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@google.com/ > > > > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-06 14:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-05 17:22 [PATCH] perf build: Fix build error when NO_LIBBPF=1 Mike Leach 2023-01-05 19:02 ` Ian Rogers 2023-01-05 23:40 ` Mike Leach 2023-01-06 6:24 ` Ian Rogers 2023-01-06 9:58 ` Mike Leach 2023-01-06 14:36 ` Ian Rogers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).