2017-12-19 16:10 UTC+0000 ~ Roman Gushchin <g...@fb.com>
> 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!
>
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
