[bpf] tools/bpf: fix bpftool build with OUTPUT set
diff mbox series

Message ID 20190718142041.83342-1-iii@linux.ibm.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • [bpf] tools/bpf: fix bpftool build with OUTPUT set
Related show

Commit Message

Ilya Leoshkevich July 18, 2019, 2:20 p.m. UTC
Hi Lorenz,

I've been using the following patch for quite some time now.
Please let me know if it works for you.

Best regards,
Ilya

---

When OUTPUT is set, bpftool and libbpf put their objects into the same
directory, and since some of them have the same names, the collision
happens.

Fix by invoking libbpf build in a manner similar to $(call descend) -
descend itself cannot be used, since libbpf is a sibling, and not a
child, of bpftool.

Also, don't link bpftool with libbpf.a twice.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/bpf/bpftool/Makefile | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Comments

Jakub Kicinski July 18, 2019, 6:51 p.m. UTC | #1
On Thu, 18 Jul 2019 16:20:41 +0200, Ilya Leoshkevich wrote:
> Hi Lorenz,
> 
> I've been using the following patch for quite some time now.
> Please let me know if it works for you.
> 
> Best regards,
> Ilya
> 
> ---
> 
> When OUTPUT is set, bpftool and libbpf put their objects into the same
> directory, and since some of them have the same names, the collision
> happens.
> 
> Fix by invoking libbpf build in a manner similar to $(call descend) -
> descend itself cannot be used, since libbpf is a sibling, and not a
> child, of bpftool.
> 
> Also, don't link bpftool with libbpf.a twice.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/bpf/bpftool/Makefile | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index a7afea4dec47..2cbc3c166f44 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -15,23 +15,18 @@ else
>  endif
>  
>  BPF_DIR = $(srctree)/tools/lib/bpf/
> -
> -ifneq ($(OUTPUT),)
> -  BPF_PATH = $(OUTPUT)
> -else
> -  BPF_PATH = $(BPF_DIR)
> -endif
> -
> -LIBBPF = $(BPF_PATH)libbpf.a
> +BPF_PATH = $(objtree)/tools/lib/bpf

objtree won't be set for simple make in the directory. Perhaps we
should stick to using OUTPUT and srctree?

We should probably make a script with all the ways of calling make
should work. Otherwise we can lose track too easily.

# thru kbuild
make tools/bpf

T=$(mktemp -d)
make tools/bpf OUTPUT=$T
rm -rf $T

# from kernel source tree
make -C tools/bpf/bpftool

T=$(mktemp -d)
make -C tools/bpf/bpftool OUTPUT=$T
rm -rf $T

# from tools
cd tools/
make bpf

T=$(mktemp -d)
make bpf OUTPUT=$T
rm -rf $T

# from bpftool's dir
cd bpf/bpftool
make

T=$(mktemp -d)
make OUTPUT=$T
rm -rf $T

.. add your own.

> +LIBBPF = $(BPF_PATH)/libbpf.a
>  
>  BPFTOOL_VERSION := $(shell make --no-print-directory -sC ../../.. kernelversion)
>  
>  $(LIBBPF): FORCE
> -	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) $(OUTPUT)libbpf.a
> +	$(Q)mkdir -p $(BPF_PATH)
> +	$(Q)$(MAKE) $(COMMAND_O) subdir=tools/lib/bpf -C $(BPF_DIR) $(LIBBPF)
>  
>  $(LIBBPF)-clean:
>  	$(call QUIET_CLEAN, libbpf)
> -	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) clean >/dev/null
> +	$(Q)$(MAKE) $(COMMAND_O) subdir=tools/lib/bpf -C $(BPF_DIR) clean >/dev/null
>  
>  prefix ?= /usr/local
>  bash_compdir ?= /usr/share/bash-completion/completions
> @@ -112,7 +107,7 @@ $(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
>  	$(QUIET_CC)$(COMPILE.c) -MMD -o $@ $<
>  
>  $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
> -	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $^ $(LIBS)
> +	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(OBJS) $(LIBS)
>  
>  $(OUTPUT)%.o: %.c
>  	$(QUIET_CC)$(COMPILE.c) -MMD -o $@ $<
Ilya Leoshkevich July 19, 2019, 1:12 p.m. UTC | #2
> Am 18.07.2019 um 20:51 schrieb Jakub Kicinski <jakub.kicinski@netronome.com>:
> 
> We should probably make a script with all the ways of calling make
> should work. Otherwise we can lose track too easily.

Thanks for the script!

I’m trying to make it all pass now, and hitting a weird issue in the
Kbuild case. The build prints "No rule to make target
'scripts/Makefile.ubsan.o'" and proceeds with an empty BPFTOOL_VERSION,
which causes problems later on.

I've found that this is caused by sub_make_done=1 environment variable,
and unsetting it indeed fixes the problem, since the root Makefile no
longer uses the implicit %.o rule.

However, I wonder if that would be acceptable in the final version of
the patch, and whether there is a cleaner way to achieve the same
effect?

Best regards,
Ilya
Jakub Kicinski July 19, 2019, 6:17 p.m. UTC | #3
On Fri, 19 Jul 2019 15:12:24 +0200, Ilya Leoshkevich wrote:
> > Am 18.07.2019 um 20:51 schrieb Jakub Kicinski <jakub.kicinski@netronome.com>:
> > 
> > We should probably make a script with all the ways of calling make
> > should work. Otherwise we can lose track too easily.  
> 
> Thanks for the script!
> 
> I’m trying to make it all pass now, and hitting a weird issue in the
> Kbuild case. The build prints "No rule to make target
> 'scripts/Makefile.ubsan.o'" and proceeds with an empty BPFTOOL_VERSION,
> which causes problems later on.

Does it only break with UBSAN enabled?

> I've found that this is caused by sub_make_done=1 environment variable,
> and unsetting it indeed fixes the problem, since the root Makefile no
> longer uses the implicit %.o rule.
> 
> However, I wonder if that would be acceptable in the final version of
> the patch, and whether there is a cleaner way to achieve the same
> effect?

I'm not sure to be honest. Did you check how perf deals with that?

My goal was primarily to make sure we don't regress, so maybe if some
corner cases don't work that's not the end of the world. I think a good
rule of the thumb would be "if it works for perf it should work for
bpftool" ;) Perf gets a lot more build testing.
Lorenz Bauer July 23, 2019, 12:59 p.m. UTC | #4
Hi Ilya,

Thanks for your patch! I tried it but had problems with
cross-compilation. Not sure if this is related to the patch or not
though, I haven't had the time to follow up.

Best
Lorenz

On Thu, 18 Jul 2019 at 15:20, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Hi Lorenz,
>
> I've been using the following patch for quite some time now.
> Please let me know if it works for you.
>
> Best regards,
> Ilya
>
> ---
>
> When OUTPUT is set, bpftool and libbpf put their objects into the same
> directory, and since some of them have the same names, the collision
> happens.
>
> Fix by invoking libbpf build in a manner similar to $(call descend) -
> descend itself cannot be used, since libbpf is a sibling, and not a
> child, of bpftool.
>
> Also, don't link bpftool with libbpf.a twice.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tools/bpf/bpftool/Makefile | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index a7afea4dec47..2cbc3c166f44 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -15,23 +15,18 @@ else
>  endif
>
>  BPF_DIR = $(srctree)/tools/lib/bpf/
> -
> -ifneq ($(OUTPUT),)
> -  BPF_PATH = $(OUTPUT)
> -else
> -  BPF_PATH = $(BPF_DIR)
> -endif
> -
> -LIBBPF = $(BPF_PATH)libbpf.a
> +BPF_PATH = $(objtree)/tools/lib/bpf
> +LIBBPF = $(BPF_PATH)/libbpf.a
>
>  BPFTOOL_VERSION := $(shell make --no-print-directory -sC ../../.. kernelversion)
>
>  $(LIBBPF): FORCE
> -       $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) $(OUTPUT)libbpf.a
> +       $(Q)mkdir -p $(BPF_PATH)
> +       $(Q)$(MAKE) $(COMMAND_O) subdir=tools/lib/bpf -C $(BPF_DIR) $(LIBBPF)
>
>  $(LIBBPF)-clean:
>         $(call QUIET_CLEAN, libbpf)
> -       $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) clean >/dev/null
> +       $(Q)$(MAKE) $(COMMAND_O) subdir=tools/lib/bpf -C $(BPF_DIR) clean >/dev/null
>
>  prefix ?= /usr/local
>  bash_compdir ?= /usr/share/bash-completion/completions
> @@ -112,7 +107,7 @@ $(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
>         $(QUIET_CC)$(COMPILE.c) -MMD -o $@ $<
>
>  $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
> -       $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $^ $(LIBS)
> +       $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(OBJS) $(LIBS)
>
>  $(OUTPUT)%.o: %.c
>         $(QUIET_CC)$(COMPILE.c) -MMD -o $@ $<
> --
> 2.21.0
>
Ilya Leoshkevich July 23, 2019, 3:14 p.m. UTC | #5
> Am 19.07.2019 um 20:17 schrieb Jakub Kicinski <jakub.kicinski@netronome.com>:
> 
> On Fri, 19 Jul 2019 15:12:24 +0200, Ilya Leoshkevich wrote:
>>> Am 18.07.2019 um 20:51 schrieb Jakub Kicinski <jakub.kicinski@netronome.com>:
>>> 
>>> We should probably make a script with all the ways of calling make
>>> should work. Otherwise we can lose track too easily.  
>> 
>> Thanks for the script!
>> 
>> I’m trying to make it all pass now, and hitting a weird issue in the
>> Kbuild case. The build prints "No rule to make target
>> 'scripts/Makefile.ubsan.o'" and proceeds with an empty BPFTOOL_VERSION,
>> which causes problems later on.
> 
> Does it only break with UBSAN enabled?

No, all the time. I think this is a coincidence - make happens to scan
scripts/Makefile.ubsan first.

> 
>> I've found that this is caused by sub_make_done=1 environment variable,
>> and unsetting it indeed fixes the problem, since the root Makefile no
>> longer uses the implicit %.o rule.
>> 
>> However, I wonder if that would be acceptable in the final version of
>> the patch, and whether there is a cleaner way to achieve the same
>> effect?
> 
> I'm not sure to be honest. Did you check how perf deals with that?

perf obtains the version using "git describe". However, if we are
building it from a tarball, it falls back to "make kernelversion" and
fails in a similar way:

linux-5.3-rc1$ make defconfig
linux-5.3-rc1$ make tools/perf
<snip>
make[6]: Circular scripts/Makefile.ubsan.mod <- scripts/Makefile.ubsan.o dependency dropped.
make[6]: m2c: Command not found
make[6]: *** [<builtin>: scripts/Makefile.ubsan.o] Error 127
make[5]: *** [Makefile:1765: scripts/Makefile.ubsan.o] Error 2
<snip>

The same trick helps:

--- tools/perf/util/PERF-VERSION-GEN.orig	2019-07-23 17:12:07.621123187 +0200
+++ tools/perf/util/PERF-VERSION-GEN	2019-07-23 17:12:33.441133619 +0200
@@ -26,7 +26,7 @@
 fi
 if test -z "$TAG"
 then
-	TAG=$(MAKEFLAGS= make -sC ../.. kernelversion)
+	TAG=$(MAKEFLAGS= sub_make_done= make -sC ../.. kernelversion)
 fi
 VN="$TAG$CID"
 if test -n "$CID"

Patch
diff mbox series

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index a7afea4dec47..2cbc3c166f44 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -15,23 +15,18 @@  else
 endif
 
 BPF_DIR = $(srctree)/tools/lib/bpf/
-
-ifneq ($(OUTPUT),)
-  BPF_PATH = $(OUTPUT)
-else
-  BPF_PATH = $(BPF_DIR)
-endif
-
-LIBBPF = $(BPF_PATH)libbpf.a
+BPF_PATH = $(objtree)/tools/lib/bpf
+LIBBPF = $(BPF_PATH)/libbpf.a
 
 BPFTOOL_VERSION := $(shell make --no-print-directory -sC ../../.. kernelversion)
 
 $(LIBBPF): FORCE
-	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) $(OUTPUT)libbpf.a
+	$(Q)mkdir -p $(BPF_PATH)
+	$(Q)$(MAKE) $(COMMAND_O) subdir=tools/lib/bpf -C $(BPF_DIR) $(LIBBPF)
 
 $(LIBBPF)-clean:
 	$(call QUIET_CLEAN, libbpf)
-	$(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(OUTPUT) clean >/dev/null
+	$(Q)$(MAKE) $(COMMAND_O) subdir=tools/lib/bpf -C $(BPF_DIR) clean >/dev/null
 
 prefix ?= /usr/local
 bash_compdir ?= /usr/share/bash-completion/completions
@@ -112,7 +107,7 @@  $(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
 	$(QUIET_CC)$(COMPILE.c) -MMD -o $@ $<
 
 $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
-	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $^ $(LIBS)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ $(OBJS) $(LIBS)
 
 $(OUTPUT)%.o: %.c
 	$(QUIET_CC)$(COMPILE.c) -MMD -o $@ $<