Re: [Buildroot] [PATCH] package/libbpf: Don't remove bpf.h on host

[Arnout Vandecappelle <arnout@mind.be>]

  Hi Daniel, Francis,

On 07/02/2023 21:10, Francis Laniel wrote:
> Hi.
> 
> Le lundi 28 novembre 2022, 16:06:20 CET Lang Daniel a écrit :
>> libbpf >1.0.0 defines libbpf_bpf_link_type_str(enum bpf_link_type) in
>> src/libbpf.h, which is included by host-pahole.
>> bpf_link_type is defined in linux/bpf.h, therefore the comment stating
>> that pahole doesn't need bpf.h is no longer valid.
>>
>> Fixes:
>> -
>> http://autobuild.buildroot.net/results/d126a4b6eca786402dc362c86f8df3addec3
>> d217/
>>
>> Signed-off-by: Daniel Lang <d.lang@abatec.at>
>> ---
>> I wasn't able to reproduce the mentioned compile error in the kernel.
>> The mentioned file (tools/lib/bpf/strset.c) shouldn't be compiled when
>> compiling the kernel as it would recompile libbpf.
>> ---
>>   package/libbpf/libbpf.mk | 17 +----------------
>>   1 file changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/package/libbpf/libbpf.mk b/package/libbpf/libbpf.mk
>> index 820f1dc4bf..0381fd833a 100644
>> --- a/package/libbpf/libbpf.mk
>> +++ b/package/libbpf/libbpf.mk
>> @@ -39,26 +39,11 @@ define LIBBPF_INSTALL_TARGET_CMDS
>>   		-C $(@D)/src install DESTDIR=$(TARGET_DIR)
>>   endef
>>
>> -# We need to install_uapi_headers so we have btf.h to compile
>> -# host-pahole.
>> -# Nonetheless, this target adds bpf.h which generates a conflict when
>> -# building the kernel:
>> -# In file included from libbpf_internal.h:17:0, from strset.c:9:
>> -# relo_core.h:10:6: error: nested redefinition of 'enum bpf_core_relo_kind'
>> -# enum bpf_core_relo_kind {
>> -# ^~~~~~~~~~~~~~~~~~
>> -# relo_core.h:10:6: error: redeclaration of 'enum bpf_core_relo_kind'
>> -# In file included from libbpf_legacy.h:13:0,
>> -# 		from libbpf_internal.h:16,
>> -# 		from strset.c:9:
>> -# /home/francis/buildroot/output/host/include/linux/bpf.h:6497:6: note:
>> originally defined here -# enum bpf_core_relo_kind {
>> -# So, better to remove remove it now since we do not need it to build
>> +# We need to install_uapi_headers so we have bpf.h and btf.h to compile
>>   # host-pahole, the only user of host-libbpf.
>>   define HOST_LIBBPF_INSTALL_CMDS
>>   	$(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) \
>>   		-C $(@D)/src install install_uapi_headers DESTDIR=$(HOST_DIR)
>> -	rm $(HOST_DIR)/include/linux/bpf.h
>>   endef
>>
>>   $(eval $(generic-package))
> 
> Thank you for this patch and sorry for my very late reply.
> Sadly, I do not think the patch addresses the underlying issue.
> Indeed, this is not pahole which does not need this file, as pahole relies on
> <linux/bpf.h> to be built.
> So, the whole point of this comment and the rm is to avoid a problem when
> building the kernel when pahole was built before.

  The current solution, however, also doesn't work, which is why Franci's patch 
[1] is needed to fix it again.

  However, we believe this is not the proper fix. The real issue is the following:

- libbpf installs headers which are possibly incompatible with the kernel's 
headers. Normally this is not a problem, because you have either the kernel's or 
libbpf's headers installed, not both.

- However, when we install libbpf headers in $(HOST_DIR)/include, the kernel 
build itself picks up those headers instead of its own internal headers. That is 
why bpf.h is removed here to begin with. If bpf.h is not removed (and the kernel 
headers are indeed incompatible with the libbpf ones), then the kernel build 
will fail.

- There is however no reason why the kernel should pick up $(HOST_DIR)/include 
instead of its own internal headers. Normally it doesn't do that, because it 
supplies -I options to its own headers, and these have precedence over the 
system included /usr/include etc. However, we force HOSTCC="$(HOSTCC) 
$(HOST_CFLAGS)" and HOST_CFLAGS has -I$(HOST_DIR)/include. This one will come 
before all of the ones that the kernel adds and will have precedence.

Therefore, we believe that the solution is to use -isystem instead of -I in the 
HOSTCC setting of the kernel. This is indeed what is already done for uboot:

	HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem 
/,$(HOST_CFLAGS)))" \

Note that there is a risk that this by itself goes wrong as well. We tried at 
some point to use -isystem instead of -I in HOST_CFLAGS:

commit 6f8162cf8c1abef7e0a4771fe0d6b26a28f5c2b6
Author: David Raeman <draeman@bbn.com>
Date:   Mon Jul 25 21:52:26 2016

     package/Makefile.in should grab HOST_DIR headers using -isystem instead of -I.


but this was reverted a copule of days later:

commit 255b6f80d395ef048f46cfcf75dba690c56af657
Author: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date:   Sat Jul 30 18:10:18 2016

     Revert "package/Makefile.in should grab HOST_DIR headers using -isystem 
instead of -I."

(Unfortunately, the commit has no further explanation of what went wrong).

  So, could you (Daniel and Francis) try to apply this patch and replace -I with 
-isystem like is done for U-Boot, and do a bunch of kernel builds to see if it 
breaks anything?

  For now, I've marked this patch as Changes Requested (it can only be applied 
if the bpf.h problem is solved in some other way), and marked Francis's patch 
[1] as Superseded.

  Regards,
  Arnout


[1] 
https://patchwork.ozlabs.org/project/buildroot/patch/20220610165441.84812-2-flaniel@linux.microsoft.com/


> 
> Sadly, I tested your patch and I hit the problem described in the comment
> regarding redefinition of bpf_core_relo_kind.
> 
> Did you also hit it or you were able to build the kernel with
> BR2_LINUX_KERNEL_NEEDS_HOST_PAHOLE?
> 
> 
> Best regards and thank you in advance.
> 
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot