diff mbox series

[bpf,v3] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS

Message ID 20241203182222.3915763-1-eddyz87@gmail.com (mailing list archive)
State Accepted
Commit 5a6ea7022ff4d2a65ae328619c586d6a8909b48b
Delegated to: BPF
Headers show
Series [bpf,v3] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS | expand

Checks

Context Check Description
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (sched_ext, false, 360) / sched_ext on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-31 success Logs for x86_64-llvm-17 / test (sched_ext, false, 360) / sched_ext on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-38 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-45 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-36 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-37 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-11 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-39 success Logs for x86_64-llvm-18 / test (sched_ext, false, 360) / sched_ext on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-27 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-43 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-44 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-35 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (sched_ext, false, 360) / sched_ext on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-42 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 5 maintainers not CCed: song@kernel.org haoluo@google.com jolsa@kernel.org kpsingh@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 4 this patch: 4
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes fail Problems with Fixes tag: 1
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: Unknown commit id '13b25489b6f8', maybe rebased or not pulled? WARNING: use relative pathname instead of absolute in changelog text
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Eduard Zingerman Dec. 3, 2024, 6:22 p.m. UTC 
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)
  (Andrii)
Changes v2 [2] -> v3:
- make sure --sysroot option is set for libbpf's EXTRA_CFLAGS,
  if $(SYSROOT) is set (Stanislav)

[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/
[2] https://lore.kernel.org/bpf/20241202234741.3492084-1-eddyz87@gmail.com/

Fixes: 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 samples/bpf/Makefile | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

patchwork-bot+netdevbpf@kernel.org Dec. 3, 2024, 10:20 p.m. UTC | #1 
Hello:

This patch was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Tue,  3 Dec 2024 10:22:22 -0800 you 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_)
>           |        ^
> 
> [...]

Here is the summary with links:
  - [bpf,v3] samples/bpf: remove unnecessary -I flags from libbpf EXTRA_CFLAGS
    https://git.kernel.org/bpf/bpf/c/5a6ea7022ff4

You are awesome, thank you!
Viktor Malik Dec. 4, 2024, 9:41 a.m. UTC | #2 
On 12/3/24 19:22, 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)
>   (Andrii)
> Changes v2 [2] -> v3:
> - make sure --sysroot option is set for libbpf's EXTRA_CFLAGS,
>   if $(SYSROOT) is set (Stanislav)
> 
> [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/
> [2] https://lore.kernel.org/bpf/20241202234741.3492084-1-eddyz87@gmail.com/
> 
> Fixes: 13b25489b6f8 ("kbuild: change working directory to external module directory with M=")
> Acked-by: Stanislav Fomichev <sdf@fomichev.me>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  samples/bpf/Makefile | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> 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)" \

I was not quick enough to reply before this got merged, sorry about that.

This will break situations when we want to pass extra flags to the
libbpf sub-make from the command line, e.g. to build samples as PIE:

    $ make TPROGS_USER_CFLAGS="-fpie" TPROGS_USER_LDFLAGS="-pie"
    [...]
    /usr/bin/ld: /bpf/samples/bpf/libbpf/libbpf.a(libbpf-in.o):
relocation R_X86_64_32 against `.rodata.str1.1' can not be used when
making a PIE object; recompile with -fPIE
    /usr/bin/ld: failed to set dynamic section sizes: bad value

I think that we should add

COMMON_CFLAGS = $(TPROGS_USER_CFLAGS)

somewhere to the top of the Makefile.

Viktor


>  		LDFLAGS="$(TPROGS_LDFLAGS)" srctree=$(BPF_SAMPLES_PATH)/../../ \
>  		O= OUTPUT=$(LIBBPF_OUTPUT)/ DESTDIR=$(LIBBPF_DESTDIR) prefix= \
>  		$@ install_headers
Eduard Zingerman Dec. 4, 2024, 5:13 p.m. UTC | #3 
On Wed, 2024-12-04 at 10:41 +0100, Viktor Malik wrote:

[...]

> I was not quick enough to reply before this got merged, sorry about that.
> 
> This will break situations when we want to pass extra flags to the
> libbpf sub-make from the command line, e.g. to build samples as PIE:
> 
>     $ make TPROGS_USER_CFLAGS="-fpie" TPROGS_USER_LDFLAGS="-pie"
>     [...]
>     /usr/bin/ld: /bpf/samples/bpf/libbpf/libbpf.a(libbpf-in.o):
> relocation R_X86_64_32 against `.rodata.str1.1' can not be used when
> making a PIE object; recompile with -fPIE
>     /usr/bin/ld: failed to set dynamic section sizes: bad value
> 
> I think that we should add
> 
> COMMON_CFLAGS = $(TPROGS_USER_CFLAGS)
> 
> somewhere to the top of the Makefile.

Hi Viktor,

This makes sense. Surprisingly enough, your example with -pie compiles
successfully for me, both using clang and gcc. I'll post an update
adding the suggested line.

Thanks,
Eduard
diff mbox series

Patch

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