On Tue, Dec 19, 2017 at 03:57:02PM +0000, Quentin Monnet wrote:
> Hi Roman, thanks for working on this!
>
> 2017-12-19 14:38 UTC+0000 ~ Roman Gushchin <g...@fb.com>
> > Bpftool build is broken with binutils version 2.28 and later.
> > The cause is commit 003ca0fd2286 ("Refactor disassembler selection")
> > in the binutils repo, which changed the disassembler() function
> > signature.
> >
> > Fix this by checking binutils version and use an appropriate
> > disassembler() signature.
> >
> > Signed-off-by: Roman Gushchin <g...@fb.com>
> > Cc: Jakub Kicinski <jakub.kicin...@netronome.com>
> > Cc: Alexei Starovoitov <a...@kernel.org>
> > Cc: Daniel Borkmann <dan...@iogearbox.net>
> > ---
> > tools/bpf/Makefile | 6 ++++++
> > tools/bpf/bpf_jit_disasm.c | 7 +++++++
> > tools/bpf/bpftool/Makefile | 6 ++++++
> > tools/bpf/bpftool/jit_disasm.c | 5 +++++
> > 4 files changed, 24 insertions(+)
> >
> > diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
> > index 07a6697466ef..3fd32fd0daa1 100644
> > --- a/tools/bpf/Makefile
> > +++ b/tools/bpf/Makefile
> > @@ -6,8 +6,14 @@ LEX = flex
> > YACC = bison
> > MAKE = make
> >
> > +BINUTILS_VER := $(word 4, $(shell readelf -v | head -n 1))
> > +BINUTILS_VER_MAJ := $(word 1, $(subst ., , $(subst -, , ${BINUTILS_VER})))
> > +BINUTILS_VER_MIN := $(word 2, $(subst ., , $(subst -, , ${BINUTILS_VER})))
> > +
> > CFLAGS += -Wall -O2
> > CFLAGS += -D__EXPORTED_HEADERS__ -I../../include/uapi -I../../include
> > +CFLAGS += -DBINUTILS_VER_MAJ=${BINUTILS_VER_MAJ}
> > +CFLAGS += -DBINUTILS_VER_MIN=${BINUTILS_VER_MIN}
> >
> > %.yacc.c: %.y
> > $(YACC) -o $@ -d $<
> > diff --git a/tools/bpf/bpf_jit_disasm.c b/tools/bpf/bpf_jit_disasm.c
> > index 75bf526a0168..3ef7c8bdc0f3 100644
> > --- a/tools/bpf/bpf_jit_disasm.c
> > +++ b/tools/bpf/bpf_jit_disasm.c
> > @@ -72,7 +72,14 @@ static void get_asm_insns(uint8_t *image, size_t len,
> > int opcodes)
> >
> > disassemble_init_for_target(&info);
> >
> > +#if (BINUTILS_VER_MAJ >= 2) && (BINUTILS_VER_MIN >= 28)
> > + disassemble = disassembler(bfd_get_arch(bfdf),
> > + bfd_big_endian(bfdf),
> > + bfd_get_mach(bfdf),
> > + bfdf);
> > +#else
> > disassemble = disassembler(bfdf);
> > +#endif
> > assert(disassemble);
> >
> > do {
> > diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> > index 3f17ad317512..94ad51bf14b5 100644
> > --- a/tools/bpf/bpftool/Makefile
> > +++ b/tools/bpf/bpftool/Makefile
> > @@ -40,6 +40,12 @@ CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wshadow
> > CFLAGS += -D__EXPORTED_HEADERS__ -I$(srctree)/tools/include/uapi
> > -I$(srctree)/tools/include -I$(srctree)/tools/lib/bpf
> > -I$(srctree)/kernel/bpf/
> > LIBS = -lelf -lbfd -lopcodes $(LIBBPF)
> >
> > +BINUTILS_VER := $(word 4, $(shell readelf -v | head -n 1))
>
> This does not seem to be portable. I tried that on Ubuntu and `readelf
> -v` returns "GNU readelf (GNU Binutils for Ubuntu) 2.26.1", and
> BINUTILS_VER catches "Binutils".
>
> > +BINUTILS_VER_MAJ := $(word 1, $(subst ., , $(subst -, , ${BINUTILS_VER})))
> > +BINUTILS_VER_MIN := $(word 2, $(subst ., , $(subst -, , ${BINUTILS_VER})))
> > +CFLAGS += -DBINUTILS_VER_MAJ=${BINUTILS_VER_MAJ}
> > +CFLAGS += -DBINUTILS_VER_MIN=${BINUTILS_VER_MIN}
> > +
> > INSTALL ?= install
> > RM ?= rm -f
> >
> > diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
> > index 1551d3918d4c..eaa7127e9eeb 100644
> > --- a/tools/bpf/bpftool/jit_disasm.c
> > +++ b/tools/bpf/bpftool/jit_disasm.c
> > @@ -107,7 +107,12 @@ void disasm_print_insn(unsigned char *image, ssize_t
> > len, int opcodes)
> >
> > disassemble_init_for_target(&info);
> >
> > +#if (BINUTILS_VER_MAJ >= 2) && (BINUTILS_VER_MIN >= 28)
> > + disassemble = disassembler(bfd_get_arch(bfdf), bfd_big_endian(bfdf),
> > + bfd_get_mach(bfdf), bfdf);
> > +#else
> > disassemble = disassembler(bfdf);
> > +#endif
> > assert(disassemble);
> >
> > if (json_output)
>
> I discussed this issue with Jakub recently, and one suggestion he had
> was to look in tools/build/feature to add a new "feature", by trying to
> compile short programs, for making the distinction between binutils
> versions. It probably requires more work, but could be more robust than
> parsing the version from the command line?
Hm, might be an option. Parsing readelf output is pretty ugly, here I agree.
In general it feels more like a binutils issue, so we have to workaround it
in either way.
Is Jakub or someone else working on it?
Thanks!
