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

[Quentin Monnet]

2017-12-19 16:10 UTC+0000 ~ Roman Gushchin <guro@xxxxxx>
> 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!
> 

Jakub isn't. On our side, I noticed last week that there was this change
in binutils, and started to have a look at how these "features" work.
But I have nothing that works so far, so feel free to tackle this.

Quentin