lkml.org 
[lkml]   [2017]   [Dec]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2 bpf-next 2/2] tools/bpftool: fix bpftool build with bintutils >= 2.8
    On Fri, Dec 22, 2017 at 06:50:01PM +0000, Quentin Monnet wrote:
    > Hi Roman,
    >
    > 2017-12-22 16:11 UTC+0000 ~ Roman Gushchin <guro@fb.com>
    > > Bpftool build is broken with binutils version 2.28 and later.
    >
    > Could you check the binutils version? I believe it changed in 2.29
    > instead of 2.28. Could you update your commit log and subject
    > accordingly, please?
    >
    > > The cause is commit 003ca0fd2286 ("Refactor disassembler selection")
    > > in the binutils repo, which changed the disassembler() function
    > > signature.
    > >
    > > Fix this by adding a new "feature" to the tools/build/features
    > > infrastructure and make it responsible for decision which
    > > disassembler() function signature to use.
    > >
    > > Signed-off-by: Roman Gushchin <guro@fb.com>
    > > Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
    > > Cc: Alexei Starovoitov <ast@kernel.org>
    > > Cc: Daniel Borkmann <daniel@iogearbox.net>
    > > ---
    > > tools/bpf/Makefile | 29 +++++++++++++++++++++++
    > > tools/bpf/bpf_jit_disasm.c | 7 ++++++
    > > tools/bpf/bpftool/Makefile | 24 +++++++++++++++++++
    > > tools/bpf/bpftool/jit_disasm.c | 7 ++++++
    > > tools/build/feature/Makefile | 4 ++++
    > > tools/build/feature/test-disassembler-four-args.c | 15 ++++++++++++
    > > 6 files changed, 86 insertions(+)
    > > create mode 100644 tools/build/feature/test-disassembler-four-args.c
    > >
    > > diff --git a/tools/bpf/Makefile b/tools/bpf/Makefile
    > > index 07a6697466ef..c8ec0ae16bf0 100644
    > > --- a/tools/bpf/Makefile
    > > +++ b/tools/bpf/Makefile
    > > @@ -9,6 +9,35 @@ MAKE = make
    > > CFLAGS += -Wall -O2
    > > CFLAGS += -D__EXPORTED_HEADERS__ -I../../include/uapi -I../../include
    > >
    > > +ifeq ($(srctree),)
    > > +srctree := $(patsubst %/,%,$(dir $(CURDIR)))
    > > +srctree := $(patsubst %/,%,$(dir $(srctree)))
    > > +endif
    > > +
    > > +FEATURE_USER = .bpf
    > > +FEATURE_TESTS = libbfd disassembler-four-args
    > > +FEATURE_DISPLAY = libbfd disassembler-four-args
    >
    > Thanks for adding libbfd as I requested. However, you do not use it in
    > the Makefile to prevent compilation if the feature is not detected (see
    > "bpfdep" or "elfdep" in tools/lib/bpf/Makefile. Sorry, I should have
    > pointed it in my previous review.
    >
    > But actually, I have another issue related to the libbfd feature: since
    > commit 280e7c48c3b8 ("perf tools: fix BFD detection on opensuse") it
    > requires libiberty so that libbfd is correctly detected, but libiberty
    > is not needed on all distros (at least Ubuntu can have libbfd without
    > libiberty). Typically, detection fails on my setup, although I do have
    > libbfd installed. So forcing libbfd feature here may eventually force
    > users to install libraries they do not need to compile bpftool, which is
    > not what we want.
    >
    > I do not have a clean work around to suggest. Maybe have one
    > "libbfd-something" feature that tries to compile without libiberty, then
    > another one that tries with it, and compile the tools if at least one of
    > them succeeds. But it's probably for another patch series. In the
    > meantime, would you please simply remove libbfd detection here and
    > accept my apologies for suggesting to add it in the previous review?

    I think since libbfd is already used by bpftool it's a good thing
    to add feature detection. Even if it's not perfect on some setups.

    Roman,
    I think you still need to do one more respin to address commit log nit?

    \
     
     \ /
      Last update: 2017-12-27 03:33    [W:4.766 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site