Re: [PATCH v1] samples/bpf: Fix build out of source tree

[Anh Tuan Phan <tuananhlfc@gmail.com>]

On 7/21/23 00:05, Stanislav Fomichev wrote:
> On Wed, Jul 19, 2023 at 9:13 AM Anh Tuan Phan <tuananhlfc@gmail.com> wrote:
>>
>>
>>
>> On 7/17/23 23:46, Stanislav Fomichev wrote:
>>> On Sun, Jul 16, 2023 at 2:42 AM Anh Tuan Phan <tuananhlfc@gmail.com> 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@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>  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@gmail.com>
>>>>>> ---
>>>>>>
>>>>>> 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.