BPF List
 help / color / mirror / Atom feed
* [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
@ 2024-12-02 23:47 Eduard Zingerman
  2024-12-03  0:23 ` Stanislav Fomichev
  0 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2024-12-02 23:47 UTC (permalink / raw)
  To: bpf, ast
  Cc: andrii, daniel, martin.lau, kernel-team, yonghong.song, masahiroy,
	Eduard Zingerman

Commit [0] breaks samples/bpf build:

    $ make M=samples/bpf
    ...
    make -C /path/to/kernel/samples/bpf/../../tools/lib/bpf \
     ...
     EXTRA_CFLAGS=" \
     ...
     -fsanitize=bounds \
     -I/path/to/kernel/usr/include \
     ...
    	/path/to/kernel/samples/bpf/libbpf/libbpf.a install_headers
      CC      /path/to/kernel/samples/bpf/libbpf/staticobjs/libbpf.o
    In file included from libbpf.c:29:
    /path/to/kernel/tools/include/linux/err.h:35:8: error: 'inline' can only appear on functions
       35 | static inline void * __must_check ERR_PTR(long error_)
          |        ^

The error is caused by `objtree` variable changing definition from `.`
(dot) to an absolute path:
- The variable TPROGS_CFLAGS is constructed as follows:
  ...
  TPROGS_CFLAGS += -I$(objtree)/usr/include
- It is passed as EXTRA_CFLAGS for libbpf compilation:
  $(LIBBPF): ...
    ...
	$(MAKE) -C $(LIBBPF_SRC) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)"
- Before commit [0], the line passed to libbpf makefile was
  '-I./usr/include', where '.' referred to LIBBPF_SRC due to -C flag.
  The directory $(LIBBPF_SRC)/usr/include does not exist and thus
  was never resolved by C compiler.
- After commit [0], the line passed to libbpf makefile became:
  '<output-dir>/usr/include', this directory exists and is resolved by
  C compiler.
- Both 'tools/include' and 'usr/include' define files err.h and types.h.
- libbpf expects headers like 'linux/err.h' and 'linux/types.h'
  defined in 'tools/include', not 'usr/include', hence the compilation
  error.

This commit removes unnecessary -I flags from libbpf compilation.
(libbpf sets up the necessary includes at lib/bpf/Makefile:63).

Changes v1 [1] -> v2:
- dropped unnecessary replacement of KBUILD_OUTPUT with $(objtree)

[0] commit 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")
[1] https://lore.kernel.org/bpf/20241202212154.3174402-1-eddyz87@gmail.com/

Fixes: 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 samples/bpf/Makefile | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index bcf103a4c14f..44f7e05973de 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)
@@ -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
-- 
2.47.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
  2024-12-02 23:47 [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS Eduard Zingerman
@ 2024-12-03  0:23 ` Stanislav Fomichev
  2024-12-03  0:52   ` Eduard Zingerman
  0 siblings, 1 reply; 6+ messages in thread
From: Stanislav Fomichev @ 2024-12-03  0:23 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
	masahiroy

On 12/02, Eduard Zingerman wrote:
> Commit [0] breaks samples/bpf build:
> 
>     $ make M=samples/bpf
>     ...
>     make -C /path/to/kernel/samples/bpf/../../tools/lib/bpf \
>      ...
>      EXTRA_CFLAGS=" \
>      ...
>      -fsanitize=bounds \
>      -I/path/to/kernel/usr/include \
>      ...
>     	/path/to/kernel/samples/bpf/libbpf/libbpf.a install_headers
>       CC      /path/to/kernel/samples/bpf/libbpf/staticobjs/libbpf.o
>     In file included from libbpf.c:29:
>     /path/to/kernel/tools/include/linux/err.h:35:8: error: 'inline' can only appear on functions
>        35 | static inline void * __must_check ERR_PTR(long error_)
>           |        ^
> 
> The error is caused by `objtree` variable changing definition from `.`
> (dot) to an absolute path:
> - The variable TPROGS_CFLAGS is constructed as follows:
>   ...
>   TPROGS_CFLAGS += -I$(objtree)/usr/include
> - It is passed as EXTRA_CFLAGS for libbpf compilation:
>   $(LIBBPF): ...
>     ...
> 	$(MAKE) -C $(LIBBPF_SRC) RM='rm -rf' EXTRA_CFLAGS="$(TPROGS_CFLAGS)"
> - Before commit [0], the line passed to libbpf makefile was
>   '-I./usr/include', where '.' referred to LIBBPF_SRC due to -C flag.
>   The directory $(LIBBPF_SRC)/usr/include does not exist and thus
>   was never resolved by C compiler.
> - After commit [0], the line passed to libbpf makefile became:
>   '<output-dir>/usr/include', this directory exists and is resolved by
>   C compiler.
> - Both 'tools/include' and 'usr/include' define files err.h and types.h.
> - libbpf expects headers like 'linux/err.h' and 'linux/types.h'
>   defined in 'tools/include', not 'usr/include', hence the compilation
>   error.
> 
> This commit removes unnecessary -I flags from libbpf compilation.
> (libbpf sets up the necessary includes at lib/bpf/Makefile:63).
> 
> Changes v1 [1] -> v2:
> - dropped unnecessary replacement of KBUILD_OUTPUT with $(objtree)
> 
> [0] commit 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")
> [1] https://lore.kernel.org/bpf/20241202212154.3174402-1-eddyz87@gmail.com/
> 
> Fixes: 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")
> 
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  samples/bpf/Makefile | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index bcf103a4c14f..44f7e05973de 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)
> @@ -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
> -- 
> 2.47.0
> 

Naive question: why pass EXTRA_CFLAGS to libbpf at all? Can we drop it?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
  2024-12-03  0:23 ` Stanislav Fomichev
@ 2024-12-03  0:52   ` Eduard Zingerman
  2024-12-03  1:44     ` Eduard Zingerman
  0 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2024-12-03  0:52 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
	masahiroy

On Mon, 2024-12-02 at 16:23 -0800, Stanislav Fomichev wrote:

[...[

> > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> > index bcf103a4c14f..44f7e05973de 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)
> > @@ -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
> > -- 
> > 2.47.0
> > 
> 
> 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")


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
  2024-12-03  0:52   ` Eduard Zingerman
@ 2024-12-03  1:44     ` Eduard Zingerman
  2024-12-03  4:50       ` Eduard Zingerman
  0 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2024-12-03  1:44 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
	masahiroy

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
  2024-12-03  1:44     ` Eduard Zingerman
@ 2024-12-03  4:50       ` Eduard Zingerman
  2024-12-03  5:00         ` Stanislav Fomichev
  0 siblings, 1 reply; 6+ messages in thread
From: Eduard Zingerman @ 2024-12-03  4:50 UTC (permalink / raw)
  To: Stanislav Fomichev
  Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
	masahiroy

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
  2024-12-03  4:50       ` Eduard Zingerman
@ 2024-12-03  5:00         ` Stanislav Fomichev
  0 siblings, 0 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2024-12-03  5:00 UTC (permalink / raw)
  To: Eduard Zingerman
  Cc: bpf, ast, andrii, daniel, martin.lau, kernel-team, yonghong.song,
	masahiroy

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-12-03  5:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-02 23:47 [PATCH bpf v2] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS Eduard Zingerman
2024-12-03  0:23 ` Stanislav Fomichev
2024-12-03  0:52   ` Eduard Zingerman
2024-12-03  1:44     ` Eduard Zingerman
2024-12-03  4:50       ` Eduard Zingerman
2024-12-03  5:00         ` Stanislav Fomichev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox