Re: [PATCH bpf-next] bpftool: Clean up HOST_CFLAGS, HOST_LDFLAGS for bootstrap bpftool

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Mar 20, 2024 at 01:41:03AM +0000, Quentin Monnet wrote:
> Bpftool's Makefile uses $(HOST_CFLAGS) to build the bootstrap version of
> bpftool, in order to pick the flags for the host (where we run the
> bootstrap version) and not for the target system (where we plan to run
> the full bpftool binary). But we pass too much information through this
> variable.
> 
> In particular, we set HOST_CFLAGS by copying most of the $(CFLAGS); but
> we do this after the feature detection for bpftool, which means that
> $(CFLAGS), hence $(HOST_CFLAGS), contain all macro definitions for using
> the different optional features. For example, -DHAVE_LLVM_SUPPORT may be
> passed to the $(HOST_CFLAGS), even though the LLVM disassembler is not
> used in the bootstrap version, and the related library may even be
> missing for the host architecture.
> 
> A similar thing happens with the $(LDFLAGS), that we use unchanged for
> linking the bootstrap version even though they may contains flags to
> link against additional libraries.
> 
> To address the $(HOST_CFLAGS) issue, we move the definition of
> $(HOST_CFLAGS) earlier in the Makefile, before the $(CFLAGS) update
> resulting from the feature probing - none of which being relevant to the
> bootstrap version. To clean up the $(LDFLAGS) for the bootstrap version,
> we introduce a dedicated $(HOST_LDFLAGS) variable that we base on
> $(LDFLAGS), before the feature probing as well.
> 
> On my setup, the following macro and libraries are removed from the
> compiler invocation to build bpftool after this patch:
> 
>   -DUSE_LIBCAP
>   -DHAVE_LLVM_SUPPORT
>   -I/usr/lib/llvm-17/include
>   -D_GNU_SOURCE
>   -D__STDC_CONSTANT_MACROS
>   -D__STDC_FORMAT_MACROS
>   -D__STDC_LIMIT_MACROS
>   -lLLVM-17
>   -L/usr/lib/llvm-17/lib
> 
> Another advantage of cleaning up these flags is that displaying
> available features with "bpftool version" becomes more accurate for the
> bootstrap bpftool, and no longer reflects the features detected (and
> available only) for the final binary.
> 
> Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Signed-off-by: Quentin Monnet <qmo@kernel.org>

got some fuzz when applying it, but other than that lgtm

Acked-by: Jiri Olsa <jolsa@kernel.org>

jirka

> ---
>  tools/bpf/bpftool/Makefile | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
> index e9154ace80ff..972f8d727130 100644
> --- a/tools/bpf/bpftool/Makefile
> +++ b/tools/bpf/bpftool/Makefile
> @@ -89,6 +89,10 @@ ifneq ($(EXTRA_LDFLAGS),)
>  LDFLAGS += $(EXTRA_LDFLAGS)
>  endif
>  
> +HOST_CFLAGS := $(subst -I$(LIBBPF_INCLUDE),-I$(LIBBPF_BOOTSTRAP_INCLUDE),\
> +		$(subst $(CLANG_CROSS_FLAGS),,$(CFLAGS)))
> +HOST_LDFLAGS := $(LDFLAGS)
> +
>  INSTALL ?= install
>  RM ?= rm -f
>  
> @@ -178,9 +182,6 @@ ifeq ($(filter -DHAVE_LLVM_SUPPORT -DHAVE_LIBBFD_SUPPORT,$(CFLAGS)),)
>    SRCS := $(filter-out jit_disasm.c,$(SRCS))
>  endif
>  
> -HOST_CFLAGS = $(subst -I$(LIBBPF_INCLUDE),-I$(LIBBPF_BOOTSTRAP_INCLUDE),\
> -		$(subst $(CLANG_CROSS_FLAGS),,$(CFLAGS)))
> -
>  BPFTOOL_BOOTSTRAP := $(BOOTSTRAP_OUTPUT)bpftool
>  
>  BOOTSTRAP_OBJS = $(addprefix $(BOOTSTRAP_OUTPUT),main.o common.o json_writer.o gen.o btf.o xlated_dumper.o btf_dumper.o disasm.o)
> @@ -238,7 +239,7 @@ $(OUTPUT)disasm.o: $(srctree)/kernel/bpf/disasm.c
>  	$(QUIET_CC)$(CC) $(CFLAGS) -c -MMD $< -o $@
>  
>  $(BPFTOOL_BOOTSTRAP): $(BOOTSTRAP_OBJS) $(LIBBPF_BOOTSTRAP)
> -	$(QUIET_LINK)$(HOSTCC) $(HOST_CFLAGS) $(LDFLAGS) $(BOOTSTRAP_OBJS) $(LIBS_BOOTSTRAP) -o $@
> +	$(QUIET_LINK)$(HOSTCC) $(HOST_CFLAGS) $(HOST_LDFLAGS) $(BOOTSTRAP_OBJS) $(LIBS_BOOTSTRAP) -o $@
>  
>  $(OUTPUT)bpftool: $(OBJS) $(LIBBPF)
>  	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $(OBJS) $(LIBS) -o $@
> -- 
> 2.34.1
>