* [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf
@ 2025-04-17 2:18 arnaldo.melo
2025-04-17 15:49 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: arnaldo.melo @ 2025-04-17 2:18 UTC (permalink / raw)
To: Namhyung Kim
Cc: Adrian Hunter, Ian Rogers, James Clark, Jiri Olsa, Kan Liang,
Quentin Monnet, Linux Kernel Mailing List, linux-perf-users
The tools/build/feature/test-all.c file tries to build with the most
common set of libraries expected to be present to build perf, and these
days libbfd (binutils) isn't one since it was made opt-in via
BUILD_NONDISTRO=1 on the make command line as it has license issues.
Fix this by removing the tests from there.
Fixes: dd317df072071903 ("perf build: Make binutil libraries opt in")
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Quentin Monnet <qmo@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/build/Makefile.feature | 12 ------------
tools/build/feature/Makefile | 2 +-
tools/build/feature/test-all.c | 19 -------------------
tools/perf/Makefile.config | 1 +
4 files changed, 2 insertions(+), 32 deletions(-)
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 57bd995ce6afa318..da025a8040a9a154 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -42,17 +42,12 @@ endef
#
# All the others should have lines in tools/build/feature/test-all.c like:
#
-# #define main main_test_disassembler_init_styled
-# # include "test-disassembler-init-styled.c"
-# #undef main
-#
# #define main main_test_libzstd
# # include "test-libzstd.c"
# #undef main
#
# int main(int argc, char *argv[])
# {
-# main_test_disassembler_four_args();
# main_test_libzstd();
# return 0;
# }
@@ -60,7 +55,6 @@ endef
# If the sample above works, then we end up with these lines in the FEATURE-DUMP
# file:
#
-# feature-disassembler-four-args=1
# feature-libzstd=1
#
FEATURE_TESTS_BASIC := \
@@ -71,8 +65,6 @@ FEATURE_TESTS_BASIC := \
get_current_dir_name \
gettid \
glibc \
- libbfd \
- libbfd-buildid \
libelf \
libelf-getphdrnum \
libelf-gelf_getnote \
@@ -102,8 +94,6 @@ FEATURE_TESTS_BASIC := \
setns \
libaio \
libzstd \
- disassembler-four-args \
- disassembler-init-styled \
file-handle
# FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
@@ -119,8 +109,6 @@ FEATURE_TESTS_EXTRA := \
hello \
libbabeltrace \
libcapstone \
- libbfd-liberty \
- libbfd-liberty-z \
libopencsd \
cxx \
llvm \
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index b8b5fb183dd40693..76724931f68e1b92 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -110,7 +110,7 @@ all: $(FILES)
__BUILD = $(CC) $(CFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.c,$(@F)) $(LDFLAGS)
BUILD = $(__BUILD) > $(@:.bin=.make.output) 2>&1
BUILD_BFD = $(BUILD) -DPACKAGE='"perf"' -lbfd -ldl
- BUILD_ALL = $(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -lslang $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -DPACKAGE='"perf"' -lbfd -ldl -lz -llzma -lzstd
+ BUILD_ALL = $(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -lslang $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -lz -llzma -lzstd
__BUILDXX = $(CXX) $(CXXFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.cpp,$(@F)) $(LDFLAGS)
BUILDXX = $(__BUILDXX) > $(@:.bin=.make.output) 2>&1
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 03ddaac6f4c4dfa2..1010f233d9c1ad49 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -66,14 +66,6 @@
# include "test-libslang.c"
#undef main
-#define main main_test_libbfd
-# include "test-libbfd.c"
-#undef main
-
-#define main main_test_libbfd_buildid
-# include "test-libbfd-buildid.c"
-#undef main
-
#define main main_test_backtrace
# include "test-backtrace.c"
#undef main
@@ -158,14 +150,6 @@
# include "test-reallocarray.c"
#undef main
-#define main main_test_disassembler_four_args
-# include "test-disassembler-four-args.c"
-#undef main
-
-#define main main_test_disassembler_init_styled
-# include "test-disassembler-init-styled.c"
-#undef main
-
#define main main_test_libzstd
# include "test-libzstd.c"
#undef main
@@ -193,8 +177,6 @@ int main(int argc, char *argv[])
main_test_libelf_gelf_getnote();
main_test_libelf_getshdrstrndx();
main_test_libslang();
- main_test_libbfd();
- main_test_libbfd_buildid();
main_test_backtrace();
main_test_libnuma();
main_test_numa_num_possible_cpus();
@@ -213,7 +195,6 @@ int main(int argc, char *argv[])
main_test_setns();
main_test_libaio();
main_test_reallocarray();
- main_test_disassembler_four_args();
main_test_libzstd();
main_test_libtraceevent();
main_test_libtracefs();
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index 9f08a6e96b351707..7e9aa3d910c2cdcc 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -917,6 +917,7 @@ ifneq ($(NO_JEVENTS),1)
endif
ifdef BUILD_NONDISTRO
+ $(call feature_check,libbfd)
ifeq ($(feature-libbfd), 1)
EXTLIBS += -lbfd -lopcodes
else
--
2.49.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf
2025-04-17 2:18 [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf arnaldo.melo
@ 2025-04-17 15:49 ` Ian Rogers
2025-04-17 21:58 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2025-04-17 15:49 UTC (permalink / raw)
To: arnaldo.melo
Cc: Namhyung Kim, Adrian Hunter, James Clark, Jiri Olsa, Kan Liang,
Quentin Monnet, Linux Kernel Mailing List, linux-perf-users
On Wed, Apr 16, 2025 at 7:18 PM <arnaldo.melo@gmail.com> wrote:
>
> The tools/build/feature/test-all.c file tries to build with the most
> common set of libraries expected to be present to build perf, and these
> days libbfd (binutils) isn't one since it was made opt-in via
> BUILD_NONDISTRO=1 on the make command line as it has license issues.
>
> Fix this by removing the tests from there.
>
> Fixes: dd317df072071903 ("perf build: Make binutil libraries opt in")
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Ian Rogers <irogers@google.com>
> Cc: James Clark <james.clark@linaro.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Kan Liang <kan.liang@linux.intel.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Quentin Monnet <qmo@kernel.org>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Reviewed-by: Ian Rogers <irogers@google.com>
There is a wider set of cleanups that remove BUILD_NONDISTRO and
libbfd that I posted back in January:
https://lore.kernel.org/lkml/20250122174308.350350-1-irogers@google.com/
These changes are carried in:
https://github.com/googleprodkernel/linux-perf/commits/google_tools_master/
The remaining use of libbfd for BPF JIT code disassembly is converted
to using either capstone or LLVM:
https://lore.kernel.org/lkml/20250122174308.350350-11-irogers@google.com/
Namhyung had concerns over code like:
https://github.com/googleprodkernel/linux-perf/blob/google_tools_master/tools/perf/util/capstone.c#L23-L132
where structs and enums derived from pahole are declared rather than
gathered from a #include. Doing things this way was deliberate in the
patch series so that the code could assume capstone or llvm support
may be present, falling back when not, and that the build wouldn't
need to have support for a no header file option that could do nothing
(this option would always be to try to fallback on dlopen and nobody
could create a less enabled build by forgetting to have a header
file). Theoretically the structs and defines, incorporated by way of
pahole, could change and a header file dependency would be robust to
this. In practice this would be an ABI incompatible change just as
changing a function name looked up by dlsym would be. Namhyung took
onboard my suggestion that we could reduce the set of structs/enums/..
for capstone by disabling the `print_insn_x86` when using dlopen, but
I think such a change should warn the user of reduced functionality,
cleaning up the warning would just bring back the code as I had
proposed:
https://lore.kernel.org/lkml/CAP-5=fXL0hXFT+t6gHp2RFd4dKnebSkd+rewudpmdentKGPURw@mail.gmail.com/
I think the patch series should be a priority to land as:
1) there is substantial cleanup wrt libbfd, libiberty, .. with
dependencies being factored out into their our C files;
2) the dependencies on libcapstone and libllvm are broken at build
time, allowing distributions to ship perf with a more minimal set of
dependencies and then later get the faster code or better support by
installing the libraries - I think ideally we'd do the same for the
libpython dependency as Namhyung has done in his uftrace;
3) the series adds BPF JIT disassembly.
Maybe this can be an occasion we respect the opinions of the patch
author and land what may be just a good patch series, but not quite
perfect to someone else's definition of perfect. We can always put
patches on top to make things perfect and discuss the merits at that
moment.
Thanks,
Ian
> ---
> tools/build/Makefile.feature | 12 ------------
> tools/build/feature/Makefile | 2 +-
> tools/build/feature/test-all.c | 19 -------------------
> tools/perf/Makefile.config | 1 +
> 4 files changed, 2 insertions(+), 32 deletions(-)
>
> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> index 57bd995ce6afa318..da025a8040a9a154 100644
> --- a/tools/build/Makefile.feature
> +++ b/tools/build/Makefile.feature
> @@ -42,17 +42,12 @@ endef
> #
> # All the others should have lines in tools/build/feature/test-all.c like:
> #
> -# #define main main_test_disassembler_init_styled
> -# # include "test-disassembler-init-styled.c"
> -# #undef main
> -#
> # #define main main_test_libzstd
> # # include "test-libzstd.c"
> # #undef main
> #
> # int main(int argc, char *argv[])
> # {
> -# main_test_disassembler_four_args();
> # main_test_libzstd();
> # return 0;
> # }
> @@ -60,7 +55,6 @@ endef
> # If the sample above works, then we end up with these lines in the FEATURE-DUMP
> # file:
> #
> -# feature-disassembler-four-args=1
> # feature-libzstd=1
> #
> FEATURE_TESTS_BASIC := \
> @@ -71,8 +65,6 @@ FEATURE_TESTS_BASIC := \
> get_current_dir_name \
> gettid \
> glibc \
> - libbfd \
> - libbfd-buildid \
> libelf \
> libelf-getphdrnum \
> libelf-gelf_getnote \
> @@ -102,8 +94,6 @@ FEATURE_TESTS_BASIC := \
> setns \
> libaio \
> libzstd \
> - disassembler-four-args \
> - disassembler-init-styled \
> file-handle
>
> # FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
> @@ -119,8 +109,6 @@ FEATURE_TESTS_EXTRA := \
> hello \
> libbabeltrace \
> libcapstone \
> - libbfd-liberty \
> - libbfd-liberty-z \
> libopencsd \
> cxx \
> llvm \
> diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
> index b8b5fb183dd40693..76724931f68e1b92 100644
> --- a/tools/build/feature/Makefile
> +++ b/tools/build/feature/Makefile
> @@ -110,7 +110,7 @@ all: $(FILES)
> __BUILD = $(CC) $(CFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.c,$(@F)) $(LDFLAGS)
> BUILD = $(__BUILD) > $(@:.bin=.make.output) 2>&1
> BUILD_BFD = $(BUILD) -DPACKAGE='"perf"' -lbfd -ldl
> - BUILD_ALL = $(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -lslang $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -DPACKAGE='"perf"' -lbfd -ldl -lz -llzma -lzstd
> + BUILD_ALL = $(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -lslang $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -lz -llzma -lzstd
>
> __BUILDXX = $(CXX) $(CXXFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.cpp,$(@F)) $(LDFLAGS)
> BUILDXX = $(__BUILDXX) > $(@:.bin=.make.output) 2>&1
> diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
> index 03ddaac6f4c4dfa2..1010f233d9c1ad49 100644
> --- a/tools/build/feature/test-all.c
> +++ b/tools/build/feature/test-all.c
> @@ -66,14 +66,6 @@
> # include "test-libslang.c"
> #undef main
>
> -#define main main_test_libbfd
> -# include "test-libbfd.c"
> -#undef main
> -
> -#define main main_test_libbfd_buildid
> -# include "test-libbfd-buildid.c"
> -#undef main
> -
> #define main main_test_backtrace
> # include "test-backtrace.c"
> #undef main
> @@ -158,14 +150,6 @@
> # include "test-reallocarray.c"
> #undef main
>
> -#define main main_test_disassembler_four_args
> -# include "test-disassembler-four-args.c"
> -#undef main
> -
> -#define main main_test_disassembler_init_styled
> -# include "test-disassembler-init-styled.c"
> -#undef main
> -
> #define main main_test_libzstd
> # include "test-libzstd.c"
> #undef main
> @@ -193,8 +177,6 @@ int main(int argc, char *argv[])
> main_test_libelf_gelf_getnote();
> main_test_libelf_getshdrstrndx();
> main_test_libslang();
> - main_test_libbfd();
> - main_test_libbfd_buildid();
> main_test_backtrace();
> main_test_libnuma();
> main_test_numa_num_possible_cpus();
> @@ -213,7 +195,6 @@ int main(int argc, char *argv[])
> main_test_setns();
> main_test_libaio();
> main_test_reallocarray();
> - main_test_disassembler_four_args();
> main_test_libzstd();
> main_test_libtraceevent();
> main_test_libtracefs();
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index 9f08a6e96b351707..7e9aa3d910c2cdcc 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -917,6 +917,7 @@ ifneq ($(NO_JEVENTS),1)
> endif
>
> ifdef BUILD_NONDISTRO
> + $(call feature_check,libbfd)
> ifeq ($(feature-libbfd), 1)
> EXTLIBS += -lbfd -lopcodes
> else
> --
> 2.49.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf
2025-04-17 15:49 ` Ian Rogers
@ 2025-04-17 21:58 ` Arnaldo Carvalho de Melo
2025-04-17 23:12 ` Ian Rogers
0 siblings, 1 reply; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-04-17 21:58 UTC (permalink / raw)
To: Ian Rogers
Cc: arnaldo.melo, Namhyung Kim, Adrian Hunter, James Clark, Jiri Olsa,
Kan Liang, Quentin Monnet, Linux Kernel Mailing List,
linux-perf-users
On Thu, Apr 17, 2025 at 08:49:35AM -0700, Ian Rogers wrote:
> On Wed, Apr 16, 2025 at 7:18 PM <arnaldo.melo@gmail.com> wrote:
> >
> > The tools/build/feature/test-all.c file tries to build with the most
> > common set of libraries expected to be present to build perf, and these
> > days libbfd (binutils) isn't one since it was made opt-in via
> > BUILD_NONDISTRO=1 on the make command line as it has license issues.
> >
> > Fix this by removing the tests from there.
> >
> > Fixes: dd317df072071903 ("perf build: Make binutil libraries opt in")
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Ian Rogers <irogers@google.com>
> > Cc: James Clark <james.clark@linaro.org>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > Cc: Kan Liang <kan.liang@linux.intel.com>
> > Cc: Namhyung Kim <namhyung@kernel.org>
> > Cc: Quentin Monnet <qmo@kernel.org>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Reviewed-by: Ian Rogers <irogers@google.com>
Thanks.
> There is a wider set of cleanups that remove BUILD_NONDISTRO and
> libbfd that I posted back in January:
> https://lore.kernel.org/lkml/20250122174308.350350-1-irogers@google.com/
I thought that discussion hadn't come to a conclusion, then I was in
vacation in January + LSFMM/BPF, lost track of it, will read your
message below and look at it again.
Its just that I recreated my toolbox container for building perf to a
fedora:42 based one and went on trying to build it from the base system
that gets installed in such containers till the point where I found the
above problem.
Now doing 'make -C tools/perf build-test' I'm getting this after the
above patch:
make_nondistro_O: cd . && make BUILD_NONDISTRO=1 FEATURES_DUMP=/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.NssFD0ssxu DESTDIR=/tmp/tmp.oEuROiOLtI
cd . && make BUILD_NONDISTRO=1 FEATURES_DUMP=/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.NssFD0ssxu DESTDIR=/tmp/tmp.oEuROiOLtI
BUILD: Doing 'make -j32' parallel build
Warning: Kernel ABI header differences:
diff -u tools/include/uapi/linux/bits.h include/uapi/linux/bits.h
diff -u tools/include/linux/bits.h include/linux/bits.h
diff -u tools/include/vdso/unaligned.h include/vdso/unaligned.h
diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h
Makefile.config:952: Old version of libbfd/binutils things like PE executable profiling will not be available
Makefile.config:968: No libllvm 13+ found, slower source file resolution, please install llvm-devel/llvm-dev
Makefile.config:1147: No openjdk development package found, please install JDK package, e.g. openjdk-8-jdk, java-1.8.0-openjdk-devel
GEN /tmp/tmp.NssFD0ssxu/common-cmds.h
<SNIP>
In file included from util/disasm_bpf.c:18:
/home/acme/git/perf-tools-next/tools/include/tools/dis-asm-compat.h:10:6: error: redeclaration of ‘enum disassembler_style’
10 | enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
| ^~~~~~~~~~~~~~~~~~
In file included from util/disasm_bpf.c:15:
/usr/include/dis-asm.h:53:6: note: originally defined here
53 | enum disassembler_style
| ^~~~~~~~~~~~~~~~~~
/home/acme/git/perf-tools-next/tools/include/tools/dis-asm-compat.h: In function ‘init_disassemble_info_compat’:
/home/acme/git/perf-tools-next/tools/include/tools/dis-asm-compat.h:50:9: error: too few arguments to function ‘init_disassemble_info’; expected 4, have 3
50 | init_disassemble_info(info, stream,
| ^~~~~~~~~~~~~~~~~~~~~
/usr/include/dis-asm.h:482:13: note: declared here
482 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
| ^~~~~~~~~~~~~~~~~~~~~
util/disasm_bpf.c: In function ‘symbol__disassemble_bpf’:
util/disasm_bpf.c:109:36: error: incompatible type for argument 1 of ‘disassembler’
109 | disassemble = disassembler(bfdf);
| ^~~~
| |
| bfd *
/usr/include/dis-asm.h:411:63: note: expected ‘enum bfd_architecture’ but argument is of type ‘bfd *’
411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
| ~~~~~~~~~~~~~~~~~~~~~~^~~
util/disasm_bpf.c:109:23: error: too few arguments to function ‘disassembler’; expected 4, have 1
109 | disassemble = disassembler(bfdf);
| ^~~~~~~~~~~~
/usr/include/dis-asm.h:411:27: note: declared here
411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
| ^~~~~~~~~~~~
make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:86: /tmp/tmp.NssFD0ssxu/util/disasm_bpf.o] Error 1
make[6]: *** Waiting for unfinished jobs....
CC /tmp/tmp.NssFD0ssxu/bench/mem-memcpy-x86-64-asm.o
<SNIP>
make[5]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
make[4]: *** [Makefile.perf:798: /tmp/tmp.NssFD0ssxu/perf-util-in.o] Error 2
make[4]: *** Waiting for unfinished jobs....
LD /tmp/tmp.NssFD0ssxu/tests/workloads/perf-test-in.o
LD /tmp/tmp.NssFD0ssxu/tests/perf-test-in.o
LD /tmp/tmp.NssFD0ssxu/perf-test-in.o
LD /tmp/tmp.NssFD0ssxu/perf-in.o
CC /tmp/tmp.NssFD0ssxu/pmu-events/pmu-events.o
LD /tmp/tmp.NssFD0ssxu/pmu-events/pmu-events-in.o
make[3]: *** [Makefile.perf:290: sub-make] Error 2
make[2]: *** [Makefile:76: all] Error 2
make[1]: *** [tests/make:341: make_nondistro_O] Error 1
make: *** [Makefile:109: build-test] Error 2
make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
- Arnaldo
> These changes are carried in:
> https://github.com/googleprodkernel/linux-perf/commits/google_tools_master/
> The remaining use of libbfd for BPF JIT code disassembly is converted
> to using either capstone or LLVM:
> https://lore.kernel.org/lkml/20250122174308.350350-11-irogers@google.com/
> Namhyung had concerns over code like:
> https://github.com/googleprodkernel/linux-perf/blob/google_tools_master/tools/perf/util/capstone.c#L23-L132
> where structs and enums derived from pahole are declared rather than
> gathered from a #include. Doing things this way was deliberate in the
> patch series so that the code could assume capstone or llvm support
> may be present, falling back when not, and that the build wouldn't
> need to have support for a no header file option that could do nothing
> (this option would always be to try to fallback on dlopen and nobody
> could create a less enabled build by forgetting to have a header
> file). Theoretically the structs and defines, incorporated by way of
> pahole, could change and a header file dependency would be robust to
> this. In practice this would be an ABI incompatible change just as
> changing a function name looked up by dlsym would be. Namhyung took
> onboard my suggestion that we could reduce the set of structs/enums/..
> for capstone by disabling the `print_insn_x86` when using dlopen, but
> I think such a change should warn the user of reduced functionality,
> cleaning up the warning would just bring back the code as I had
> proposed:
> https://lore.kernel.org/lkml/CAP-5=fXL0hXFT+t6gHp2RFd4dKnebSkd+rewudpmdentKGPURw@mail.gmail.com/
>
> I think the patch series should be a priority to land as:
> 1) there is substantial cleanup wrt libbfd, libiberty, .. with
> dependencies being factored out into their our C files;
> 2) the dependencies on libcapstone and libllvm are broken at build
> time, allowing distributions to ship perf with a more minimal set of
> dependencies and then later get the faster code or better support by
> installing the libraries - I think ideally we'd do the same for the
> libpython dependency as Namhyung has done in his uftrace;
> 3) the series adds BPF JIT disassembly.
>
> Maybe this can be an occasion we respect the opinions of the patch
> author and land what may be just a good patch series, but not quite
> perfect to someone else's definition of perfect. We can always put
> patches on top to make things perfect and discuss the merits at that
> moment.
>
> Thanks,
> Ian
>
> > ---
> > tools/build/Makefile.feature | 12 ------------
> > tools/build/feature/Makefile | 2 +-
> > tools/build/feature/test-all.c | 19 -------------------
> > tools/perf/Makefile.config | 1 +
> > 4 files changed, 2 insertions(+), 32 deletions(-)
> >
> > diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> > index 57bd995ce6afa318..da025a8040a9a154 100644
> > --- a/tools/build/Makefile.feature
> > +++ b/tools/build/Makefile.feature
> > @@ -42,17 +42,12 @@ endef
> > #
> > # All the others should have lines in tools/build/feature/test-all.c like:
> > #
> > -# #define main main_test_disassembler_init_styled
> > -# # include "test-disassembler-init-styled.c"
> > -# #undef main
> > -#
> > # #define main main_test_libzstd
> > # # include "test-libzstd.c"
> > # #undef main
> > #
> > # int main(int argc, char *argv[])
> > # {
> > -# main_test_disassembler_four_args();
> > # main_test_libzstd();
> > # return 0;
> > # }
> > @@ -60,7 +55,6 @@ endef
> > # If the sample above works, then we end up with these lines in the FEATURE-DUMP
> > # file:
> > #
> > -# feature-disassembler-four-args=1
> > # feature-libzstd=1
> > #
> > FEATURE_TESTS_BASIC := \
> > @@ -71,8 +65,6 @@ FEATURE_TESTS_BASIC := \
> > get_current_dir_name \
> > gettid \
> > glibc \
> > - libbfd \
> > - libbfd-buildid \
> > libelf \
> > libelf-getphdrnum \
> > libelf-gelf_getnote \
> > @@ -102,8 +94,6 @@ FEATURE_TESTS_BASIC := \
> > setns \
> > libaio \
> > libzstd \
> > - disassembler-four-args \
> > - disassembler-init-styled \
> > file-handle
> >
> > # FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
> > @@ -119,8 +109,6 @@ FEATURE_TESTS_EXTRA := \
> > hello \
> > libbabeltrace \
> > libcapstone \
> > - libbfd-liberty \
> > - libbfd-liberty-z \
> > libopencsd \
> > cxx \
> > llvm \
> > diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
> > index b8b5fb183dd40693..76724931f68e1b92 100644
> > --- a/tools/build/feature/Makefile
> > +++ b/tools/build/feature/Makefile
> > @@ -110,7 +110,7 @@ all: $(FILES)
> > __BUILD = $(CC) $(CFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.c,$(@F)) $(LDFLAGS)
> > BUILD = $(__BUILD) > $(@:.bin=.make.output) 2>&1
> > BUILD_BFD = $(BUILD) -DPACKAGE='"perf"' -lbfd -ldl
> > - BUILD_ALL = $(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -lslang $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -DPACKAGE='"perf"' -lbfd -ldl -lz -llzma -lzstd
> > + BUILD_ALL = $(BUILD) -fstack-protector-all -O2 -D_FORTIFY_SOURCE=2 -ldw -lelf -lnuma -lelf -lslang $(FLAGS_PERL_EMBED) $(FLAGS_PYTHON_EMBED) -lz -llzma -lzstd
> >
> > __BUILDXX = $(CXX) $(CXXFLAGS) -MD -Wall -Werror -o $@ $(patsubst %.bin,%.cpp,$(@F)) $(LDFLAGS)
> > BUILDXX = $(__BUILDXX) > $(@:.bin=.make.output) 2>&1
> > diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
> > index 03ddaac6f4c4dfa2..1010f233d9c1ad49 100644
> > --- a/tools/build/feature/test-all.c
> > +++ b/tools/build/feature/test-all.c
> > @@ -66,14 +66,6 @@
> > # include "test-libslang.c"
> > #undef main
> >
> > -#define main main_test_libbfd
> > -# include "test-libbfd.c"
> > -#undef main
> > -
> > -#define main main_test_libbfd_buildid
> > -# include "test-libbfd-buildid.c"
> > -#undef main
> > -
> > #define main main_test_backtrace
> > # include "test-backtrace.c"
> > #undef main
> > @@ -158,14 +150,6 @@
> > # include "test-reallocarray.c"
> > #undef main
> >
> > -#define main main_test_disassembler_four_args
> > -# include "test-disassembler-four-args.c"
> > -#undef main
> > -
> > -#define main main_test_disassembler_init_styled
> > -# include "test-disassembler-init-styled.c"
> > -#undef main
> > -
> > #define main main_test_libzstd
> > # include "test-libzstd.c"
> > #undef main
> > @@ -193,8 +177,6 @@ int main(int argc, char *argv[])
> > main_test_libelf_gelf_getnote();
> > main_test_libelf_getshdrstrndx();
> > main_test_libslang();
> > - main_test_libbfd();
> > - main_test_libbfd_buildid();
> > main_test_backtrace();
> > main_test_libnuma();
> > main_test_numa_num_possible_cpus();
> > @@ -213,7 +195,6 @@ int main(int argc, char *argv[])
> > main_test_setns();
> > main_test_libaio();
> > main_test_reallocarray();
> > - main_test_disassembler_four_args();
> > main_test_libzstd();
> > main_test_libtraceevent();
> > main_test_libtracefs();
> > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > index 9f08a6e96b351707..7e9aa3d910c2cdcc 100644
> > --- a/tools/perf/Makefile.config
> > +++ b/tools/perf/Makefile.config
> > @@ -917,6 +917,7 @@ ifneq ($(NO_JEVENTS),1)
> > endif
> >
> > ifdef BUILD_NONDISTRO
> > + $(call feature_check,libbfd)
> > ifeq ($(feature-libbfd), 1)
> > EXTLIBS += -lbfd -lopcodes
> > else
> > --
> > 2.49.0
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf
2025-04-17 21:58 ` Arnaldo Carvalho de Melo
@ 2025-04-17 23:12 ` Ian Rogers
2025-04-18 15:23 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 5+ messages in thread
From: Ian Rogers @ 2025-04-17 23:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: arnaldo.melo, Namhyung Kim, Adrian Hunter, James Clark, Jiri Olsa,
Kan Liang, Quentin Monnet, Linux Kernel Mailing List,
linux-perf-users
On Thu, Apr 17, 2025 at 2:58 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
> > There is a wider set of cleanups that remove BUILD_NONDISTRO and
> > libbfd that I posted back in January:
> > https://lore.kernel.org/lkml/20250122174308.350350-1-irogers@google.com/
>
> I thought that discussion hadn't come to a conclusion, then I was in
> vacation in January + LSFMM/BPF, lost track of it, will read your
> message below and look at it again.
>
> Its just that I recreated my toolbox container for building perf to a
> fedora:42 based one and went on trying to build it from the base system
> that gets installed in such containers till the point where I found the
> above problem.
>
> Now doing 'make -C tools/perf build-test' I'm getting this after the
> above patch:
>
> make_nondistro_O: cd . && make BUILD_NONDISTRO=1 FEATURES_DUMP=/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.NssFD0ssxu DESTDIR=/tmp/tmp.oEuROiOLtI
> cd . && make BUILD_NONDISTRO=1 FEATURES_DUMP=/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.NssFD0ssxu DESTDIR=/tmp/tmp.oEuROiOLtI
> BUILD: Doing 'make -j32' parallel build
> Warning: Kernel ABI header differences:
> diff -u tools/include/uapi/linux/bits.h include/uapi/linux/bits.h
> diff -u tools/include/linux/bits.h include/linux/bits.h
> diff -u tools/include/vdso/unaligned.h include/vdso/unaligned.h
> diff -u tools/arch/arm64/include/asm/cputype.h arch/arm64/include/asm/cputype.h
> Makefile.config:952: Old version of libbfd/binutils things like PE executable profiling will not be available
> Makefile.config:968: No libllvm 13+ found, slower source file resolution, please install llvm-devel/llvm-dev
> Makefile.config:1147: No openjdk development package found, please install JDK package, e.g. openjdk-8-jdk, java-1.8.0-openjdk-devel
>
> GEN /tmp/tmp.NssFD0ssxu/common-cmds.h
> <SNIP>
> In file included from util/disasm_bpf.c:18:
> /home/acme/git/perf-tools-next/tools/include/tools/dis-asm-compat.h:10:6: error: redeclaration of ‘enum disassembler_style’
> 10 | enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
> | ^~~~~~~~~~~~~~~~~~
> In file included from util/disasm_bpf.c:15:
> /usr/include/dis-asm.h:53:6: note: originally defined here
> 53 | enum disassembler_style
> | ^~~~~~~~~~~~~~~~~~
> /home/acme/git/perf-tools-next/tools/include/tools/dis-asm-compat.h: In function ‘init_disassemble_info_compat’:
> /home/acme/git/perf-tools-next/tools/include/tools/dis-asm-compat.h:50:9: error: too few arguments to function ‘init_disassemble_info’; expected 4, have 3
> 50 | init_disassemble_info(info, stream,
> | ^~~~~~~~~~~~~~~~~~~~~
> /usr/include/dis-asm.h:482:13: note: declared here
> 482 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
> | ^~~~~~~~~~~~~~~~~~~~~
> util/disasm_bpf.c: In function ‘symbol__disassemble_bpf’:
> util/disasm_bpf.c:109:36: error: incompatible type for argument 1 of ‘disassembler’
> 109 | disassemble = disassembler(bfdf);
> | ^~~~
> | |
> | bfd *
> /usr/include/dis-asm.h:411:63: note: expected ‘enum bfd_architecture’ but argument is of type ‘bfd *’
> 411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
> | ~~~~~~~~~~~~~~~~~~~~~~^~~
> util/disasm_bpf.c:109:23: error: too few arguments to function ‘disassembler’; expected 4, have 1
> 109 | disassemble = disassembler(bfdf);
> | ^~~~~~~~~~~~
> /usr/include/dis-asm.h:411:27: note: declared here
> 411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
> | ^~~~~~~~~~~~
> make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:86: /tmp/tmp.NssFD0ssxu/util/disasm_bpf.o] Error 1
> make[6]: *** Waiting for unfinished jobs....
> CC /tmp/tmp.NssFD0ssxu/bench/mem-memcpy-x86-64-asm.o
> <SNIP>
> make[5]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:142: util] Error 2
> make[4]: *** [Makefile.perf:798: /tmp/tmp.NssFD0ssxu/perf-util-in.o] Error 2
> make[4]: *** Waiting for unfinished jobs....
> LD /tmp/tmp.NssFD0ssxu/tests/workloads/perf-test-in.o
> LD /tmp/tmp.NssFD0ssxu/tests/perf-test-in.o
> LD /tmp/tmp.NssFD0ssxu/perf-test-in.o
> LD /tmp/tmp.NssFD0ssxu/perf-in.o
> CC /tmp/tmp.NssFD0ssxu/pmu-events/pmu-events.o
> LD /tmp/tmp.NssFD0ssxu/pmu-events/pmu-events-in.o
> make[3]: *** [Makefile.perf:290: sub-make] Error 2
> make[2]: *** [Makefile:76: all] Error 2
> make[1]: *** [tests/make:341: make_nondistro_O] Error 1
> make: *** [Makefile:109: build-test] Error 2
> make: Leaving directory '/home/acme/git/perf-tools-next/tools/perf'
Looked like a bad cherry pick, I sent rebased versions of the patches
and an extra small cleanup patch in the v4 patch series:
https://lore.kernel.org/lkml/20250417230740.86048-1-irogers@google.com/
Thanks,
Ian
> - Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf
2025-04-17 23:12 ` Ian Rogers
@ 2025-04-18 15:23 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 5+ messages in thread
From: Arnaldo Carvalho de Melo @ 2025-04-18 15:23 UTC (permalink / raw)
To: Ian Rogers
Cc: arnaldo.melo, Namhyung Kim, Adrian Hunter, James Clark, Jiri Olsa,
Kan Liang, Quentin Monnet, Linux Kernel Mailing List,
linux-perf-users
On Thu, Apr 17, 2025 at 04:12:16PM -0700, Ian Rogers wrote:
> On Thu, Apr 17, 2025 at 2:58 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > There is a wider set of cleanups that remove BUILD_NONDISTRO and
> > > libbfd that I posted back in January:
> > > https://lore.kernel.org/lkml/20250122174308.350350-1-irogers@google.com/
> > I thought that discussion hadn't come to a conclusion, then I was in
> > vacation in January + LSFMM/BPF, lost track of it, will read your
> > message below and look at it again.
> > Its just that I recreated my toolbox container for building perf to a
> > fedora:42 based one and went on trying to build it from the base system
> > that gets installed in such containers till the point where I found the
> > above problem.
> > Now doing 'make -C tools/perf build-test' I'm getting this after the
> > above patch:
> > make_nondistro_O: cd . && make BUILD_NONDISTRO=1 FEATURES_DUMP=/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.NssFD0ssxu DESTDIR=/tmp/tmp.oEuROiOLtI
> > cd . && make BUILD_NONDISTRO=1 FEATURES_DUMP=/home/acme/git/perf-tools-next/tools/perf/BUILD_TEST_FEATURE_DUMP -j32 O=/tmp/tmp.NssFD0ssxu DESTDIR=/tmp/tmp.oEuROiOLtI
> > BUILD: Doing 'make -j32' parallel build
<SNIP>
> > util/disasm_bpf.c:109:23: error: too few arguments to function ‘disassembler’; expected 4, have 1
> > 109 | disassemble = disassembler(bfdf);
> > | ^~~~~~~~~~~~
> > /usr/include/dis-asm.h:411:27: note: declared here
> > 411 | extern disassembler_ftype disassembler (enum bfd_architecture arc,
> > | ^~~~~~~~~~~~
> > make[6]: *** [/home/acme/git/perf-tools-next/tools/build/Makefile.build:86: /tmp/tmp.NssFD0ssxu/util/disasm_bpf.o] Error 1
> > make[6]: *** Waiting for unfinished jobs....
> > CC /tmp/tmp.NssFD0ssxu/bench/mem-memcpy-x86-64-asm.o
> > <SNIP>
<SNIP>
> Looked like a bad cherry pick, I sent rebased versions of the patches
I didn't cherry pick from your series, I mean on what is in
perf-tools-next/perf-tools-next
> and an extra small cleanup patch in the v4 patch series:
> https://lore.kernel.org/lkml/20250417230740.86048-1-irogers@google.com/
Thanks for resending it, I'll test with it.
- Arnaldo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-04-18 15:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-17 2:18 [PATCH 1/1] tools build: Remove libbfd from the set of expected libraries to build perf arnaldo.melo
2025-04-17 15:49 ` Ian Rogers
2025-04-17 21:58 ` Arnaldo Carvalho de Melo
2025-04-17 23:12 ` Ian Rogers
2025-04-18 15:23 ` Arnaldo Carvalho de Melo
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).