Hi,
binutils changed the signature of init_disassemble_info(), which now causes
perf and bpftool to fail to compile (e.g. on debian unstable).
Relevant binutils commit: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=60a3da00bd5407f07d64dff82a4dae98230dfaac
util/annotate.c: In function ‘symbol__disassemble_bpf’:
util/annotate.c:1765:9: error: too few arguments to function ‘init_disassemble_info’
1765 | init_disassemble_info(&info, s,
| ^~~~~~~~~~~~~~~~~~~~~
In file included from util/annotate.c:1718:
/usr/include/dis-asm.h:472:13: note: declared here
472 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
| ^~~~~~~~~~~~~~~~~~~~~
with equivalent failures in
tools/bpf/bpf_jit_disasm.c
tools/bpf/bpftool/jit_disasm.c
The fix is easy enough, add a wrapper around fprintf() that conforms to the
new signature.
However I assume the necessary feature test and wrapper should only be added
once? I don't know the kernel stuff well enough to choose the right structure
here.
Attached is my local fix for perf. Obviously would need work to be a real
solution.
Greetings,
Andres Freund
On Wed, 22 Jun 2022 at 19:19, Andres Freund <[email protected]> wrote:
>
> Hi,
>
> binutils changed the signature of init_disassemble_info(), which now causes
> perf and bpftool to fail to compile (e.g. on debian unstable).
>
> Relevant binutils commit: https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=60a3da00bd5407f07d64dff82a4dae98230dfaac
>
> util/annotate.c: In function ‘symbol__disassemble_bpf’:
> util/annotate.c:1765:9: error: too few arguments to function ‘init_disassemble_info’
> 1765 | init_disassemble_info(&info, s,
> | ^~~~~~~~~~~~~~~~~~~~~
> In file included from util/annotate.c:1718:
> /usr/include/dis-asm.h:472:13: note: declared here
> 472 | extern void init_disassemble_info (struct disassemble_info *dinfo, void *stream,
> | ^~~~~~~~~~~~~~~~~~~~~
>
> with equivalent failures in
>
> tools/bpf/bpf_jit_disasm.c
> tools/bpf/bpftool/jit_disasm.c
Hi Andres,
Too bad the libbfd API is changing again :/ But many thanks for
pinning down the relevant commit, and for the report!
> The fix is easy enough, add a wrapper around fprintf() that conforms to the
> new signature.
>
> However I assume the necessary feature test and wrapper should only be added
> once? I don't know the kernel stuff well enough to choose the right structure
> here.
We can probably find a common header for the wrapper under
tools/include/. One possibility could be a new header under
tools/include/tools/, like for libc_compat.h. Although personally I
don't mind too much about redefining the wrapper several times given
how short it is, and because maybe some tools could redefine it anyway
to use colour output in the future.
The feature test would better be shared, it would probably be similar
to what was done in the following commit to accommodate for a previous
change in libbfd:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fb982666e380c1632a74495b68b3c33a66e76430
> Attached is my local fix for perf. Obviously would need work to be a real
> solution.
Thanks a lot! Would you be willing to submit a patch for the feature
detection and wrapper?
Best regards,
Quentin
Hi,
On 2022-06-22 23:53:58 +0100, Quentin Monnet wrote:
> Too bad the libbfd API is changing again :/
Yea, not great. Particularly odd that
/* For compatibility with existing code. */
#define INIT_DISASSEMBLE_INFO(INFO, STREAM, FPRINTF_FUNC, FPRINTF_STYLED_FUNC) \
was changed. Leaving the "For compatibility with existing code." around,
despite obviously not providing compatibility...
CCed the author of that commit, maybe worth fixing?
Given that disassemble_set_printf() was added, it seems like it'd have been
easy to not change init_disassemble_info() / INIT_DISASSEMBLE_INFO() and
require disassemble_set_printf() to be called to get styled printf support.
> > The fix is easy enough, add a wrapper around fprintf() that conforms to the
> > new signature.
> >
> > However I assume the necessary feature test and wrapper should only be added
> > once? I don't know the kernel stuff well enough to choose the right structure
> > here.
>
> We can probably find a common header for the wrapper under
> tools/include/. One possibility could be a new header under
> tools/include/tools/, like for libc_compat.h. Although personally I
> don't mind too much about redefining the wrapper several times given
> how short it is, and because maybe some tools could redefine it anyway
> to use colour output in the future.
I'm more bothered by duplicating the necessary ifdefery than duplicating the
short fprintf wrapper...
> The feature test would better be shared, it would probably be similar
> to what was done in the following commit to accommodate for a previous
> change in libbfd:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fb982666e380c1632a74495b68b3c33a66e76430
Ah, beautiful hand-rolled feature tests :)
> > Attached is my local fix for perf. Obviously would need work to be a real
> > solution.
>
> Thanks a lot! Would you be willing to submit a patch for the feature
> detection and wrapper?
I'll give it a go, albeit probably not today.
Greetings,
Andres Freund
Andres Freund <[email protected]> writes:
> Hi,
>
> On 2022-06-22 23:53:58 +0100, Quentin Monnet wrote:
>> Too bad the libbfd API is changing again :/
>
> Yea, not great. Particularly odd that
> /* For compatibility with existing code. */
> #define INIT_DISASSEMBLE_INFO(INFO, STREAM, FPRINTF_FUNC, FPRINTF_STYLED_FUNC) \
>
> was changed. Leaving the "For compatibility with existing code." around,
> despite obviously not providing compatibility...
>
> CCed the author of that commit, maybe worth fixing?
Greetings!
First, massive appologies for breaking you existing code. I wasn't
aware that the libopcodes disassembler was used by anything out of
tree.
> Given that disassemble_set_printf() was added, it seems like it'd have been
> easy to not change init_disassemble_info() / INIT_DISASSEMBLE_INFO() and
> require disassemble_set_printf() to be called to get styled printf support.
When I did this work I desperately wanted to maintain the original API
as much as possible. But I couldn't figure out a way for this to work.
The problem is that we now have two classes of disassembler, those that
call the styled printf callback, and those that call the classic
non-styled printf.
When I originally did this work I did want to leave
INIT_DISASSEMBLE_INFO untouched, and provide a default styled printf
that would forward calls to the non-styled printf.
The problem I ran into is that the disassemble_info printf API is not
great. If the API had passed the disassemble_info* as one of the
arguments then this would have been trivial. But instead, we only get
passed a 'void *', which is one of the fields of disassemble_info.
As a result there's no easy way to forward calls from this imagined
default styled printf, to the actual, user supplied non-styled printf.
I did consider changing the 'void *' field from being the stream to
write to, to be the disassemble_info*, but then the non-styled printf
calls would need to be updated anyway, which would be a breaking change.
I also considered changing the 'void *' stream to be the
'disassemble_info*', then wrapping both styled and non-styled printf
calls. This would allow me to provide a default for the styled printf,
and we can pull the required information from the 'disassemble_info*',
but the problem I have now is that the wrapper functions would be a
vararg function, and the user supplied printf functions are also vararg
functions.... and I didn't know how to forward the args from my wrapper
to the user supplied functions without changing the API of the user
supplied functions to take a va_list ... which again is an API breaking
change.
I'm aware that non of the above helps you at all, other than to say, I
didn't make this change without a little thought. But, that doesn't
mean there isn't a better way this could have been done. So, if you
have any suggestions, then I'm happy to give them a go.
Once again, sorry for the additional work this has created for you, and
if I can help at all to put this right, then please, do let me know.
Oh, and you're absolutely correct about the comment. I should have just
deleted it. Or really, I should have just deleted the macro entirely I
guess and forced everyone to call init_disassemble_info directly. Bit
late for that now though!
Thanks,
Andrew
>> > The fix is easy enough, add a wrapper around fprintf() that conforms to the
>> > new signature.
>> >
>> > However I assume the necessary feature test and wrapper should only be added
>> > once? I don't know the kernel stuff well enough to choose the right structure
>> > here.
>>
>> We can probably find a common header for the wrapper under
>> tools/include/. One possibility could be a new header under
>> tools/include/tools/, like for libc_compat.h. Although personally I
>> don't mind too much about redefining the wrapper several times given
>> how short it is, and because maybe some tools could redefine it anyway
>> to use colour output in the future.
>
> I'm more bothered by duplicating the necessary ifdefery than duplicating the
> short fprintf wrapper...
>
>
>> The feature test would better be shared, it would probably be similar
>> to what was done in the following commit to accommodate for a previous
>> change in libbfd:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fb982666e380c1632a74495b68b3c33a66e76430
>
> Ah, beautiful hand-rolled feature tests :)
>
>
>> > Attached is my local fix for perf. Obviously would need work to be a real
>> > solution.
>>
>> Thanks a lot! Would you be willing to submit a patch for the feature
>> detection and wrapper?
>
> I'll give it a go, albeit probably not today.
>
> Greetings,
>
> Andres Freund
binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
I first fixed this without introducing the compat header, as suggested by
Quentin, but I thought the amount of repeated boilerplate was a bit too
much. So instead I introduced a compat header to wrap the API changes. Even
tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
looks nicer this way.
I'm not regular contributor, so it very well might be my procedures are a
bit off...
I wasn't sure whether the split of the commits conforms with the kernel
habits should the changes to tools/bpf and tools/perf be split into
separate commits?
Nor was I sure if I the right [number of] people to CC?
WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
in feature/Makefile and why -ldl isn't needed in the other places. But...
Andres Freund (3):
tools build: add feature test for init_disassemble_info API changes
tools: add dis-asm-compat.h to centralize handling of version
differences
tools: Use tools/dis-asm-compat.h to fix compilation errors with new
binutils
To: [email protected]
To: [email protected]
Cc: Quentin Monnet <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
tools/bpf/Makefile | 7 ++-
tools/bpf/bpf_jit_disasm.c | 5 +-
tools/bpf/bpftool/Makefile | 7 ++-
tools/bpf/bpftool/jit_disasm.c | 40 +++++++++++---
tools/build/Makefile.feature | 4 +-
tools/build/feature/Makefile | 4 ++
tools/build/feature/test-all.c | 4 ++
.../feature/test-disassembler-init-styled.c | 13 +++++
tools/include/tools/dis-asm-compat.h | 53 +++++++++++++++++++
tools/perf/Makefile.config | 8 +++
tools/perf/util/annotate.c | 7 +--
11 files changed, 135 insertions(+), 17 deletions(-)
create mode 100644 tools/build/feature/test-disassembler-init-styled.c
create mode 100644 tools/include/tools/dis-asm-compat.h
--
2.35.1.677.gabf474a5dd
binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
I first fixed this without introducing the compat header, as suggested by
Quentin, but I thought the amount of repeated boilerplate was a bit too
much. So instead I introduced a compat header to wrap the API changes. Even
tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
looks nicer this way.
I'm not regular contributor, so it very well might be my procedures are a
bit off...
I am not sure I added the right [number of] people to CC?
WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
in feature/Makefile and why -ldl isn't needed in the other places. But...
V2:
- split patches further, so that tools/bpf and tools/perf part are entirely
separate
- included a bit more information about tests I did in commit messages
- add a maybe_unused to fprintf_json_styled's style argument
Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
To: [email protected]
To: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]
Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
Andres Freund (5):
tools build: add feature test for init_disassemble_info API changes
tools include: add dis-asm-compat.h to handle version differences
tools perf: Fix compilation error with new binutils
tools bpf_jit_disasm: Fix compilation error with new binutils
tools bpftool: Fix compilation error with new binutils
tools/bpf/Makefile | 7 ++-
tools/bpf/bpf_jit_disasm.c | 5 +-
tools/bpf/bpftool/Makefile | 7 ++-
tools/bpf/bpftool/jit_disasm.c | 42 ++++++++++++---
tools/build/Makefile.feature | 4 +-
tools/build/feature/Makefile | 4 ++
tools/build/feature/test-all.c | 4 ++
.../feature/test-disassembler-init-styled.c | 13 +++++
tools/include/tools/dis-asm-compat.h | 53 +++++++++++++++++++
tools/perf/Makefile.config | 8 +++
tools/perf/util/annotate.c | 7 +--
11 files changed, 137 insertions(+), 17 deletions(-)
create mode 100644 tools/build/feature/test-disassembler-init-styled.c
create mode 100644 tools/include/tools/dis-asm-compat.h
--
2.37.0.3.g30cc8d0f14
On Sun, Jul 03, 2022 at 02:25:46PM -0700, Andres Freund wrote:
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
> I first fixed this without introducing the compat header, as suggested by
> Quentin, but I thought the amount of repeated boilerplate was a bit too
> much. So instead I introduced a compat header to wrap the API changes. Even
> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> looks nicer this way.
>
> I'm not regular contributor, so it very well might be my procedures are a
> bit off...
>
> I am not sure I added the right [number of] people to CC?
>
> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> in feature/Makefile and why -ldl isn't needed in the other places. But...
>
> V2:
> - split patches further, so that tools/bpf and tools/perf part are entirely
> separate
> - included a bit more information about tests I did in commit messages
> - add a maybe_unused to fprintf_json_styled's style argument
>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> To: [email protected]
> To: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]
> Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
>
> Andres Freund (5):
> tools build: add feature test for init_disassemble_info API changes
> tools include: add dis-asm-compat.h to handle version differences
> tools perf: Fix compilation error with new binutils
> tools bpf_jit_disasm: Fix compilation error with new binutils
> tools bpftool: Fix compilation error with new binutils
I think the disassembler checks should not be displayed by default,
with your change I can see all the time:
... disassembler-four-args: [ on ]
... disassembler-init-styled: [ OFF ]
could you please squash something like below in? moving disassembler
checks out of sight and do manual detection
thanks,
jirka
---
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 339686b99a6e..bce9a9b52b2c 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -69,8 +69,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
@@ -106,7 +104,9 @@ FEATURE_TESTS_EXTRA := \
libbpf-bpf_create_map \
libpfm4 \
libdebuginfod \
- clang-bpf-co-re
+ clang-bpf-co-re \
+ disassembler-four-args \
+ disassembler-init-styled
FEATURE_TESTS ?= $(FEATURE_TESTS_BASIC)
@@ -135,9 +135,7 @@ FEATURE_DISPLAY ?= \
get_cpuid \
bpf \
libaio \
- libzstd \
- disassembler-four-args \
- disassembler-init-styled
+ libzstd
# Set FEATURE_CHECK_(C|LD)FLAGS-all for all FEATURE_TESTS features.
# If in the future we need per-feature checks/flags for features not
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index ee417c321adb..2aa0bad11f05 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -914,8 +914,6 @@ ifndef NO_LIBBFD
FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -lz -ldl
endif
endif
- $(call feature_check,disassembler-four-args)
- $(call feature_check,disassembler-init-styled)
endif
ifeq ($(feature-libbfd-buildid), 1)
@@ -1025,6 +1023,9 @@ ifdef HAVE_KVM_STAT_SUPPORT
CFLAGS += -DHAVE_KVM_STAT_SUPPORT
endif
+$(call feature_check,disassembler-four-args)
+$(call feature_check,disassembler-init-styled)
+
ifeq ($(feature-disassembler-four-args), 1)
CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
endif
Hi,
On 2022-07-04 11:13:33 +0200, Jiri Olsa wrote:
> I think the disassembler checks should not be displayed by default,
> with your change I can see all the time:
>
> ... disassembler-four-args: [ on ]
> ... disassembler-init-styled: [ OFF ]
>
>
> could you please squash something like below in? moving disassembler
> checks out of sight and do manual detection
Makes sense - I was wondering why disassembler-four-args is displayed, but
though it better to mirror the existing behaviour. Does "hiding"
disassembler-four-args need to be its own set of commits?
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index ee417c321adb..2aa0bad11f05 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -914,8 +914,6 @@ ifndef NO_LIBBFD
> FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -lz -ldl
> endif
> endif
> - $(call feature_check,disassembler-four-args)
> - $(call feature_check,disassembler-init-styled)
> endif
>
> ifeq ($(feature-libbfd-buildid), 1)
> @@ -1025,6 +1023,9 @@ ifdef HAVE_KVM_STAT_SUPPORT
> CFLAGS += -DHAVE_KVM_STAT_SUPPORT
> endif
>
> +$(call feature_check,disassembler-four-args)
> +$(call feature_check,disassembler-init-styled)
> +
> ifeq ($(feature-disassembler-four-args), 1)
> CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
> endif
This I don't understand - why do we want these to run under NO_LIBBFD etc?
Greetings,
Andres Freund
On Mon, Jul 04, 2022 at 01:19:22PM -0700, Andres Freund wrote:
> Hi,
>
> On 2022-07-04 11:13:33 +0200, Jiri Olsa wrote:
> > I think the disassembler checks should not be displayed by default,
> > with your change I can see all the time:
> >
> > ... disassembler-four-args: [ on ]
> > ... disassembler-init-styled: [ OFF ]
> >
> >
> > could you please squash something like below in? moving disassembler
> > checks out of sight and do manual detection
>
> Makes sense - I was wondering why disassembler-four-args is displayed, but
> though it better to mirror the existing behaviour. Does "hiding"
> disassembler-four-args need to be its own set of commits?
I guess first hide the disassembler-four-args and add the new the same way
>
>
> > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > index ee417c321adb..2aa0bad11f05 100644
> > --- a/tools/perf/Makefile.config
> > +++ b/tools/perf/Makefile.config
> > @@ -914,8 +914,6 @@ ifndef NO_LIBBFD
> > FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -lz -ldl
> > endif
> > endif
> > - $(call feature_check,disassembler-four-args)
> > - $(call feature_check,disassembler-init-styled)
> > endif
> >
> > ifeq ($(feature-libbfd-buildid), 1)
> > @@ -1025,6 +1023,9 @@ ifdef HAVE_KVM_STAT_SUPPORT
> > CFLAGS += -DHAVE_KVM_STAT_SUPPORT
> > endif
> >
> > +$(call feature_check,disassembler-four-args)
> > +$(call feature_check,disassembler-init-styled)
> > +
> > ifeq ($(feature-disassembler-four-args), 1)
> > CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
> > endif
>
> This I don't understand - why do we want these to run under NO_LIBBFD etc?
when I was quickly testing that I did not have any of them detected
and got compile fail.. so I moved it to safe place ;-) it might be
placed in smarter place
thanks,
jirka
>
> Greetings,
>
> Andres Freund
On Sun, Jul 3, 2022 at 11:25 PM Andres Freund <[email protected]> wrote:
>
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
HI,
what was the base for this patchset?
I tried with Linux v5.19-rc5 and it doesn not apply cleanly.
Regards,
-Sedat-
> I first fixed this without introducing the compat header, as suggested by
> Quentin, but I thought the amount of repeated boilerplate was a bit too
> much. So instead I introduced a compat header to wrap the API changes. Even
> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> looks nicer this way.
>
> I'm not regular contributor, so it very well might be my procedures are a
> bit off...
>
> I am not sure I added the right [number of] people to CC?
>
> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> in feature/Makefile and why -ldl isn't needed in the other places. But...
>
> V2:
> - split patches further, so that tools/bpf and tools/perf part are entirely
> separate
> - included a bit more information about tests I did in commit messages
> - add a maybe_unused to fprintf_json_styled's style argument
>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> To: [email protected]
> To: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]
> Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
>
> Andres Freund (5):
> tools build: add feature test for init_disassemble_info API changes
> tools include: add dis-asm-compat.h to handle version differences
> tools perf: Fix compilation error with new binutils
> tools bpf_jit_disasm: Fix compilation error with new binutils
> tools bpftool: Fix compilation error with new binutils
>
> tools/bpf/Makefile | 7 ++-
> tools/bpf/bpf_jit_disasm.c | 5 +-
> tools/bpf/bpftool/Makefile | 7 ++-
> tools/bpf/bpftool/jit_disasm.c | 42 ++++++++++++---
> tools/build/Makefile.feature | 4 +-
> tools/build/feature/Makefile | 4 ++
> tools/build/feature/test-all.c | 4 ++
> .../feature/test-disassembler-init-styled.c | 13 +++++
> tools/include/tools/dis-asm-compat.h | 53 +++++++++++++++++++
> tools/perf/Makefile.config | 8 +++
> tools/perf/util/annotate.c | 7 +--
> 11 files changed, 137 insertions(+), 17 deletions(-)
> create mode 100644 tools/build/feature/test-disassembler-init-styled.c
> create mode 100644 tools/include/tools/dis-asm-compat.h
>
> --
> 2.37.0.3.g30cc8d0f14
>
On Sun, Jul 10, 2022 at 1:43 PM Sedat Dilek <[email protected]> wrote:
>
> On Sun, Jul 3, 2022 at 11:25 PM Andres Freund <[email protected]> wrote:
> >
> > binutils changed the signature of init_disassemble_info(), which now causes
> > compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> > binutils commit:
> > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
> >
>
> HI,
>
> what was the base for this patchset?
> I tried with Linux v5.19-rc5 and it doesn not apply cleanly.
>
More coffee.
$ egrep 'Auto-detecting|disassembler' make-log_perf-python3.10-install_bin.txt
15:Auto-detecting system features:
36:... disassembler-four-args: [ on ]
37:... disassembler-init-styled: [ on ]
Tested-by: Sedat Dilek <[email protected]> # LLVM-14 (x86-64)
-Sedat-
>
> > I first fixed this without introducing the compat header, as suggested by
> > Quentin, but I thought the amount of repeated boilerplate was a bit too
> > much. So instead I introduced a compat header to wrap the API changes. Even
> > tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> > looks nicer this way.
> >
> > I'm not regular contributor, so it very well might be my procedures are a
> > bit off...
> >
> > I am not sure I added the right [number of] people to CC?
> >
> > WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> > nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> > in feature/Makefile and why -ldl isn't needed in the other places. But...
> >
> > V2:
> > - split patches further, so that tools/bpf and tools/perf part are entirely
> > separate
> > - included a bit more information about tests I did in commit messages
> > - add a maybe_unused to fprintf_json_styled's style argument
> >
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Jiri Olsa <[email protected]>
> > Cc: Sedat Dilek <[email protected]>
> > Cc: Quentin Monnet <[email protected]>
> > To: [email protected]
> > To: [email protected]
> > Link: https://lore.kernel.org/lkml/[email protected]
> > Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
> >
> > Andres Freund (5):
> > tools build: add feature test for init_disassemble_info API changes
> > tools include: add dis-asm-compat.h to handle version differences
> > tools perf: Fix compilation error with new binutils
> > tools bpf_jit_disasm: Fix compilation error with new binutils
> > tools bpftool: Fix compilation error with new binutils
> >
> > tools/bpf/Makefile | 7 ++-
> > tools/bpf/bpf_jit_disasm.c | 5 +-
> > tools/bpf/bpftool/Makefile | 7 ++-
> > tools/bpf/bpftool/jit_disasm.c | 42 ++++++++++++---
> > tools/build/Makefile.feature | 4 +-
> > tools/build/feature/Makefile | 4 ++
> > tools/build/feature/test-all.c | 4 ++
> > .../feature/test-disassembler-init-styled.c | 13 +++++
> > tools/include/tools/dis-asm-compat.h | 53 +++++++++++++++++++
> > tools/perf/Makefile.config | 8 +++
> > tools/perf/util/annotate.c | 7 +--
> > 11 files changed, 137 insertions(+), 17 deletions(-)
> > create mode 100644 tools/build/feature/test-disassembler-init-styled.c
> > create mode 100644 tools/include/tools/dis-asm-compat.h
> >
> > --
> > 2.37.0.3.g30cc8d0f14
> >
On Sun, Jul 3, 2022 at 11:25 PM Andres Freund <[email protected]> wrote:
>
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
> I first fixed this without introducing the compat header, as suggested by
> Quentin, but I thought the amount of repeated boilerplate was a bit too
> much. So instead I introduced a compat header to wrap the API changes. Even
> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> looks nicer this way.
>
> I'm not regular contributor, so it very well might be my procedures are a
> bit off...
>
> I am not sure I added the right [number of] people to CC?
>
> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> in feature/Makefile and why -ldl isn't needed in the other places. But...
>
> V2:
> - split patches further, so that tools/bpf and tools/perf part are entirely
> separate
> - included a bit more information about tests I did in commit messages
> - add a maybe_unused to fprintf_json_styled's style argument
>
[ CC Ben ]
The Debian kernel-team has integrated your patchset v2.
In case you build without libbfd support there is [1].
So, feel free to take this for v3.
-Sedat-
[1] https://salsa.debian.org/kernel-team/linux/-/blob/sid/debian/patches/bugfix/all/tools-perf-fix-build-without-libbfd.patch
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> To: [email protected]
> To: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]
> Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
>
> Andres Freund (5):
> tools build: add feature test for init_disassemble_info API changes
> tools include: add dis-asm-compat.h to handle version differences
> tools perf: Fix compilation error with new binutils
> tools bpf_jit_disasm: Fix compilation error with new binutils
> tools bpftool: Fix compilation error with new binutils
>
> tools/bpf/Makefile | 7 ++-
> tools/bpf/bpf_jit_disasm.c | 5 +-
> tools/bpf/bpftool/Makefile | 7 ++-
> tools/bpf/bpftool/jit_disasm.c | 42 ++++++++++++---
> tools/build/Makefile.feature | 4 +-
> tools/build/feature/Makefile | 4 ++
> tools/build/feature/test-all.c | 4 ++
> .../feature/test-disassembler-init-styled.c | 13 +++++
> tools/include/tools/dis-asm-compat.h | 53 +++++++++++++++++++
> tools/perf/Makefile.config | 8 +++
> tools/perf/util/annotate.c | 7 +--
> 11 files changed, 137 insertions(+), 17 deletions(-)
> create mode 100644 tools/build/feature/test-disassembler-init-styled.c
> create mode 100644 tools/include/tools/dis-asm-compat.h
>
> --
> 2.37.0.3.g30cc8d0f14
>
On Thu, 2022-07-14 at 11:16 +0200, Sedat Dilek wrote:
> On Sun, Jul 3, 2022 at 11:25 PM Andres Freund <[email protected]> wrote:
> >
> > binutils changed the signature of init_disassemble_info(), which now causes
> > compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> > binutils commit:
> > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
> >
> > I first fixed this without introducing the compat header, as suggested by
> > Quentin, but I thought the amount of repeated boilerplate was a bit too
> > much. So instead I introduced a compat header to wrap the API changes. Even
> > tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> > looks nicer this way.
> >
> > I'm not regular contributor, so it very well might be my procedures are a
> > bit off...
> >
> > I am not sure I added the right [number of] people to CC?
> >
> > WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> > nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> > in feature/Makefile and why -ldl isn't needed in the other places. But...
> >
> > V2:
> > - split patches further, so that tools/bpf and tools/perf part are entirely
> > separate
> > - included a bit more information about tests I did in commit messages
> > - add a maybe_unused to fprintf_json_styled's style argument
> >
>
> [ CC Ben ]
>
> The Debian kernel-team has integrated your patchset v2.
>
> In case you build without libbfd support there is [1].
> So, feel free to take this for v3.
>
> -Sedat-
>
> [1] https://salsa.debian.org/kernel-team/linux/-/blob/sid/debian/patches/bugfix/all/tools-perf-fix-build-without-libbfd.patch
[...]
Thanks, I meant to send that fix upstream but got distracted. It
should really be folded into "tools perf: Fix compilation error with
new binutils".
Ben.
>
--
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.
Hi,
On 2022-07-14 15:25:44 +0200, Ben Hutchings wrote:
> Thanks, I meant to send that fix upstream but got distracted. It
> should really be folded into "tools perf: Fix compilation error with
> new binutils".
I'll try to send a new version out soon. I think the right process is to add
Signed-off-by: Ben Hutchings <[email protected]>
to the patch I squash it with?
Greetings,
Andres Freund
On Fri, 2022-07-15 at 12:16 -0700, Andres Freund wrote:
> Hi,
>
> On 2022-07-14 15:25:44 +0200, Ben Hutchings wrote:
> > Thanks, I meant to send that fix upstream but got distracted. It
> > should really be folded into "tools perf: Fix compilation error with
> > new binutils".
>
> I'll try to send a new version out soon. I think the right process is to add
> Signed-off-by: Ben Hutchings <[email protected]>
> to the patch I squash it with?
Yes, OK.
Ben.
--
Ben Hutchings
Unix is many things to many people,
but it's never been everything to anybody.
Em Fri, Jul 15, 2022 at 12:16:41PM -0700, Andres Freund escreveu:
> Hi,
>
> On 2022-07-14 15:25:44 +0200, Ben Hutchings wrote:
> > Thanks, I meant to send that fix upstream but got distracted. It
> > should really be folded into "tools perf: Fix compilation error with
> > new binutils".
>
> I'll try to send a new version out soon. I think the right process is to add
> Signed-off-by: Ben Hutchings <[email protected]>
> to the patch I squash it with?
Hi,
How is this going? Any new patch coming soon? :-)
- Arnaldo
Hi,
On 2022-07-27 12:47:16 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 15, 2022 at 12:16:41PM -0700, Andres Freund escreveu:
> > On 2022-07-14 15:25:44 +0200, Ben Hutchings wrote:
> > > Thanks, I meant to send that fix upstream but got distracted. It
> > > should really be folded into "tools perf: Fix compilation error with
> > > new binutils".
> >
> > I'll try to send a new version out soon. I think the right process is to add
> > Signed-off-by: Ben Hutchings <[email protected]>
> > to the patch I squash it with?
>
> Hi,
>
> How is this going? Any new patch coming soon? :-)
Sorry - had hoped to finish sending it out before my vacation (and then on the
flight, but wifi didn't work...). Now back, will work on it asap.
Greetings,
Andres Freund
binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
I first fixed this without introducing the compat header, as suggested by
Quentin, but I thought the amount of repeated boilerplate was a bit too
much. So instead I introduced a compat header to wrap the API changes. Even
tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
looks nicer this way.
I'm not regular contributor, so it very well might be my procedures are a
bit off...
I am not sure I added the right [number of] people to CC?
WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
in feature/Makefile and why -ldl isn't needed in the other places. But...
V2:
- split patches further, so that tools/bpf and tools/perf part are entirely
separate
- included a bit more information about tests I did in commit messages
- add a maybe_unused to fprintf_json_styled's style argument
V3:
- don't include dis-asm-compat.h when building without libbfd
(Ben Hutchings)
- don't include compiler.h in dis-asm-compat.h, use (void) casts instead,
to avoid compiler.h include due to potential licensing conflict
- dual-license dis-asm-compat.h, for better compatibility with the rest of
bpftool's code (suggested by Quentin Monnet)
- don't display feature-disassembler-init-styled test
(suggested by Jiri Olsa)
- don't display feature-disassembler-four-args test, I split this for the
different subsystems, but maybe that's overkill? (suggested by Jiri Olsa)
Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
CC: Ben Hutchings <[email protected]>
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/lkml/[email protected]
Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
Andres Freund (8):
tools build: Add feature test for init_disassemble_info API changes
tools build: Don't display disassembler-four-args feature test
tools include: add dis-asm-compat.h to handle version differences
tools perf: Fix compilation error with new binutils
tools bpf_jit_disasm: Fix compilation error with new binutils
tools bpf_jit_disasm: Don't display disassembler-four-args feature
test
tools bpftool: Fix compilation error with new binutils
tools bpftool: Don't display disassembler-four-args feature test
tools/bpf/Makefile | 7 ++-
tools/bpf/bpf_jit_disasm.c | 5 +-
tools/bpf/bpftool/Makefile | 8 ++-
tools/bpf/bpftool/jit_disasm.c | 42 +++++++++++---
tools/build/Makefile.feature | 4 +-
tools/build/feature/Makefile | 4 ++
tools/build/feature/test-all.c | 4 ++
.../feature/test-disassembler-init-styled.c | 13 +++++
tools/include/tools/dis-asm-compat.h | 55 +++++++++++++++++++
tools/perf/Makefile.config | 8 +++
tools/perf/util/annotate.c | 7 ++-
11 files changed, 138 insertions(+), 19 deletions(-)
create mode 100644 tools/build/feature/test-disassembler-init-styled.c
create mode 100644 tools/include/tools/dis-asm-compat.h
--
2.37.0.3.g30cc8d0f14
The feature check does not seem important enough to display. Suggested by
Jiri Olsa.
Cc: Jiri Olsa <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/bpf/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
index 664601ab1705..243b79f2b451 100644
--- a/tools/bpf/Makefile
+++ b/tools/bpf/Makefile
@@ -35,7 +35,7 @@ endif
FEATURE_USER = .bpf
FEATURE_TESTS = libbfd disassembler-four-args disassembler-init-styled
-FEATURE_DISPLAY = libbfd disassembler-four-args
+FEATURE_DISPLAY = libbfd
check_feat := 1
NON_CHECK_FEAT_TARGETS := clean bpftool_clean runqslower_clean resolve_btfids_clean
--
2.37.0.3.g30cc8d0f14
binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/perf/util/annotate.c, e.g. on debian
unstable. 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 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
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Sedat Dilek <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
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 73e0762092fe..ee417c321adb 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -298,6 +298,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
@@ -905,13 +906,16 @@ ifndef NO_LIBBFD
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 -lz -ldl
endif
endif
$(call feature_check,disassembler-four-args)
+ $(call feature_check,disassembler-init-styled)
endif
ifeq ($(feature-libbfd-buildid), 1)
@@ -1025,6 +1029,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 82cc396ef516..2c6a485c3de5 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1720,6 +1720,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
#include <bpf/btf.h>
#include <bpf/libbpf.h>
#include <linux/btf.h>
+#include <tools/dis-asm-compat.h>
static int symbol__disassemble_bpf(struct symbol *sym,
struct annotate_args *args)
@@ -1762,9 +1763,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.37.0.3.g30cc8d0f14
The feature check does not seem important enough to display. Suggested by
Jiri Olsa.
Cc: Jiri Olsa <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/build/Makefile.feature | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 8f6578e4d324..fc6ce0b2535a 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -135,8 +135,7 @@ FEATURE_DISPLAY ?= \
get_cpuid \
bpf \
libaio \
- libzstd \
- disassembler-four-args
+ libzstd
# Set FEATURE_CHECK_(C|LD)FLAGS-all for all FEATURE_TESTS features.
# If in the future we need per-feature checks/flags for features not
--
2.37.0.3.g30cc8d0f14
binutils changed the signature of init_disassemble_info(), which now causes
compilation to fail for tools/bpf/bpftool/jit_disasm.c, e.g. on debian
unstable. 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 verified that bpftool can still disassemble bpf programs, both with an
old and new dis-asm.h API. There are no output changes for plain and json
formats. 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:
2f: pop %r14
31: pop %r13
33: pop %rbx
- 34: leaveq
- 35: retq
+ 34: leave
+ 35: ret
Cc: Alexei Starovoitov <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/bpf/bpftool/Makefile | 5 +++-
tools/bpf/bpftool/jit_disasm.c | 42 +++++++++++++++++++++++++++-------
2 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index c6d2c77d0252..436e671b2657 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -93,7 +93,7 @@ INSTALL ?= install
RM ?= rm -f
FEATURE_USER = .bpftool
-FEATURE_TESTS = libbfd disassembler-four-args zlib libcap \
+FEATURE_TESTS = libbfd disassembler-four-args disassembler-init-styled zlib libcap \
clang-bpf-co-re
FEATURE_DISPLAY = libbfd disassembler-four-args zlib libcap \
clang-bpf-co-re
@@ -117,6 +117,9 @@ endif
ifeq ($(feature-disassembler-four-args), 1)
CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
endif
+ifeq ($(feature-disassembler-init-styled), 1)
+ CFLAGS += -DDISASM_INIT_STYLED
+endif
LIBS = $(LIBBPF) -lelf -lz
LIBS_BOOTSTRAP = $(LIBBPF_BOOTSTRAP) -lelf -lz
diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
index 24734f2249d6..aaf99a0168c9 100644
--- a/tools/bpf/bpftool/jit_disasm.c
+++ b/tools/bpf/bpftool/jit_disasm.c
@@ -24,6 +24,7 @@
#include <sys/stat.h>
#include <limits.h>
#include <bpf/libbpf.h>
+#include <tools/dis-asm-compat.h>
#include "json_writer.h"
#include "main.h"
@@ -39,15 +40,12 @@ static void get_exec_path(char *tpath, size_t size)
}
static int oper_count;
-static int fprintf_json(void *out, const char *fmt, ...)
+static int printf_json(void *out, const char *fmt, va_list ap)
{
- va_list ap;
char *s;
int err;
- va_start(ap, fmt);
err = vasprintf(&s, fmt, ap);
- va_end(ap);
if (err < 0)
return -1;
@@ -73,6 +71,32 @@ static int fprintf_json(void *out, const char *fmt, ...)
return 0;
}
+static int fprintf_json(void *out, const char *fmt, ...)
+{
+ va_list ap;
+ int r;
+
+ va_start(ap, fmt);
+ r = printf_json(out, fmt, ap);
+ va_end(ap);
+
+ return r;
+}
+
+static int fprintf_json_styled(void *out,
+ enum disassembler_style style __maybe_unused,
+ const char *fmt, ...)
+{
+ va_list ap;
+ int r;
+
+ va_start(ap, fmt);
+ r = printf_json(out, fmt, ap);
+ va_end(ap);
+
+ return r;
+}
+
void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
const char *arch, const char *disassembler_options,
const struct btf *btf,
@@ -99,11 +123,13 @@ void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
assert(bfd_check_format(bfdf, bfd_object));
if (json_output)
- init_disassemble_info(&info, stdout,
- (fprintf_ftype) fprintf_json);
+ init_disassemble_info_compat(&info, stdout,
+ (fprintf_ftype) fprintf_json,
+ fprintf_json_styled);
else
- init_disassemble_info(&info, stdout,
- (fprintf_ftype) fprintf);
+ init_disassemble_info_compat(&info, stdout,
+ (fprintf_ftype) fprintf,
+ fprintf_styled);
/* Update architecture info for offload. */
if (arch) {
--
2.37.0.3.g30cc8d0f14
The feature check does not seem important enough to display. Requested by
Jiri Olsa.
Cc: Jiri Olsa <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/bpf/bpftool/Makefile | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 436e671b2657..517df016d54a 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -95,8 +95,7 @@ RM ?= rm -f
FEATURE_USER = .bpftool
FEATURE_TESTS = libbfd disassembler-four-args disassembler-init-styled zlib libcap \
clang-bpf-co-re
-FEATURE_DISPLAY = libbfd disassembler-four-args zlib libcap \
- clang-bpf-co-re
+FEATURE_DISPLAY = libbfd zlib libcap clang-bpf-co-re
check_feat := 1
NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall
--
2.37.0.3.g30cc8d0f14
binutils changed the signature of init_disassemble_info(), which now causes
compilation to fail for tools/bpf/bpf_jit_disasm.c, e.g. on debian
unstable. 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 verified that bpf_jit_disasm can still disassemble bpf programs, both
with the old and new dis-asm.h API. 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:
f4: mov %r14,%rsi
f7: mov %r15,%rdx
fa: mov $0x2a,%ecx
- ff: callq 0xffffffffea8c4988
+ ff: call 0xffffffffea8c4988
104: test %rax,%rax
107: jge 0x0000000000000110
109: xor %eax,%eax
- 10b: jmpq 0x0000000000000073
+ 10b: jmp 0x0000000000000073
110: cmp $0x16,%rax
However, I had to use an older kernel to generate the bpf_jit_enabled = 2
output, as that has been broken since 5.18 / 1022a5498f6f:
https://lore.kernel.org/[email protected]
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/bpf/Makefile | 5 ++++-
tools/bpf/bpf_jit_disasm.c | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
index b11cfc86a3d0..664601ab1705 100644
--- a/tools/bpf/Makefile
+++ b/tools/bpf/Makefile
@@ -34,7 +34,7 @@ else
endif
FEATURE_USER = .bpf
-FEATURE_TESTS = libbfd disassembler-four-args
+FEATURE_TESTS = libbfd disassembler-four-args disassembler-init-styled
FEATURE_DISPLAY = libbfd disassembler-four-args
check_feat := 1
@@ -56,6 +56,9 @@ endif
ifeq ($(feature-disassembler-four-args), 1)
CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
endif
+ifeq ($(feature-disassembler-init-styled), 1)
+CFLAGS += -DDISASM_INIT_STYLED
+endif
$(OUTPUT)%.yacc.c: $(srctree)/tools/bpf/%.y
$(QUIET_BISON)$(YACC) -o $@ -d $<
diff --git a/tools/bpf/bpf_jit_disasm.c b/tools/bpf/bpf_jit_disasm.c
index c8ae95804728..a90a5d110f92 100644
--- a/tools/bpf/bpf_jit_disasm.c
+++ b/tools/bpf/bpf_jit_disasm.c
@@ -28,6 +28,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <limits.h>
+#include <tools/dis-asm-compat.h>
#define CMD_ACTION_SIZE_BUFFER 10
#define CMD_ACTION_READ_ALL 3
@@ -64,7 +65,9 @@ static void get_asm_insns(uint8_t *image, size_t len, int opcodes)
assert(bfdf);
assert(bfd_check_format(bfdf, bfd_object));
- init_disassemble_info(&info, stdout, (fprintf_ftype) fprintf);
+ init_disassemble_info_compat(&info, stdout,
+ (fprintf_ftype) fprintf,
+ fprintf_styled);
info.arch = bfd_get_arch(bfdf);
info.mach = bfd_get_mach(bfdf);
info.buffer = image;
--
2.37.0.3.g30cc8d0f14
binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/{perf,bpf}, e.g. on debian unstable.
Relevant binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
This commit introduces a wrapper for init_disassemble_info(), to avoid
spreading #ifdef DISASM_INIT_STYLED to a bunch of places. Subsequent
commits will use it to fix the build failures.
It likely is worth adding a wrapper for disassember(), to avoid the already
existing DISASM_FOUR_ARGS_SIGNATURE ifdefery.
Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Cc: Ben Hutchings <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
Signed-off-by: Ben Hutchings <[email protected]>
---
tools/include/tools/dis-asm-compat.h | 55 ++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 tools/include/tools/dis-asm-compat.h
diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
new file mode 100644
index 000000000000..70f331e23ed3
--- /dev/null
+++ b/tools/include/tools/dis-asm-compat.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+#ifndef _TOOLS_DIS_ASM_COMPAT_H
+#define _TOOLS_DIS_ASM_COMPAT_H
+
+#include <stdio.h>
+#include <dis-asm.h>
+
+/* define types for older binutils version, to centralize ifdef'ery a bit */
+#ifndef DISASM_INIT_STYLED
+enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
+typedef int (*fprintf_styled_ftype) (void *, enum disassembler_style, const char*, ...);
+#endif
+
+/*
+ * Trivial fprintf wrapper to be used as the fprintf_styled_func argument to
+ * init_disassemble_info_compat() when normal fprintf suffices.
+ */
+static inline int fprintf_styled(void *out,
+ enum disassembler_style style,
+ const char *fmt, ...)
+{
+ va_list args;
+ int r;
+
+ (void)style;
+
+ va_start(args, fmt);
+ r = vfprintf(out, fmt, args);
+ va_end(args);
+
+ return r;
+}
+
+/*
+ * Wrapper for init_disassemble_info() that hides version
+ * differences. Depending on binutils version and architecture either
+ * fprintf_func or fprintf_styled_func will be called.
+ */
+static inline void init_disassemble_info_compat(struct disassemble_info *info,
+ void *stream,
+ fprintf_ftype unstyled_func,
+ fprintf_styled_ftype styled_func)
+{
+#ifdef DISASM_INIT_STYLED
+ init_disassemble_info(info, stream,
+ unstyled_func,
+ styled_func);
+#else
+ (void)styled_func;
+ init_disassemble_info(info, stream,
+ unstyled_func);
+#endif
+}
+
+#endif /* _TOOLS_DIS_ASM_COMPAT_H */
--
2.37.0.3.g30cc8d0f14
binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/{perf,bpf}, e.g. on debian unstable.
Relevant binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
This commit adds a feature test to detect the new signature. Subsequent
commits will use it to fix the build failures.
Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/build/Makefile.feature | 1 +
tools/build/feature/Makefile | 4 ++++
tools/build/feature/test-all.c | 4 ++++
tools/build/feature/test-disassembler-init-styled.c | 13 +++++++++++++
4 files changed, 22 insertions(+)
create mode 100644 tools/build/feature/test-disassembler-init-styled.c
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 888a0421d43b..8f6578e4d324 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -70,6 +70,7 @@ FEATURE_TESTS_BASIC := \
libaio \
libzstd \
disassembler-four-args \
+ disassembler-init-styled \
file-handle
# FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index 7c2a17e23c30..c3059739318a 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -18,6 +18,7 @@ FILES= \
test-libbfd.bin \
test-libbfd-buildid.bin \
test-disassembler-four-args.bin \
+ test-disassembler-init-styled.bin \
test-reallocarray.bin \
test-libbfd-liberty.bin \
test-libbfd-liberty-z.bin \
@@ -248,6 +249,9 @@ $(OUTPUT)test-libbfd-buildid.bin:
$(OUTPUT)test-disassembler-four-args.bin:
$(BUILD) -DPACKAGE='"perf"' -lbfd -lopcodes
+$(OUTPUT)test-disassembler-init-styled.bin:
+ $(BUILD) -DPACKAGE='"perf"' -lbfd -lopcodes
+
$(OUTPUT)test-reallocarray.bin:
$(BUILD)
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 5ffafb967b6e..957c02c7b163 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -166,6 +166,10 @@
# 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
diff --git a/tools/build/feature/test-disassembler-init-styled.c b/tools/build/feature/test-disassembler-init-styled.c
new file mode 100644
index 000000000000..f1ce0ec3bee9
--- /dev/null
+++ b/tools/build/feature/test-disassembler-init-styled.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stdio.h>
+#include <dis-asm.h>
+
+int main(void)
+{
+ struct disassemble_info info;
+
+ init_disassemble_info(&info, stdout,
+ NULL, NULL);
+
+ return 0;
+}
--
2.37.0.3.g30cc8d0f14
Hi,
On 2022-07-05 00:12:37 +0200, Jiri Olsa wrote:
> On Mon, Jul 04, 2022 at 01:19:22PM -0700, Andres Freund wrote:
> > > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > > index ee417c321adb..2aa0bad11f05 100644
> > > --- a/tools/perf/Makefile.config
> > > +++ b/tools/perf/Makefile.config
> > > @@ -914,8 +914,6 @@ ifndef NO_LIBBFD
> > > FEATURE_CHECK_LDFLAGS-disassembler-init-styled += -liberty -lz -ldl
> > > endif
> > > endif
> > > - $(call feature_check,disassembler-four-args)
> > > - $(call feature_check,disassembler-init-styled)
> > > endif
> > >
> > > ifeq ($(feature-libbfd-buildid), 1)
> > > @@ -1025,6 +1023,9 @@ ifdef HAVE_KVM_STAT_SUPPORT
> > > CFLAGS += -DHAVE_KVM_STAT_SUPPORT
> > > endif
> > >
> > > +$(call feature_check,disassembler-four-args)
> > > +$(call feature_check,disassembler-init-styled)
> > > +
> > > ifeq ($(feature-disassembler-four-args), 1)
> > > CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
> > > endif
> >
> > This I don't understand - why do we want these to run under NO_LIBBFD etc?
>
> when I was quickly testing that I did not have any of them detected
> and got compile fail.. so I moved it to safe place ;-) it might be
> placed in smarter place
I think that's because you'd removed them from FEATURE_TESTS_BASIC in
Makefile.feature. In v3 I just sent out I just removed them from
FEATURE_DISPLAY, without any more "structural" changes in
tools/perf/Makefile.config.
Greetings,
Andres Freund
Em Sun, Jul 31, 2022 at 06:38:26PM -0700, Andres Freund escreveu:
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
> I first fixed this without introducing the compat header, as suggested by
> Quentin, but I thought the amount of repeated boilerplate was a bit too
> much. So instead I introduced a compat header to wrap the API changes. Even
> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> looks nicer this way.
>
> I'm not regular contributor, so it very well might be my procedures are a
> bit off...
>
> I am not sure I added the right [number of] people to CC?
I think its ok
> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
I think its related to libbfd, and it comes from a long time ago, trying
to find the cset adding that...
> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> in feature/Makefile and why -ldl isn't needed in the other places. But...
>
> V2:
> - split patches further, so that tools/bpf and tools/perf part are entirely
> separate
Cool, thanks, I'll process the first 4 patches, then at some point the
bpftool bits can be merged, alternatively I can process those as well if
the bpftool maintainers are ok with it.
I'll just wait a bit to see if Jiri and others have something to say.
- Arnaldo
> - included a bit more information about tests I did in commit messages
> - add a maybe_unused to fprintf_json_styled's style argument
>
> V3:
> - don't include dis-asm-compat.h when building without libbfd
> (Ben Hutchings)
> - don't include compiler.h in dis-asm-compat.h, use (void) casts instead,
> to avoid compiler.h include due to potential licensing conflict
> - dual-license dis-asm-compat.h, for better compatibility with the rest of
> bpftool's code (suggested by Quentin Monnet)
> - don't display feature-disassembler-init-styled test
> (suggested by Jiri Olsa)
> - don't display feature-disassembler-four-args test, I split this for the
> different subsystems, but maybe that's overkill? (suggested by Jiri Olsa)
>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> CC: Ben Hutchings <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]
> Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
>
> Andres Freund (8):
> tools build: Add feature test for init_disassemble_info API changes
> tools build: Don't display disassembler-four-args feature test
> tools include: add dis-asm-compat.h to handle version differences
> tools perf: Fix compilation error with new binutils
> tools bpf_jit_disasm: Fix compilation error with new binutils
> tools bpf_jit_disasm: Don't display disassembler-four-args feature
> test
> tools bpftool: Fix compilation error with new binutils
> tools bpftool: Don't display disassembler-four-args feature test
>
> tools/bpf/Makefile | 7 ++-
> tools/bpf/bpf_jit_disasm.c | 5 +-
> tools/bpf/bpftool/Makefile | 8 ++-
> tools/bpf/bpftool/jit_disasm.c | 42 +++++++++++---
> tools/build/Makefile.feature | 4 +-
> tools/build/feature/Makefile | 4 ++
> tools/build/feature/test-all.c | 4 ++
> .../feature/test-disassembler-init-styled.c | 13 +++++
> tools/include/tools/dis-asm-compat.h | 55 +++++++++++++++++++
> tools/perf/Makefile.config | 8 +++
> tools/perf/util/annotate.c | 7 ++-
> 11 files changed, 138 insertions(+), 19 deletions(-)
> create mode 100644 tools/build/feature/test-disassembler-init-styled.c
> create mode 100644 tools/include/tools/dis-asm-compat.h
>
> --
> 2.37.0.3.g30cc8d0f14
--
- Arnaldo
On 01/08/2022 13:45, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jul 31, 2022 at 06:38:26PM -0700, Andres Freund escreveu:
>> binutils changed the signature of init_disassemble_info(), which now causes
>> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
>> binutils commit:
>> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>>
>> I first fixed this without introducing the compat header, as suggested by
>> Quentin, but I thought the amount of repeated boilerplate was a bit too
>> much. So instead I introduced a compat header to wrap the API changes. Even
>> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
>> looks nicer this way.
>>
>> I'm not regular contributor, so it very well might be my procedures are a
>> bit off...
>>
>> I am not sure I added the right [number of] people to CC?
>
> I think its ok
>
>> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
>
> I think its related to libbfd, and it comes from a long time ago, trying
> to find the cset adding that...
>
>> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
>> in feature/Makefile and why -ldl isn't needed in the other places. But...
>>
>> V2:
>> - split patches further, so that tools/bpf and tools/perf part are entirely
>> separate
>
> Cool, thanks, I'll process the first 4 patches, then at some point the
> bpftool bits can be merged, alternatively I can process those as well if
> the bpftool maintainers are ok with it.
>
> I'll just wait a bit to see if Jiri and others have something to say.
>
> - Arnaldo
Thanks for this work! For the series:
Acked-by: Quentin Monnet <[email protected]>
For what it's worth, it would make sense to me that these patches remain
together (so, through Arnaldo's tree), given that both the perf and
bpftool parts depend on dis-asm-compat.h being available.
Quentin
Em Mon, Aug 01, 2022 at 04:15:19PM +0100, Quentin Monnet escreveu:
> On 01/08/2022 13:45, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Jul 31, 2022 at 06:38:26PM -0700, Andres Freund escreveu:
> >> binutils changed the signature of init_disassemble_info(), which now causes
> >> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> >> binutils commit:
> >> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
> >>
> >> I first fixed this without introducing the compat header, as suggested by
> >> Quentin, but I thought the amount of repeated boilerplate was a bit too
> >> much. So instead I introduced a compat header to wrap the API changes. Even
> >> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> >> looks nicer this way.
> >>
> >> I'm not regular contributor, so it very well might be my procedures are a
> >> bit off...
> >>
> >> I am not sure I added the right [number of] people to CC?
> >
> > I think its ok
> >
> >> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> >
> > I think its related to libbfd, and it comes from a long time ago, trying
> > to find the cset adding that...
> >
> >> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> >> in feature/Makefile and why -ldl isn't needed in the other places. But...
> >>
> >> V2:
> >> - split patches further, so that tools/bpf and tools/perf part are entirely
> >> separate
> >
> > Cool, thanks, I'll process the first 4 patches, then at some point the
> > bpftool bits can be merged, alternatively I can process those as well if
> > the bpftool maintainers are ok with it.
> >
> > I'll just wait a bit to see if Jiri and others have something to say.
> >
> > - Arnaldo
>
> Thanks for this work! For the series:
>
> Acked-by: Quentin Monnet <[email protected]>
>
> For what it's worth, it would make sense to me that these patches remain
> together (so, through Arnaldo's tree), given that both the perf and
> bpftool parts depend on dis-asm-compat.h being available.
Ok, so I'm tentatively adding it to my local tree to do some tests, if
someone disagrees, please holler.
- Arnaldo
Em Sun, Jul 31, 2022 at 06:38:29PM -0700, Andres Freund escreveu:
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf}, e.g. on debian unstable.
> Relevant binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
> This commit introduces a wrapper for init_disassemble_info(), to avoid
> spreading #ifdef DISASM_INIT_STYLED to a bunch of places. Subsequent
> commits will use it to fix the build failures.
>
> It likely is worth adding a wrapper for disassember(), to avoid the already
> existing DISASM_FOUR_ARGS_SIGNATURE ifdefery.
>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> Cc: Ben Hutchings <[email protected]>
> Link: http://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Andres Freund <[email protected]>
> Signed-off-by: Ben Hutchings <[email protected]>
So, who is the author of this patch? Ben? b4 complained about it:
NOTE: some trailers ignored due to from/email mismatches:
! Trailer: Signed-off-by: Ben Hutchings <[email protected]>
Msg From: Andres Freund <[email protected]>
NOTE: Rerun with -S to apply them anyway
If it is Ben, then we would need a:
From: Ben Hutchings <[email protected]>
At the beginning of the patch, right?
- Arnaldo
> ---
> tools/include/tools/dis-asm-compat.h | 55 ++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 tools/include/tools/dis-asm-compat.h
>
> diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
> new file mode 100644
> index 000000000000..70f331e23ed3
> --- /dev/null
> +++ b/tools/include/tools/dis-asm-compat.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +#ifndef _TOOLS_DIS_ASM_COMPAT_H
> +#define _TOOLS_DIS_ASM_COMPAT_H
> +
> +#include <stdio.h>
> +#include <dis-asm.h>
> +
> +/* define types for older binutils version, to centralize ifdef'ery a bit */
> +#ifndef DISASM_INIT_STYLED
> +enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
> +typedef int (*fprintf_styled_ftype) (void *, enum disassembler_style, const char*, ...);
> +#endif
> +
> +/*
> + * Trivial fprintf wrapper to be used as the fprintf_styled_func argument to
> + * init_disassemble_info_compat() when normal fprintf suffices.
> + */
> +static inline int fprintf_styled(void *out,
> + enum disassembler_style style,
> + const char *fmt, ...)
> +{
> + va_list args;
> + int r;
> +
> + (void)style;
> +
> + va_start(args, fmt);
> + r = vfprintf(out, fmt, args);
> + va_end(args);
> +
> + return r;
> +}
> +
> +/*
> + * Wrapper for init_disassemble_info() that hides version
> + * differences. Depending on binutils version and architecture either
> + * fprintf_func or fprintf_styled_func will be called.
> + */
> +static inline void init_disassemble_info_compat(struct disassemble_info *info,
> + void *stream,
> + fprintf_ftype unstyled_func,
> + fprintf_styled_ftype styled_func)
> +{
> +#ifdef DISASM_INIT_STYLED
> + init_disassemble_info(info, stream,
> + unstyled_func,
> + styled_func);
> +#else
> + (void)styled_func;
> + init_disassemble_info(info, stream,
> + unstyled_func);
> +#endif
> +}
> +
> +#endif /* _TOOLS_DIS_ASM_COMPAT_H */
> --
> 2.37.0.3.g30cc8d0f14
--
- Arnaldo
Em Sun, Jul 31, 2022 at 06:38:32PM -0700, Andres Freund escreveu:
> The feature check does not seem important enough to display. Suggested by
> Jiri Olsa.
>
> Cc: Jiri Olsa <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> Link: http://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Andres Freund <[email protected]>
> ---
> tools/bpf/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
> index 664601ab1705..243b79f2b451 100644
> --- a/tools/bpf/Makefile
> +++ b/tools/bpf/Makefile
> @@ -35,7 +35,7 @@ endif
>
> FEATURE_USER = .bpf
> FEATURE_TESTS = libbfd disassembler-four-args disassembler-init-styled
> -FEATURE_DISPLAY = libbfd disassembler-four-args
> +FEATURE_DISPLAY = libbfd
>
> check_feat := 1
> NON_CHECK_FEAT_TARGETS := clean bpftool_clean runqslower_clean resolve_btfids_clean
> --
> 2.37.0.3.g30cc8d0f14
After this patch:
⬢[acme@toolbox perf]$ git log --oneline -7
cebe4f3a4a0af5bf (HEAD) tools bpf_jit_disasm: Don't display disassembler-four-args feature test
7f62593e5582cb27 tools bpf_jit_disasm: Fix compilation error with new binutils
ee4dc290ee5c09b7 tools perf: Fix compilation error with new binutils
335f8d183a609793 tools include: add dis-asm-compat.h to handle version differences
f2f95e8d0def9c5f tools build: Don't display disassembler-four-args feature test
ede0fece841bb743 tools build: Add feature test for init_disassemble_info API changes
00b32625982e0c79 perf test: Add ARM SPE system wide test
⬢[acme@toolbox perf]$
⬢[acme@toolbox perf]$ make -C tools/bpf/bpftool/ clean
make: Entering directory '/var/home/acme/git/perf/tools/bpf/bpftool'
CLEAN libbpf
CLEAN libbpf-bootstrap
CLEAN feature-detect
CLEAN bpftool
CLEAN core-gen
make: Leaving directory '/var/home/acme/git/perf/tools/bpf/bpftool'
⬢[acme@toolbox perf]$ make -C tools/bpf/bpftool/
make: Entering directory '/var/home/acme/git/perf/tools/bpf/bpftool'
Auto-detecting system features:
... libbfd: [ on ]
... disassembler-four-args: [ on ]
... zlib: [ on ]
... libcap: [ on ]
... clang-bpf-co-re: [ on ]
<SNIP>
It is still there, we need the hunk below, that I folded into your patch, to
disable it, please ack :-)
- Arnaldo
diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index c6d2c77d02524a37..a92fb4d312ec2363 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -95,7 +95,7 @@ RM ?= rm -f
FEATURE_USER = .bpftool
FEATURE_TESTS = libbfd disassembler-four-args zlib libcap \
clang-bpf-co-re
-FEATURE_DISPLAY = libbfd disassembler-four-args zlib libcap \
+FEATURE_DISPLAY = libbfd zlib libcap \
clang-bpf-co-re
check_feat := 1
Hi,
On 2022-08-01 15:05:00 -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jul 31, 2022 at 06:38:29PM -0700, Andres Freund escreveu:
> > binutils changed the signature of init_disassemble_info(), which now causes
> > compilation failures for tools/{perf,bpf}, e.g. on debian unstable.
> > Relevant binutils commit:
> > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
> >
> > This commit introduces a wrapper for init_disassemble_info(), to avoid
> > spreading #ifdef DISASM_INIT_STYLED to a bunch of places. Subsequent
> > commits will use it to fix the build failures.
> >
> > It likely is worth adding a wrapper for disassember(), to avoid the already
> > existing DISASM_FOUR_ARGS_SIGNATURE ifdefery.
> >
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Sedat Dilek <[email protected]>
> > Cc: Quentin Monnet <[email protected]>
> > Cc: Ben Hutchings <[email protected]>
> > Link: http://lore.kernel.org/lkml/[email protected]
> > Signed-off-by: Andres Freund <[email protected]>
> > Signed-off-by: Ben Hutchings <[email protected]>
>
> So, who is the author of this patch? Ben? b4 complained about it:
I squashed a fixup of Ben into my patch (moving the include in annotate.c into
the HAVE_LIBBFD_SUPPORT section). I don't know what the proper procedure is
for that - I'd asked in
https://lore.kernel.org/[email protected]
> NOTE: some trailers ignored due to from/email mismatches:
> ! Trailer: Signed-off-by: Ben Hutchings <[email protected]>
> Msg From: Andres Freund <[email protected]>
> NOTE: Rerun with -S to apply them anyway
>
> If it is Ben, then we would need a:
>
> From: Ben Hutchings <[email protected]>
>
> At the beginning of the patch, right?
I don't know, I interact with the kernel processes too rarely...
Greetings,
Andres Freund
Em Sun, Jul 31, 2022 at 06:38:28PM -0700, Andres Freund escreveu:
> The feature check does not seem important enough to display. Suggested by
> Jiri Olsa.
I turned the last paragraph into a:
Suggested-by: Jiri Olsa <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> Link: http://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Andres Freund <[email protected]>
> ---
> tools/build/Makefile.feature | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
> index 8f6578e4d324..fc6ce0b2535a 100644
> --- a/tools/build/Makefile.feature
> +++ b/tools/build/Makefile.feature
> @@ -135,8 +135,7 @@ FEATURE_DISPLAY ?= \
> get_cpuid \
> bpf \
> libaio \
> - libzstd \
> - disassembler-four-args
> + libzstd
>
> # Set FEATURE_CHECK_(C|LD)FLAGS-all for all FEATURE_TESTS features.
> # If in the future we need per-feature checks/flags for features not
> --
> 2.37.0.3.g30cc8d0f14
--
- Arnaldo
Hi,
On 2022-08-01 15:27:22 -0300, Arnaldo Carvalho de Melo wrote:
> ⬢[acme@toolbox perf]$ git log --oneline -7
> cebe4f3a4a0af5bf (HEAD) tools bpf_jit_disasm: Don't display disassembler-four-args feature test
> 7f62593e5582cb27 tools bpf_jit_disasm: Fix compilation error with new binutils
> ee4dc290ee5c09b7 tools perf: Fix compilation error with new binutils
> 335f8d183a609793 tools include: add dis-asm-compat.h to handle version differences
> f2f95e8d0def9c5f tools build: Don't display disassembler-four-args feature test
> ede0fece841bb743 tools build: Add feature test for init_disassemble_info API changes
> 00b32625982e0c79 perf test: Add ARM SPE system wide test
> ⬢[acme@toolbox perf]$
>
> ⬢[acme@toolbox perf]$ make -C tools/bpf/bpftool/ clean
> make: Entering directory '/var/home/acme/git/perf/tools/bpf/bpftool'
> CLEAN libbpf
> CLEAN libbpf-bootstrap
> CLEAN feature-detect
> CLEAN bpftool
> CLEAN core-gen
> make: Leaving directory '/var/home/acme/git/perf/tools/bpf/bpftool'
> ⬢[acme@toolbox perf]$ make -C tools/bpf/bpftool/
> make: Entering directory '/var/home/acme/git/perf/tools/bpf/bpftool'
>
> Auto-detecting system features:
> ... libbfd: [ on ]
> ... disassembler-four-args: [ on ]
> ... zlib: [ on ]
> ... libcap: [ on ]
> ... clang-bpf-co-re: [ on ]
> <SNIP>
>
> It is still there, we need the hunk below, that I folded into your patch, to
> disable it, please ack :-)
This commit just removed disassembler-four-args display for bpf_jit_disasm,
not bpftool. That should be in a later commit.
Greetings,
Andres Freund
Em Fri, Jul 15, 2022 at 09:18:26PM +0200, Ben Hutchings escreveu:
> On Fri, 2022-07-15 at 12:16 -0700, Andres Freund wrote:
> > Hi,
> >
> > On 2022-07-14 15:25:44 +0200, Ben Hutchings wrote:
> > > Thanks, I meant to send that fix upstream but got distracted. It
> > > should really be folded into "tools perf: Fix compilation error with
> > > new binutils".
> >
> > I'll try to send a new version out soon. I think the right process is to add
> > Signed-off-by: Ben Hutchings <[email protected]>
> > to the patch I squash it with?
>
> Yes, OK.
Ok, so you agreed on this, I'm adding Ben's Signed-off-by tag then,
Thanks,
- Arnaldo
Em Sun, Jul 31, 2022 at 06:38:34PM -0700, Andres Freund escreveu:
> The feature check does not seem important enough to display. Requested by
> Jiri Olsa.
Sorry, I hadn't seen this one, removing my change.
- Arnaldo
> Cc: Jiri Olsa <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> Link: http://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Andres Freund <[email protected]>
> ---
> tools/bpf/bpftool/Makefile | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 436e671b2657..517df016d54a 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -95,8 +95,7 @@ RM ?= rm -f
> FEATURE_USER = .bpftool
> FEATURE_TESTS = libbfd disassembler-four-args disassembler-init-styled zlib libcap \
> clang-bpf-co-re
> -FEATURE_DISPLAY = libbfd disassembler-four-args zlib libcap \
> - clang-bpf-co-re
> +FEATURE_DISPLAY = libbfd zlib libcap clang-bpf-co-re
>
> check_feat := 1
> NON_CHECK_FEAT_TARGETS := clean uninstall doc doc-clean doc-install doc-uninstall
> --
> 2.37.0.3.g30cc8d0f14
--
- Arnaldo
On Mon, Aug 1, 2022 at 3:38 AM Andres Freund <[email protected]> wrote:
>
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
> I first fixed this without introducing the compat header, as suggested by
> Quentin, but I thought the amount of repeated boilerplate was a bit too
> much. So instead I introduced a compat header to wrap the API changes. Even
> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> looks nicer this way.
>
> I'm not regular contributor, so it very well might be my procedures are a
> bit off...
>
> I am not sure I added the right [number of] people to CC?
>
> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> in feature/Makefile and why -ldl isn't needed in the other places. But...
>
> V2:
> - split patches further, so that tools/bpf and tools/perf part are entirely
> separate
> - included a bit more information about tests I did in commit messages
> - add a maybe_unused to fprintf_json_styled's style argument
>
> V3:
> - don't include dis-asm-compat.h when building without libbfd
> (Ben Hutchings)
> - don't include compiler.h in dis-asm-compat.h, use (void) casts instead,
> to avoid compiler.h include due to potential licensing conflict
> - dual-license dis-asm-compat.h, for better compatibility with the rest of
> bpftool's code (suggested by Quentin Monnet)
> - don't display feature-disassembler-init-styled test
> (suggested by Jiri Olsa)
> - don't display feature-disassembler-four-args test, I split this for the
> different subsystems, but maybe that's overkill? (suggested by Jiri Olsa)
>
Hi Andres,
Just made a quick test & run with some custom patchset and LLVM-15 RC1:
[ REPRODUCER ]
LLVM_MVER="15"
##LLVM_TOOLCHAIN_PATH="/usr/lib/llvm-${LLVM_MVER}/bin"
LLVM_TOOLCHAIN_PATH="/opt/llvm/bin"
if [ -d ${LLVM_TOOLCHAIN_PATH} ]; then
export PATH="${LLVM_TOOLCHAIN_PATH}:${PATH}"
fi
PYTHON_VER="3.10"
MAKE="make"
MAKE_OPTS="V=1 -j1 HOSTCC=clang-$LLVM_MVER HOSTLD=ld.lld
HOSTAR=llvm-ar CC=clang-$LLVM_MVER LD=ld.lld AR=llvm-ar
STRIP=llvm-strip"
echo "LLVM MVER ........ $LLVM_MVER"
echo "Path settings .... $PATH"
echo "Python version ... $PYTHON_VER"
echo "make line ........ $MAKE $MAKE_OPTS"
LANG=C LC_ALL=C make -C tools/perf clean 2>&1 | tee ../make-log_perf-clean.txt
LANG=C LC_ALL=C $MAKE $MAKE_OPTS -C tools/perf
PYTHON=python${PYTHON_VER} install-bin 2>&1 | tee
../make-log_perf-install_bin_python${PYTHON_VER}_llvm${LLVM_MVER}.txt
Looks good.
$ ~/bin/perf -vv
perf version 5.19.0
dwarf: [ on ] # HAVE_DWARF_SUPPORT
dwarf_getlocations: [ on ] # HAVE_DWARF_GETLOCATIONS_SUPPORT
glibc: [ on ] # HAVE_GLIBC_SUPPORT
syscall_table: [ on ] # HAVE_SYSCALL_TABLE_SUPPORT
libbfd: [ on ] # HAVE_LIBBFD_SUPPORT
debuginfod: [ OFF ] # HAVE_DEBUGINFOD_SUPPORT
libelf: [ on ] # HAVE_LIBELF_SUPPORT
libnuma: [ on ] # HAVE_LIBNUMA_SUPPORT
numa_num_possible_cpus: [ on ] # HAVE_LIBNUMA_SUPPORT
libperl: [ on ] # HAVE_LIBPERL_SUPPORT
libpython: [ on ] # HAVE_LIBPYTHON_SUPPORT
libslang: [ on ] # HAVE_SLANG_SUPPORT
libcrypto: [ on ] # HAVE_LIBCRYPTO_SUPPORT
libunwind: [ on ] # HAVE_LIBUNWIND_SUPPORT
libdw-dwarf-unwind: [ on ] # HAVE_DWARF_SUPPORT
zlib: [ on ] # HAVE_ZLIB_SUPPORT
lzma: [ on ] # HAVE_LZMA_SUPPORT
get_cpuid: [ on ] # HAVE_AUXTRACE_SUPPORT
bpf: [ on ] # HAVE_LIBBPF_SUPPORT
aio: [ on ] # HAVE_AIO_SUPPORT
zstd: [ on ] # HAVE_ZSTD_SUPPORT
libpfm4: [ OFF ] # HAVE_LIBPFM
[ Test on Intel Sandybridge CPU ]
$ echo 0 | sudo tee /proc/sys/kernel/kptr_restrict
/proc/sys/kernel/perf_event_paranoid
0
$ ~/bin/perf test 10 93 94 95
10: PMU events :
10.1: PMU event table sanity : Ok
10.2: PMU event map aliases : Ok
10.3: Parsing of PMU event table metrics : Ok
10.4: Parsing of PMU event table metrics with fake PMUs : Ok
93: perf all metricgroups test : Ok
94: perf all metrics test : Ok
95: perf all PMU test : Ok
Feel free to add my:
Tested-by: Sedat Dilek <[email protected]> # LLVM v15.0.0-rc1 (x86-64)
Regards,
-Sedat-
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> CC: Ben Hutchings <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Link: https://lore.kernel.org/lkml/[email protected]
> Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
>
> Andres Freund (8):
> tools build: Add feature test for init_disassemble_info API changes
> tools build: Don't display disassembler-four-args feature test
> tools include: add dis-asm-compat.h to handle version differences
> tools perf: Fix compilation error with new binutils
> tools bpf_jit_disasm: Fix compilation error with new binutils
> tools bpf_jit_disasm: Don't display disassembler-four-args feature
> test
> tools bpftool: Fix compilation error with new binutils
> tools bpftool: Don't display disassembler-four-args feature test
>
> tools/bpf/Makefile | 7 ++-
> tools/bpf/bpf_jit_disasm.c | 5 +-
> tools/bpf/bpftool/Makefile | 8 ++-
> tools/bpf/bpftool/jit_disasm.c | 42 +++++++++++---
> tools/build/Makefile.feature | 4 +-
> tools/build/feature/Makefile | 4 ++
> tools/build/feature/test-all.c | 4 ++
> .../feature/test-disassembler-init-styled.c | 13 +++++
> tools/include/tools/dis-asm-compat.h | 55 +++++++++++++++++++
> tools/perf/Makefile.config | 8 +++
> tools/perf/util/annotate.c | 7 ++-
> 11 files changed, 138 insertions(+), 19 deletions(-)
> create mode 100644 tools/build/feature/test-disassembler-init-styled.c
> create mode 100644 tools/include/tools/dis-asm-compat.h
>
> --
> 2.37.0.3.g30cc8d0f14
>
On Mon, Aug 01, 2022 at 09:45:06AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jul 31, 2022 at 06:38:26PM -0700, Andres Freund escreveu:
> > binutils changed the signature of init_disassemble_info(), which now causes
> > compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
> > binutils commit:
> > https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
> >
> > I first fixed this without introducing the compat header, as suggested by
> > Quentin, but I thought the amount of repeated boilerplate was a bit too
> > much. So instead I introduced a compat header to wrap the API changes. Even
> > tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
> > looks nicer this way.
> >
> > I'm not regular contributor, so it very well might be my procedures are a
> > bit off...
> >
> > I am not sure I added the right [number of] people to CC?
>
> I think its ok
>
> > WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
>
> I think its related to libbfd, and it comes from a long time ago, trying
> to find the cset adding that...
>
> > nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
> > in feature/Makefile and why -ldl isn't needed in the other places. But...
> >
> > V2:
> > - split patches further, so that tools/bpf and tools/perf part are entirely
> > separate
>
> Cool, thanks, I'll process the first 4 patches, then at some point the
> bpftool bits can be merged, alternatively I can process those as well if
> the bpftool maintainers are ok with it.
>
> I'll just wait a bit to see if Jiri and others have something to say.
looks good
Acked-by: Jiri Olsa <[email protected]>
thanks,
jirka
>
> - Arnaldo
>
> > - included a bit more information about tests I did in commit messages
> > - add a maybe_unused to fprintf_json_styled's style argument
> >
> > V3:
> > - don't include dis-asm-compat.h when building without libbfd
> > (Ben Hutchings)
> > - don't include compiler.h in dis-asm-compat.h, use (void) casts instead,
> > to avoid compiler.h include due to potential licensing conflict
> > - dual-license dis-asm-compat.h, for better compatibility with the rest of
> > bpftool's code (suggested by Quentin Monnet)
> > - don't display feature-disassembler-init-styled test
> > (suggested by Jiri Olsa)
> > - don't display feature-disassembler-four-args test, I split this for the
> > different subsystems, but maybe that's overkill? (suggested by Jiri Olsa)
> >
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Jiri Olsa <[email protected]>
> > Cc: Sedat Dilek <[email protected]>
> > Cc: Quentin Monnet <[email protected]>
> > CC: Ben Hutchings <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Link: https://lore.kernel.org/lkml/[email protected]
> > Link: https://lore.kernel.org/lkml/CA+icZUVpr8ZeOKCj4zMMqbFT013KJz2T1csvXg+VSkdvJH1Ubw@mail.gmail.com
> >
> > Andres Freund (8):
> > tools build: Add feature test for init_disassemble_info API changes
> > tools build: Don't display disassembler-four-args feature test
> > tools include: add dis-asm-compat.h to handle version differences
> > tools perf: Fix compilation error with new binutils
> > tools bpf_jit_disasm: Fix compilation error with new binutils
> > tools bpf_jit_disasm: Don't display disassembler-four-args feature
> > test
> > tools bpftool: Fix compilation error with new binutils
> > tools bpftool: Don't display disassembler-four-args feature test
> >
> > tools/bpf/Makefile | 7 ++-
> > tools/bpf/bpf_jit_disasm.c | 5 +-
> > tools/bpf/bpftool/Makefile | 8 ++-
> > tools/bpf/bpftool/jit_disasm.c | 42 +++++++++++---
> > tools/build/Makefile.feature | 4 +-
> > tools/build/feature/Makefile | 4 ++
> > tools/build/feature/test-all.c | 4 ++
> > .../feature/test-disassembler-init-styled.c | 13 +++++
> > tools/include/tools/dis-asm-compat.h | 55 +++++++++++++++++++
> > tools/perf/Makefile.config | 8 +++
> > tools/perf/util/annotate.c | 7 ++-
> > 11 files changed, 138 insertions(+), 19 deletions(-)
> > create mode 100644 tools/build/feature/test-disassembler-init-styled.c
> > create mode 100644 tools/include/tools/dis-asm-compat.h
> >
> > --
> > 2.37.0.3.g30cc8d0f14
>
> --
>
> - Arnaldo
On 8/1/22 8:02 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 01, 2022 at 04:15:19PM +0100, Quentin Monnet escreveu:
>> On 01/08/2022 13:45, Arnaldo Carvalho de Melo wrote:
>>> Em Sun, Jul 31, 2022 at 06:38:26PM -0700, Andres Freund escreveu:
>>>> binutils changed the signature of init_disassemble_info(), which now causes
>>>> compilation failures for tools/{perf,bpf} on e.g. debian unstable. Relevant
>>>> binutils commit:
>>>> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>>>>
>>>> I first fixed this without introducing the compat header, as suggested by
>>>> Quentin, but I thought the amount of repeated boilerplate was a bit too
>>>> much. So instead I introduced a compat header to wrap the API changes. Even
>>>> tools/bpf/bpftool/jit_disasm.c, which needs its own callbacks for json, imo
>>>> looks nicer this way.
>>>>
>>>> I'm not regular contributor, so it very well might be my procedures are a
>>>> bit off...
>>>>
>>>> I am not sure I added the right [number of] people to CC?
>>>
>>> I think its ok
>>>
>>>> WRT the feature test: Not sure what the point of the -DPACKAGE='"perf"' is,
>>>
>>> I think its related to libbfd, and it comes from a long time ago, trying
>>> to find the cset adding that...
>>>
>>>> nor why tools/perf/Makefile.config sets some LDFLAGS/CFLAGS that are also
>>>> in feature/Makefile and why -ldl isn't needed in the other places. But...
>>>>
>>>> V2:
>>>> - split patches further, so that tools/bpf and tools/perf part are entirely
>>>> separate
>>>
>>> Cool, thanks, I'll process the first 4 patches, then at some point the
>>> bpftool bits can be merged, alternatively I can process those as well if
>>> the bpftool maintainers are ok with it.
>>>
>>> I'll just wait a bit to see if Jiri and others have something to say.
>>>
>>> - Arnaldo
>>
>> Thanks for this work! For the series:
>>
>> Acked-by: Quentin Monnet <[email protected]>
>>
>> For what it's worth, it would make sense to me that these patches remain
>> together (so, through Arnaldo's tree), given that both the perf and
>> bpftool parts depend on dis-asm-compat.h being available.
>
> Ok, so I'm tentatively adding it to my local tree to do some tests, if
> someone disagrees, please holler.
Ack, sgtm. Please route these fixes via your tree. Thanks Arnaldo!