Date: Wed, 27 Dec 2017 19:04:24 +0000
From: Roman Gushchin <guro@fb.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
CC: Quentin Monnet <quentin.monnet@netronome.com>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <kernel-team@fb.com>, Jakub Kicinski <jakub.kicinski@netronome.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH v2 bpf-next 2/2] tools/bpftool: fix bpftool build with
 bintutils >= 2.8
Message-ID: <20171227190417.GA2361@castle>
References: <20171222161152.24715-1-guro@fb.com>
 <20171222161152.24715-2-guro@fb.com>
 <bc74e33b-0dbc-f12a-bae8-180dd41f007b@netronome.com>
 <20171227023204.eulgkbg7epj7nl76@ast-mbp>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20171227023204.eulgkbg7epj7nl76@ast-mbp>
User-Agent: Mutt/1.9.1 (2017-09-22)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Dec 2017 19:04:34.0775 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: b49aac9c-ff24-4989-5869-08d54d5cabc6
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR15MB1075
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-27_12:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3779
Lines: 87

On Tue, Dec 26, 2017 at 06:32:05PM -0800, Alexei Starovoitov wrote:
> 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?

Yes, you're right. Thanks!

> > 
> > > 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.

Agree, we can enhance it later.

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

Sure, will send soon-ish.

Thanks!