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

[Francis Laniel <flaniel@linux.microsoft.com>]

Hi.


Le mercredi 8 février 2023, 16:47:58 CET Arnout Vandecappelle a écrit :
>   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/d126a4b6eca786402dc362c86f8df3adde
> >> c3
> >> 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)))" \

Thank you for all the details! Using -isystem sounds like a really good idea!

> 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?

I will sure take a look and will come back here with my findings!

>   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


Best regards.


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot