| Message ID | 20230417122943.2155502-2-anders.roxell@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [backport,1/2] tools perf: Fix compilation error with new binutils | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, Apr 17, 2023 at 5:30 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > From: Andres Freund <andres@anarazel.de> > > binutils changed the signature of init_disassemble_info(), which now causes > compilation failures for tools/perf/util/annotate.c, e.g. on debian > unstable. Thanks, I believe the compilation issue may well be resolved by: https://lore.kernel.org/lkml/20230311065753.3012826-8-irogers@google.com/ where binutils is made opt-in rather than opt-out. > Relevant binutils commit: > > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07 > > Wire up the feature test and switch to init_disassemble_info_compat(), > which were introduced in prior commits, fixing the compilation failure. I was kind of surprised to see no version check ifdef. Is init_disassemble_info_compat is supported in older binutils? Thanks, Ian > I verified that perf can still disassemble bpf programs by using bpftrace > under load, recording a perf trace, and then annotating the bpf "function" > with and without the changes. With old binutils there's no change in output > before/after this patch. When comparing the output from old binutils (2.35) > to new bintuils with the patch (upstream snapshot) there are a few output > differences, but they are unrelated to this patch. An example hunk is: > > 1.15 : 55:mov %rbp,%rdx > 0.00 : 58:add $0xfffffffffffffff8,%rdx > 0.00 : 5c:xor %ecx,%ecx > - 1.03 : 5e:callq 0xffffffffe12aca3c > + 1.03 : 5e:call 0xffffffffe12aca3c > 0.00 : 63:xor %eax,%eax > - 2.18 : 65:leaveq > - 2.82 : 66:retq > + 2.18 : 65:leave > + 2.82 : 66:ret > > Signed-off-by: Andres Freund <andres@anarazel.de> > Acked-by: Quentin Monnet <quentin@isovalent.com> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Ben Hutchings <benh@debian.org> > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: Sedat Dilek <sedat.dilek@gmail.com> > Cc: bpf@vger.kernel.org > Link: http://lore.kernel.org/lkml/20220622181918.ykrs5rsnmx3og4sv@alap3.anarazel.de > Link: https://lore.kernel.org/r/20220801013834.156015-5-andres@anarazel.de > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > --- > tools/perf/Makefile.config | 8 ++++++++ > tools/perf/util/annotate.c | 7 ++++--- > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config > index 3e7706c251e9..55905571f87b 100644 > --- a/tools/perf/Makefile.config > +++ b/tools/perf/Makefile.config > @@ -281,6 +281,7 @@ FEATURE_CHECK_LDFLAGS-libpython := $(PYTHON_EMBED_LDOPTS) > FEATURE_CHECK_LDFLAGS-libaio = -lrt > > FEATURE_CHECK_LDFLAGS-disassembler-four-args = -lbfd -lopcodes -ldl > +FEATURE_CHECK_LDFLAGS-disassembler-init-styled = -lbfd -lopcodes -ldl > > CORE_CFLAGS += -fno-omit-frame-pointer > CORE_CFLAGS += -ggdb3 > @@ -838,13 +839,16 @@ else > ifeq ($(feature-libbfd-liberty), 1) > EXTLIBS += -lbfd -lopcodes -liberty > FEATURE_CHECK_LDFLAGS-disassembler-four-args += -liberty -ldl > + FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -ldl > else > ifeq ($(feature-libbfd-liberty-z), 1) > EXTLIBS += -lbfd -lopcodes -liberty -lz > FEATURE_CHECK_LDFLAGS-disassembler-four-args += -liberty -lz -ldl > + FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -ldl > endif > endif > $(call feature_check,disassembler-four-args) > + $(call feature_check,disassembler-init-styled) > endif > > ifeq ($(feature-libbfd-buildid), 1) > @@ -957,6 +961,10 @@ ifeq ($(feature-disassembler-four-args), 1) > CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE > endif > > +ifeq ($(feature-disassembler-init-styled), 1) > + CFLAGS += -DDISASM_INIT_STYLED > +endif > + > ifeq (${IS_64_BIT}, 1) > ifndef NO_PERF_READ_VDSO32 > $(call feature_check,compile-32) > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index 308189454788..f2d1741b7610 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -1684,6 +1684,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil > #define PACKAGE "perf" > #include <bfd.h> > #include <dis-asm.h> > +#include <tools/dis-asm-compat.h> > > static int symbol__disassemble_bpf(struct symbol *sym, > struct annotate_args *args) > @@ -1726,9 +1727,9 @@ static int symbol__disassemble_bpf(struct symbol *sym, > ret = errno; > goto out; > } > - init_disassemble_info(&info, s, > - (fprintf_ftype) fprintf); > - > + init_disassemble_info_compat(&info, s, > + (fprintf_ftype) fprintf, > + fprintf_styled); > info.arch = bfd_get_arch(bfdf); > info.mach = bfd_get_mach(bfdf); > > -- > 2.39.2 >
2023-04-17 10:14 UTC-0700 ~ Ian Rogers <irogers@google.com> > On Mon, Apr 17, 2023 at 5:30 AM Anders Roxell <anders.roxell@linaro.org> wrote: >> >> From: Andres Freund <andres@anarazel.de> >> >> binutils changed the signature of init_disassemble_info(), which now causes >> compilation failures for tools/perf/util/annotate.c, e.g. on debian >> unstable. > > Thanks, I believe the compilation issue may well be resolved by: > https://lore.kernel.org/lkml/20230311065753.3012826-8-irogers@google.com/ > where binutils is made opt-in rather than opt-out. Hi, These commits are to make it possible to build against recent binutils, without having to leave it out at compile time, so as I understand they address a different issue? > >> Relevant binutils commit: >> >> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07 >> >> Wire up the feature test and switch to init_disassemble_info_compat(), >> which were introduced in prior commits, fixing the compilation failure. > > I was kind of surprised to see no version check ifdef. Is > init_disassemble_info_compat is supported in older binutils? It is not part of binutils, it was introduced in commit a45b3d692623 ("tools include: add dis-asm-compat.h to handle version differences"), which should likely be backported alongside these ones if it hasn't been already. Possibly the others from the same series [0], as well? I think all 5 patches from Andres' series were backported to 5.15 [1]. [0] https://lore.kernel.org/all/20220703212551.1114923-1-andres@anarazel.de/t/#m999a44663894e235b523ffc41ce87e956019ea72 [1] https://lore.kernel.org/all/e6e2df31-6327-f2ad-3049-0cbfa214ae5c@hauke-m.de/t/#u Best regards, Quentin
On Tue, Apr 18, 2023 at 9:43 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2023-04-17 10:14 UTC-0700 ~ Ian Rogers <irogers@google.com> > > On Mon, Apr 17, 2023 at 5:30 AM Anders Roxell <anders.roxell@linaro.org> wrote: > >> > >> From: Andres Freund <andres@anarazel.de> > >> > >> binutils changed the signature of init_disassemble_info(), which now causes > >> compilation failures for tools/perf/util/annotate.c, e.g. on debian > >> unstable. > > > > Thanks, I believe the compilation issue may well be resolved by: > > https://lore.kernel.org/lkml/20230311065753.3012826-8-irogers@google.com/ > > where binutils is made opt-in rather than opt-out. > > Hi, > These commits are to make it possible to build against recent binutils, > without having to leave it out at compile time, so as I understand they > address a different issue? Kind of. We don't want the Linux perf build to break. Previously if binutils were installed then Linux perf would default to linking with it and break your build were binutils to change its API. That is no longer the case as we don't default to linking against binutils. This was motivated by distributions not being able to link Linux perf with binutils due to the license incompatibilities. I don't see a problem supporting linking against newer and older binutils if people want to on non-distributed binaries. We'll probably need more build tests/containers to cover the possibilities of this. I'm not sure what's motivating binutils support other than personal experimentation though. Thanks, Ian > > > >> Relevant binutils commit: > >> > >> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07 > >> > >> Wire up the feature test and switch to init_disassemble_info_compat(), > >> which were introduced in prior commits, fixing the compilation failure. > > > > I was kind of surprised to see no version check ifdef. Is > > init_disassemble_info_compat is supported in older binutils? > > It is not part of binutils, it was introduced in commit a45b3d692623 > ("tools include: add dis-asm-compat.h to handle version differences"), > which should likely be backported alongside these ones if it hasn't been > already. Possibly the others from the same series [0], as well? > > I think all 5 patches from Andres' series were backported to 5.15 [1]. > > [0] > https://lore.kernel.org/all/20220703212551.1114923-1-andres@anarazel.de/t/#m999a44663894e235b523ffc41ce87e956019ea72 > [1] > https://lore.kernel.org/all/e6e2df31-6327-f2ad-3049-0cbfa214ae5c@hauke-m.de/t/#u > > Best regards, > Quentin
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index 3e7706c251e9..55905571f87b 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -281,6 +281,7 @@ FEATURE_CHECK_LDFLAGS-libpython := $(PYTHON_EMBED_LDOPTS) FEATURE_CHECK_LDFLAGS-libaio = -lrt FEATURE_CHECK_LDFLAGS-disassembler-four-args = -lbfd -lopcodes -ldl +FEATURE_CHECK_LDFLAGS-disassembler-init-styled = -lbfd -lopcodes -ldl CORE_CFLAGS += -fno-omit-frame-pointer CORE_CFLAGS += -ggdb3 @@ -838,13 +839,16 @@ else ifeq ($(feature-libbfd-liberty), 1) EXTLIBS += -lbfd -lopcodes -liberty FEATURE_CHECK_LDFLAGS-disassembler-four-args += -liberty -ldl + FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -ldl else ifeq ($(feature-libbfd-liberty-z), 1) EXTLIBS += -lbfd -lopcodes -liberty -lz FEATURE_CHECK_LDFLAGS-disassembler-four-args += -liberty -lz -ldl + FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -ldl endif endif $(call feature_check,disassembler-four-args) + $(call feature_check,disassembler-init-styled) endif ifeq ($(feature-libbfd-buildid), 1) @@ -957,6 +961,10 @@ ifeq ($(feature-disassembler-four-args), 1) CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE endif +ifeq ($(feature-disassembler-init-styled), 1) + CFLAGS += -DDISASM_INIT_STYLED +endif + ifeq (${IS_64_BIT}, 1) ifndef NO_PERF_READ_VDSO32 $(call feature_check,compile-32) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 308189454788..f2d1741b7610 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1684,6 +1684,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil #define PACKAGE "perf" #include <bfd.h> #include <dis-asm.h> +#include <tools/dis-asm-compat.h> static int symbol__disassemble_bpf(struct symbol *sym, struct annotate_args *args) @@ -1726,9 +1727,9 @@ static int symbol__disassemble_bpf(struct symbol *sym, ret = errno; goto out; } - init_disassemble_info(&info, s, - (fprintf_ftype) fprintf); - + init_disassemble_info_compat(&info, s, + (fprintf_ftype) fprintf, + fprintf_styled); info.arch = bfd_get_arch(bfdf); info.mach = bfd_get_mach(bfdf);