| Message ID | 20220622181918.ykrs5rsnmx3og4sv@alap3.anarazel.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | init_disassemble_info() signature changes causes compile failures | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, 22 Jun 2022 at 19:19, Andres Freund <andres@anarazel.de> 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 <andres@anarazel.de> 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
diff --git i/tools/perf/util/annotate.c w/tools/perf/util/annotate.c index 82cc396ef516..b0e364d235b4 100644 --- i/tools/perf/util/annotate.c +++ w/tools/perf/util/annotate.c @@ -1721,6 +1721,18 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil #include <bpf/libbpf.h> #include <linux/btf.h> +static int fprintf_styled(void *, enum disassembler_style, const char* fmt, ...) +{ + va_list args; + int r; + + va_start(args, fmt); + r = vprintf(fmt, args); + va_end(args); + + return r; +} + static int symbol__disassemble_bpf(struct symbol *sym, struct annotate_args *args) { @@ -1763,7 +1775,8 @@ static int symbol__disassemble_bpf(struct symbol *sym, goto out; } init_disassemble_info(&info, s, - (fprintf_ftype) fprintf); + (fprintf_ftype) fprintf, + fprintf_styled); info.arch = bfd_get_arch(bfdf); info.mach = bfd_get_mach(bfdf);
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