Re: [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS

[Stanislav Fomichev <stfomichev@gmail.com>]

On 12/02, Eduard Zingerman wrote:
> On Mon, 2024-12-02 at 17:44 -0800, Eduard Zingerman wrote:
> > On Mon, 2024-12-02 at 16:52 -0800, Eduard Zingerman wrote:
> > 
> > [...]
> > 
> > > > Naive question: why pass EXTRA_CFLAGS to libbpf at all? Can we drop it?
> > > 
> > > This was added by the commit [0].
> > > As far as I understand, the idea is to pass the following flags:
> > > 
> > >     ifeq ($(ARCH), arm)
> > >     # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
> > >     # headers when arm instruction set identification is requested.
> > >     ARM_ARCH_SELECTOR := $(filter -D__LINUX_ARM_ARCH__%, $(KBUILD_CFLAGS))
> > >     ...
> > >     TPROGS_CFLAGS += $(ARM_ARCH_SELECTOR)
> > >     endif
> > > 
> > >     ifeq ($(ARCH), mips)
> > >     TPROGS_CFLAGS += -D__SANE_USERSPACE_TYPES__
> > >     ...
> > >     endif
> > > 
> > > Not sure if these are still necessary.
> > > 
> > > [0] commit d8ceae91e9f0 ("samples/bpf: Provide C/LDFLAGS to libbpf")
> > > 
> > 
> > But this means that I should include sysroot part in the COMMON_CFLAGS.
> > I'll get the arm cross-compilation environment and double check.
> > 
> 
> So, I tested build as follows:
> - setup a debian chroot for 'testing';
> - added gcc-arm-linux-gnueabihf toolchain and dependencies necessary
>   for kernel build (as in [0]) + clang-18 + qemu-system-arm + qemu-user-static;
> - cross-compiled kernel for ARM inside that chroot:
>   $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make olddefconfig
>   $ ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- make -j
> - prepared an ARM sysroot (again, debian 'testing'):
>   $ debootstrap --arch armhf --variant=buildd testing \
>       /some/dir/trixie-armhf http://deb.debian.org/debian
>   (and installed libelf-dev inside chroot)
> - compiled samples with the following command:
>   $ CLANG=clang-18 LLC=llc-18 OPT=opt-18 LLVM_DIS=llvm-dis-18 \
>     LLVM_OBJCOPY=llvm-objcopy-18 LLVM_READELF=llvm-readelf-18 \
>     ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- \
>     SYSROOT=/some/dir/trixie-armhf/ \
>     make M=samples/bpf
> 
> [0] https://docs.kernel.org/bpf/s390.html
> 
> The compilation finishes successfully with and without EXTRA_CFLAGS
> passed to libbpf build. When EXTRA_CFLAGS are passed, I don't see any
> -D__LINUX_ARM_ARCH__% flags passed to libbpf build.
> 
> Still, I'm hesitant to remove this flag, I'd prefer to post a v3
> covering sysroot flag and be done with this. E.g. as below:
> 
> --- 8< ----------------------------------------------------
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index bcf103a4c14f..96a05e70ace3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -146,13 +146,14 @@ ifeq ($(ARCH), x86)
>  BPF_EXTRA_CFLAGS += -fcf-protection
>  endif
>  
> -TPROGS_CFLAGS += -Wall -O2
> -TPROGS_CFLAGS += -Wmissing-prototypes
> -TPROGS_CFLAGS += -Wstrict-prototypes
> -TPROGS_CFLAGS += $(call try-run,\
> +COMMON_CFLAGS += -Wall -O2
> +COMMON_CFLAGS += -Wmissing-prototypes
> +COMMON_CFLAGS += -Wstrict-prototypes
> +COMMON_CFLAGS += $(call try-run,\
>         printf "int main() { return 0; }" |\
>         $(CC) -Werror -fsanitize=bounds -x c - -o "$$TMP",-fsanitize=bounds,)
>  
> +TPROGS_CFLAGS += $(COMMON_CFLAGS)
>  TPROGS_CFLAGS += -I$(objtree)/usr/include
>  TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
>  TPROGS_CFLAGS += -I$(LIBBPF_INCLUDE)
> @@ -162,7 +163,7 @@ TPROGS_CFLAGS += -I$(srctree)/tools/lib
>  TPROGS_CFLAGS += -DHAVE_ATTR_TEST=0
>  
>  ifdef SYSROOT
> -TPROGS_CFLAGS += --sysroot=$(SYSROOT)
> +COMMON_CFLAGS += --sysroot=$(SYSROOT)
>  TPROGS_LDFLAGS := -L$(SYSROOT)/usr/lib
>  endif
>  
> @@ -229,7 +230,7 @@ clean:
>  
>  $(LIBBPF): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUTPUT)
>  # Fix up variables inherited from Kbuild that tools/ build system won't like
> -       $(MAKE) -C $(LIBBPF_SRC) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)" \
> +       $(MAKE) -C $(LIBBPF_SRC) RM='rm -rf' EXTRA_CFLAGS="$(COMMON_CFLAGS)" \
>                 LDFLAGS="$(TPROGS_LDFLAGS)" srctree=$(BPF_SAMPLES_PATH)/../../ \
>                 O= OUTPUT=$(LIBBPF_OUTPUT)/ DESTDIR=$(LIBBPF_DESTDIR) prefix= \
>                 $@ install_headers
> ---------------------------------------------------- >8 ---
> 
> (and maybe peek a better name for COMMON_CFLAGS).
> 

Agreed, let's go with what you have (especially since you've tested it).
The samples are mostly deprecated / in maintenance mode anyway.

Feel free to slap:
Acked-by: Stanislav Fomichev <sdf@fomichev.me>