On Thu, Oct 7, 2021 at 12:44 PM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> Bpftool relies on libbpf, therefore it relies on a number of headers
> from the library and must be linked against the library. The Makefile
> for bpftool exposes these objects by adding tools/lib as an include
> directory ("-I$(srctree)/tools/lib"). This is a working solution, but
> this is not the cleanest one. The risk is to involuntarily include
> objects that are not intended to be exposed by the libbpf.
>
> The headers needed to compile bpftool should in fact be "installed" from
> libbpf, with its "install_headers" Makefile target. In addition, there
> is one header which is internal to the library and not supposed to be
> used by external applications, but that bpftool uses anyway.
>
> Adjust the Makefile in order to install the header files properly before
> compiling bpftool. Also copy the additional internal header file
> (nlattr.h), but call it out explicitly. Build (and install headers) in a
> subdirectory under bpftool/ instead of tools/lib/bpf/. When descending
> from a parent Makefile, this is configurable by setting the OUTPUT,
> LIBBPF_OUTPUT and LIBBPF_DESTDIR variables.
>
> Also adjust the Makefile for BPF selftests, so as to reuse the (host)
> libbpf compiled earlier and to avoid compiling a separate version of the
> library just for bpftool.
>
> Signed-off-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> tools/bpf/bpftool/Makefile | 33 ++++++++++++++++++----------
> tools/testing/selftests/bpf/Makefile | 2 ++
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index 1fcf5b01a193..ba02d71c39ef 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -17,19 +17,23 @@ endif
> BPF_DIR = $(srctree)/tools/lib/bpf/
>
> ifneq ($(OUTPUT),)
> - LIBBPF_OUTPUT = $(OUTPUT)/libbpf/
> - LIBBPF_PATH = $(LIBBPF_OUTPUT)
> - BOOTSTRAP_OUTPUT = $(OUTPUT)/bootstrap/
> + _OUTPUT := $(OUTPUT)
> else
> - LIBBPF_OUTPUT =
> - LIBBPF_PATH = $(BPF_DIR)
> - BOOTSTRAP_OUTPUT = $(CURDIR)/bootstrap/
> + _OUTPUT := $(CURDIR)
> endif
> +BOOTSTRAP_OUTPUT := $(_OUTPUT)/bootstrap/
> +LIBBPF_OUTPUT := $(_OUTPUT)/libbpf/
> +LIBBPF_DESTDIR := $(LIBBPF_OUTPUT)
> +LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)/include
>
> -LIBBPF = $(LIBBPF_PATH)libbpf.a
> +LIBBPF = $(LIBBPF_OUTPUT)libbpf.a
> LIBBPF_BOOTSTRAP_OUTPUT = $(BOOTSTRAP_OUTPUT)libbpf/
> LIBBPF_BOOTSTRAP = $(LIBBPF_BOOTSTRAP_OUTPUT)libbpf.a
>
> +# We need to copy nlattr.h which is not otherwise exported by libbpf, but still
> +# required by bpftool.
> +LIBBPF_INTERNAL_HDRS := nlattr.h
> +
> ifeq ($(BPFTOOL_VERSION),)
> BPFTOOL_VERSION := $(shell make -rR --no-print-directory -sC ../../.. kernelversion)
> endif
> @@ -38,7 +42,13 @@ $(LIBBPF_OUTPUT) $(BOOTSTRAP_OUTPUT) $(LIBBPF_BOOTSTRAP_OUTPUT):
> $(QUIET_MKDIR)mkdir -p $@
>
> $(LIBBPF): FORCE | $(LIBBPF_OUTPUT)
> - $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) $(LIBBPF_OUTPUT)libbpf.a
> + $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_OUTPUT) \
> + DESTDIR=$(LIBBPF_DESTDIR) prefix= $(LIBBPF) install_headers
> +
> +$(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS): \
This worked only because LIBBPF_INTERNAL_HDRS is a single element list
right now. I didn't touch it for now, but please follow up with a
proper fix (you'd need to do % magic here)
> + $(addprefix $(BPF_DIR),$(LIBBPF_INTERNAL_HDRS)) $(LIBBPF)
> + $(call QUIET_INSTALL, bpf/$(notdir $@))
> + $(Q)install -m 644 -t $(LIBBPF_INCLUDE)/bpf/ $(BPF_DIR)$(notdir $@)
>
> $(LIBBPF_BOOTSTRAP): FORCE | $(LIBBPF_BOOTSTRAP_OUTPUT)
> $(Q)$(MAKE) -C $(BPF_DIR) OUTPUT=$(LIBBPF_BOOTSTRAP_OUTPUT) \
> @@ -60,10 +70,10 @@ CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
> CFLAGS += $(filter-out -Wswitch-enum -Wnested-externs,$(EXTRA_WARNINGS))
> CFLAGS += -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ \
> -I$(if $(OUTPUT),$(OUTPUT),.) \
> + -I$(LIBBPF_INCLUDE) \
> -I$(srctree)/kernel/bpf/ \
> -I$(srctree)/tools/include \
> -I$(srctree)/tools/include/uapi \
> - -I$(srctree)/tools/lib \
> -I$(srctree)/tools/perf
> CFLAGS += -DBPFTOOL_VERSION='"$(BPFTOOL_VERSION)"'
> ifneq ($(EXTRA_CFLAGS),)
> @@ -140,7 +150,7 @@ BOOTSTRAP_OBJS = $(addprefix $(BOOTSTRAP_OUTPUT),main.o common.o json_writer.o g
> $(BOOTSTRAP_OBJS): $(LIBBPF_BOOTSTRAP)
>
> OBJS = $(patsubst %.c,$(OUTPUT)%.o,$(SRCS)) $(OUTPUT)disasm.o
> -$(OBJS): $(LIBBPF)
> +$(OBJS): $(LIBBPF) $(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS)
the whole $(LIBBPF_INCLUDE)/bpf/$(LIBBPF_INTERNAL_HDRS) use in
multiple places might benefit from having a dedicated variable
>
> VMLINUX_BTF_PATHS ?= $(if $(O),$(O)/vmlinux) \
> $(if $(KBUILD_OUTPUT),$(KBUILD_OUTPUT)/vmlinux) \
> @@ -167,8 +177,7 @@ $(OUTPUT)%.bpf.o: skeleton/%.bpf.c $(OUTPUT)vmlinux.h $(LIBBPF)
> $(QUIET_CLANG)$(CLANG) \
> -I$(if $(OUTPUT),$(OUTPUT),.) \
> -I$(srctree)/tools/include/uapi/ \
> - -I$(LIBBPF_PATH) \
> - -I$(srctree)/tools/lib \
> + -I$(LIBBPF_INCLUDE) \
> -g -O2 -Wall -target bpf -c $< -o $@ && $(LLVM_STRIP) -g $@
>
> $(OUTPUT)%.skel.h: $(OUTPUT)%.bpf.o $(BPFTOOL_BOOTSTRAP)
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index c5c9a9f50d8d..849a4637f59d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -209,6 +209,8 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile) \
> CC=$(HOSTCC) LD=$(HOSTLD) \
> EXTRA_CFLAGS='-g -O0' \
> OUTPUT=$(HOST_BUILD_DIR)/bpftool/ \
> + LIBBPF_OUTPUT=$(HOST_BUILD_DIR)/libbpf/ \
> + LIBBPF_DESTDIR=$(HOST_SCRATCH_DIR)/ \
> prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install
>
> all: docs
> --
> 2.30.2
>
