Re: [RFC PATCH net-next] tools/bpf: fix build with binutils >= 2.28

[Roman Gushchin]

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 <guro@xxxxxx>
> > 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 <guro@xxxxxx>
> > Cc: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> > Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> > Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> > ---
> >  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!