On 7/21/23 00:05, Stanislav Fomichev wrote: > On Wed, Jul 19, 2023 at 9:13 AM Anh Tuan Phan <tuananhlfc@xxxxxxxxx> wrote: >> >> >> >> On 7/17/23 23:46, Stanislav Fomichev wrote: >>> On Sun, Jul 16, 2023 at 2:42 AM Anh Tuan Phan <tuananhlfc@xxxxxxxxx> wrote: >>>> >>>> >>>> >>>> On 7/11/23 00:18, Stanislav Fomichev wrote: >>>>> On 07/09, Anh Tuan Phan wrote: >>>>>> I updated the patch to reflect your suggestion. Thank you! >>>>> >>>>> In the future, can you please post a new one with v+1 instead of replying >>>>> to the old one? Thx! >>>> >>>> Will do in the next version. >>>> >>>>> >>>>>> On 7/7/23 23:53, Stanislav Fomichev wrote: >>>>>>> On 07/07, Anh Tuan Phan wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 7/7/23 01:09, Stanislav Fomichev wrote: >>>>>>>>> On 07/06, Anh Tuan Phan wrote: >>>>>>>>>> This commit fixes a few compilation issues when building out of source >>>>>>>>>> tree. The command that I used to build samples/bpf: >>>>>>>>>> >>>>>>>>>> export KBUILD_OUTPUT=/tmp >>>>>>>>>> make V=1 M=samples/bpf >>>>>>>>>> >>>>>>>>>> The compilation failed since it tried to find the header files in the >>>>>>>>>> wrong places between output directory and source tree directory >>>>>>>>>> >>>>>>>>>> Signed-off-by: Anh Tuan Phan <tuananhlfc@xxxxxxxxx> >>>>>>>>>> --- >>>>>>>>>> samples/bpf/Makefile | 8 ++++---- >>>>>>>>>> samples/bpf/Makefile.target | 2 +- >>>>>>>>>> 2 files changed, 5 insertions(+), 5 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile >>>>>>>>>> index 615f24ebc49c..32469aaa82d5 100644 >>>>>>>>>> --- a/samples/bpf/Makefile >>>>>>>>>> +++ b/samples/bpf/Makefile >>>>>>>>>> @@ -341,10 +341,10 @@ $(obj)/hbm_edt_kern.o: $(src)/hbm.h $(src)/hbm_kern.h >>>>>>>>>> # Override includes for xdp_sample_user.o because $(srctree)/usr/include in >>>>>>>>>> # TPROGS_CFLAGS causes conflicts >>>>>>>>>> XDP_SAMPLE_CFLAGS += -Wall -O2 \ >>>>>>>>>> - -I$(src)/../../tools/include \ >>>>>>>>>> + -I$(srctree)/tools/include \ >>>>>>>>> >>>>>>>>> [..] >>>>>>>>> >>>>>>>>>> -I$(src)/../../tools/include/uapi \ >>>>>>>>> >>>>>>>>> Does this $(src) need to be changed as well? >>>>>>>> >>>>>>>> I think this line doesn't affect the build. I removed it and it still >>>>>>>> compiles (after "make -C samples/bpf clean"). I guess xdp_sample_user.c >>>>>>>> doesn't include any file in tools/include/uapi. Am I missing something >>>>>>>> or should I remove this line? >>>>>>> >>>>>>> You might have these headers installed on your system already if >>>>>>> it compiles without this part. So I'd keep this part but do >>>>>>> s/src/srctree/ (and remove ../..). >>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> -I$(LIBBPF_INCLUDE) \ >>>>>>>>>> - -I$(src)/../../tools/testing/selftests/bpf >>>>>>>>>> + -I$(srctree)/tools/testing/selftests/bpf >>>>>>>>>> >>>>>>>>>> $(obj)/$(XDP_SAMPLE): TPROGS_CFLAGS = $(XDP_SAMPLE_CFLAGS) >>>>>>>>>> $(obj)/$(XDP_SAMPLE): $(src)/xdp_sample_user.h $(src)/xdp_sample_shared.h >>>>>>>>>> @@ -393,7 +393,7 @@ $(obj)/xdp_router_ipv4.bpf.o: $(obj)/xdp_sample.bpf.o >>>>>>>>>> $(obj)/%.bpf.o: $(src)/%.bpf.c $(obj)/vmlinux.h $(src)/xdp_sample.bpf.h >>>>>>>>>> $(src)/xdp_sample_shared.h >>>>>>>>>> @echo " CLANG-BPF " $@ >>>>>>>>>> $(Q)$(CLANG) -g -O2 -target bpf -D__TARGET_ARCH_$(SRCARCH) \ >>>>>>>>>> - -Wno-compare-distinct-pointer-types -I$(srctree)/include \ >>>>>>>>>> + -Wno-compare-distinct-pointer-types -I$(obj) -I$(srctree)/include \ >>>>>>>>>> -I$(srctree)/samples/bpf -I$(srctree)/tools/include \ >>>>>>>>>> -I$(LIBBPF_INCLUDE) $(CLANG_SYS_INCLUDES) \ >>>>>>>>>> -c $(filter %.bpf.c,$^) -o $@ >>>>>>>>>> @@ -412,7 +412,7 @@ xdp_router_ipv4.skel.h-deps := xdp_router_ipv4.bpf.o >>>>>>>>>> xdp_sample.bpf.o >>>>>>>>>> >>>>>>>>>> LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.bpf.c,$(foreach >>>>>>>>>> skel,$(LINKED_SKELS),$($(skel)-deps))) >>>>>>>>>> >>>>>>>>>> -BPF_SRCS_LINKED := $(notdir $(wildcard $(src)/*.bpf.c)) >>>>>>>>>> +BPF_SRCS_LINKED := $(notdir $(wildcard $(srctree)/$(src)/*.bpf.c)) >>>>>>>>>> BPF_OBJS_LINKED := $(patsubst %.bpf.c,$(obj)/%.bpf.o, $(BPF_SRCS_LINKED)) >>>>>>>>>> BPF_SKELS_LINKED := $(addprefix $(obj)/,$(LINKED_SKELS)) >>>>>>>>>> >>>>>>>>>> diff --git a/samples/bpf/Makefile.target b/samples/bpf/Makefile.target >>>>>>>>>> index 7621f55e2947..86a454cfb080 100644 >>>>>>>>>> --- a/samples/bpf/Makefile.target >>>>>>>>>> +++ b/samples/bpf/Makefile.target >>>>>>>>>> @@ -41,7 +41,7 @@ _tprogc_flags = $(TPROGS_CFLAGS) \ >>>>>>>>>> $(TPROGCFLAGS_$(basetarget).o) >>>>>>>>>> >>>>>>>>>> # $(objtree)/$(obj) for including generated headers from checkin source >>>>>>>>>> files >>>>>>>>> >>>>>>>>> [..] >>>>>>>>> >>>>>>>>>> -ifeq ($(KBUILD_EXTMOD),) >>>>>>>>>> +ifneq ($(KBUILD_EXTMOD),) >>>>>>>>> >>>>>>>>> This parts seems to be copy-pasted all over the place in its 'ifeq' >>>>>>>>> form. What is it doing and why is it needed? >>>>>>>>> >>>>>>>>>> ifdef building_out_of_srctree >>>>>>>>>> _tprogc_flags += -I $(objtree)/$(obj) >>>>>>>>>> endif >>>>>>>>>> -- >>>>>>>>>> 2.34.1 >>>>>> >>>>>> Signed-off-by: Anh Tuan Phan <tuananhlfc@xxxxxxxxx> >>>>>> --- >>>>>> >>>>>> Change from the original patch >>>>>> >>>>>> - Change "-I$(src)/../../tools/include/uapi" to >>>>>> "-I$(srctree)/tools/include/uapi" >>>>>> >>>>>> samples/bpf/Makefile | 10 +++++----- >>>>>> samples/bpf/Makefile.target | 2 +- >>>>>> 2 files changed, 6 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile >>>>>> index 615f24ebc49c..cfc960b3713a 100644 >>>>>> --- a/samples/bpf/Makefile >>>>>> +++ b/samples/bpf/Makefile >>>>>> @@ -341,10 +341,10 @@ $(obj)/hbm_edt_kern.o: $(src)/hbm.h $(src)/hbm_kern.h >>>>>> # Override includes for xdp_sample_user.o because $(srctree)/usr/include in >>>>>> # TPROGS_CFLAGS causes conflicts >>>>>> XDP_SAMPLE_CFLAGS += -Wall -O2 \ >>>>>> - -I$(src)/../../tools/include \ >>>>>> - -I$(src)/../../tools/include/uapi \ >>>>>> + -I$(srctree)/tools/include \ >>>>>> + -I$(srctree)/tools/include/uapi \ >>>>>> -I$(LIBBPF_INCLUDE) \ >>>>>> - -I$(src)/../../tools/testing/selftests/bpf >>>>>> + -I$(srctree)/tools/testing/selftests/bpf >>>>>> >>>>>> $(obj)/$(XDP_SAMPLE): TPROGS_CFLAGS = $(XDP_SAMPLE_CFLAGS) >>>>>> $(obj)/$(XDP_SAMPLE): $(src)/xdp_sample_user.h $(src)/xdp_sample_shared.h >>>>>> @@ -393,7 +393,7 @@ $(obj)/xdp_router_ipv4.bpf.o: $(obj)/xdp_sample.bpf.o >>>>>> $(obj)/%.bpf.o: $(src)/%.bpf.c $(obj)/vmlinux.h $(src)/xdp_sample.bpf.h >>>>>> $(src)/xdp_sample_shared.h >>>>>> @echo " CLANG-BPF " $@ >>>>>> $(Q)$(CLANG) -g -O2 -target bpf -D__TARGET_ARCH_$(SRCARCH) \ >>>>>> - -Wno-compare-distinct-pointer-types -I$(srctree)/include \ >>>>>> + -Wno-compare-distinct-pointer-types -I$(obj) -I$(srctree)/include \ >>>>>> -I$(srctree)/samples/bpf -I$(srctree)/tools/include \ >>>>>> -I$(LIBBPF_INCLUDE) $(CLANG_SYS_INCLUDES) \ >>>>>> -c $(filter %.bpf.c,$^) -o $@ >>>>>> @@ -412,7 +412,7 @@ xdp_router_ipv4.skel.h-deps := xdp_router_ipv4.bpf.o >>>>>> xdp_sample.bpf.o >>>>>> >>>>>> LINKED_BPF_SRCS := $(patsubst %.bpf.o,%.bpf.c,$(foreach >>>>>> skel,$(LINKED_SKELS),$($(skel)-deps))) >>>>>> >>>>>> -BPF_SRCS_LINKED := $(notdir $(wildcard $(src)/*.bpf.c)) >>>>>> +BPF_SRCS_LINKED := $(notdir $(wildcard $(srctree)/$(src)/*.bpf.c)) >>>>>> BPF_OBJS_LINKED := $(patsubst %.bpf.c,$(obj)/%.bpf.o, $(BPF_SRCS_LINKED)) >>>>>> BPF_SKELS_LINKED := $(addprefix $(obj)/,$(LINKED_SKELS)) >>>>>> >>>>>> diff --git a/samples/bpf/Makefile.target b/samples/bpf/Makefile.target >>>>>> index 7621f55e2947..86a454cfb080 100644 >>>>>> --- a/samples/bpf/Makefile.target >>>>>> +++ b/samples/bpf/Makefile.target >>>>>> @@ -41,7 +41,7 @@ _tprogc_flags = $(TPROGS_CFLAGS) \ >>>>>> $(TPROGCFLAGS_$(basetarget).o) >>>>>> >>>>>> # $(objtree)/$(obj) for including generated headers from checkin source >>>>>> files >>>>> >>>>> [..] >>>>> >>>>>> -ifeq ($(KBUILD_EXTMOD),) >>>>>> +ifneq ($(KBUILD_EXTMOD),) >>>>>> ifdef building_out_of_srctree >>>>>> _tprogc_flags += -I $(objtree)/$(obj) >>>>>> endif >>>>> >>>>> This question left undressed. Can you share more on why this change >>>>> is needed? Because it looks like it's actually needed for M='' case. >>>>> IOW, maybe we should add $(objtree)/$(obj) somewhere else? >>>> >>>> If it's needed for both cases M='' and M != '', is it better to just >>>> remove the condition with $(KBUILD_EXTMOD)? FMPOV, -I $(objtree)/$(obj) >>>> is only needed in case of building_out_of_srctree, no matter what >>>> KBUILD_EXTMOD. >>> >>> My guess is that we're missing -I $(objtree)/$(obj) somewhere else. >>> Depending on what/where it fails, we probably need to add $(objtree) >>> to some of the $(obj)s? >>> I'm mostly speculating here, because I see that "ifeq >>> ($(KBUILD_EXTMOD),) + ifdef building_out_of_srctree" pattern being >>> used in the other places. >> >> It turns out that your guess is right. "-I $(objtree)/$(obj)" is only >> needed for the following objects: xdp_redirect_map_multi_user, >> xdp_redirect_cpu_user, xdp_redirect_map_user, xdp_redirect_user, >> xdp_monitor_user and xdp_router_ipv4_user. There is a variable to add >> flags for gcc named $(TPROGCFLAGS_$(basetarget).o) but it has not been >> set. My proposed fix is to set the following variables: > > Looks like these are the tests that use bpf skeleton. > >> +TPROGCFLAGS_xdp_redirect_map_multi_user.o += -I$(objtree)/$(obj) >> +TPROGCFLAGS_xdp_redirect_cpu_user.o += -I$(objtree)/$(obj) >> +TPROGCFLAGS_xdp_redirect_map_user.o += -I$(objtree)/$(obj) >> +TPROGCFLAGS_xdp_redirect_user.o += -I$(objtree)/$(obj) >> +TPROGCFLAGS_xdp_monitor_user.o += -I$(objtree)/$(obj) >> +TPROGCFLAGS_xdp_router_ipv4_user.o += -I$(objtree)/$(obj) >> >> It works with the original "ifeq >>> ($(KBUILD_EXTMOD),) + ifdef building_out_of_srctree" pattern. What do >> you think about my proposed fix? > > Maybe we should just unconditionally add "-I $(objtree)/$(obj)" to > _tprogc_flags ? > > And then we can drop that whole part: > ifeq ($(KBUILD_EXTMOD),) > ifdef building_out_of_srctree > _tprogc_flags += -I $(objtree)/$(obj) > endif > endif > > IOW, replace that $(TPROGCFLAGS_$(basetarget).o) part with -I > $(objtree)/$(obj) and call it a day? > I honestly have no clue why it's this complicated and I doubt anybody > at this point remembers... That is the simplest way. I intended to do it but I didn't as I think the condition is there for some reasons that I haven't understood yet.
