Re: [PATCH] kbuild: add extra-y to targets-for-modules

[Artem Savkov]

On Thu, Dec 17, 2020 at 05:26:07PM +0900, Masahiro Yamada wrote:
> On Thu, Dec 17, 2020 at 8:04 AM Joe Lawrence <joe.lawrence@xxxxxxxxxx> wrote:
> >
> > On 12/16/20 1:14 AM, Masahiro Yamada wrote:
> > > On Tue, Dec 8, 2020 at 11:31 PM Artem Savkov <artem.savkov@xxxxxxxxx> wrote:
> > >>
> > >> On Tue, Dec 08, 2020 at 05:20:35PM +0800, WANG Chao wrote:
> > >>> Sorry for the late reply.
> > >>>
> > >>> On 11/25/20 at 10:42P, Masahiro Yamada wrote:
> > >>>> On Tue, Nov 24, 2020 at 12:05 AM WANG Chao <chao.wang@xxxxxxxxx> wrote:
> > >>>>>
> > >>>>> On 11/23/20 at 02:23P, Masahiro Yamada wrote:
> > >>>>>> On Tue, Nov 3, 2020 at 3:23 PM WANG Chao <chao.wang@xxxxxxxxx> wrote:
> > >>>>>>>
> > >>>>>>> extra-y target doesn't build for 'make M=...' since commit 6212804f2d78
> > >>>>>>> ("kbuild: do not create built-in objects for external module builds").
> > >>>>>>>
> > >>>>>>> This especially breaks kpatch, which is using 'extra-y := kpatch.lds'
> > >>>>>>> and 'make M=...' to build livepatch patch module.
> > >>>>>>>
> > >>>>>>> Add extra-y to targets-for-modules so that such kind of build works
> > >>>>>>> properly.
> > >>>>>>>
> > >>>>>>> Signed-off-by: WANG Chao <chao.wang@xxxxxxxxx>
> > >>>>>>> ---
> > >>>>>>>   scripts/Makefile.build | 2 +-
> > >>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>>>
> > >>>>>>> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > >>>>>>> index ae647379b579..0113a042d643 100644
> > >>>>>>> --- a/scripts/Makefile.build
> > >>>>>>> +++ b/scripts/Makefile.build
> > >>>>>>> @@ -86,7 +86,7 @@ ifdef need-builtin
> > >>>>>>>   targets-for-builtin += $(obj)/built-in.a
> > >>>>>>>   endif
> > >>>>>>>
> > >>>>>>> -targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> > >>>>>>> +targets-for-modules := $(extra-y) $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> > >>>>>>>
> > >>>>>>>   ifdef need-modorder
> > >>>>>>>   targets-for-modules += $(obj)/modules.order
> > >>>>>>> --
> > >>>>>>> 2.29.1
> > >>>>>>>
> > >>>>>>
> > >>>>>> NACK.
> > >>>>>>
> > >>>>>> Please fix your Makefile.
> > >>>>>>
> > >>>>>> Hint:
> > >>>>>> https://patchwork.kernel.org/project/linux-kbuild/patch/20201123045403.63402-6-masahiroy@xxxxxxxxxx/
> > >>>>>>
> > >>>>>>
> > >>>>>> Probably what you should use is 'targets'.
> > >>>>>
> > >>>>> I tried with 'targets' and 'always-y'. Both doesn't work for me.
> > >>>>>
> > >>>>> I narraw it down to the following example:
> > >>>>>
> > >>>>> cat > Makefile << _EOF_
> > >>>>> obj-m += foo.o
> > >>>>>
> > >>>>> ldflags-y += -T $(src)/kpatch.lds
> > >>>>> always-y += kpatch.lds
> > >>>>>
> > >>>>> foo-objs += bar.o
> > >>>>>
> > >>>>> all:
> > >>>>>          make -C /lib/modules/$(shell uname -r)/build M=$(PWD)
> > >>>>> clean:
> > >>>>>          make -C /lib/modules/$(shell uname -r)/build M=$(PWD) clean
> > >>>>> _EOF_
> > >>>>>
> > >>>>> Take a look into scripts/Makefile.build:488:
> > >>>>>
> > >>>>> __build: $(if $(KBUILD_BUILTIN), $(targets-for-builtin)) \
> > >>>>>           $(if $(KBUILD_MODULES), $(targets-for-modules)) \
> > >>>>>           $(subdir-ym) $(always-y)
> > >>>>>          @:
> > >>>>>
> > >>>>> 'always-y' is built after 'targets-for-modules'. This makes
> > >>>>> 'targets-for-modules' fails because kpatch.lds isn't there.
> > >>>>
> > >>>>
> > >>>> Heh, you rely on the targets built from left to right,
> > >>>> and you have never thought Make supports the parallel option -j.
> > >>>
> > >>> You're right. I missed that.
> > >>>
> > >>>>
> > >>>>
> > >>>> You need to specify the dependency if you expect objects
> > >>>> are built in the particular order.
> > >>>>
> > >>>> However, in this case, using ldflags-y looks wrong
> > >>>> in the first place.
> > >>>>
> > >>>> The linker script is used when combining the object
> > >>>> as well as the final link of *.ko
> > >>
> > >> We want linker script to be used on both those steps, otherwise modpost
> > >> fails.
> > >
> > >
> > > In that case, does the following work?
> > > (untested)
> > >
> > >
> > >
> > > diff --git a/kmod/patch/Makefile b/kmod/patch/Makefile
> > > index e017b17..02d4c66 100644
> > > --- a/kmod/patch/Makefile
> > > +++ b/kmod/patch/Makefile
> > > @@ -12,7 +12,9 @@ endif
> > >
> > >   obj-m += $(KPATCH_NAME).o
> > >   ldflags-y += -T $(src)/kpatch.lds
> > > -extra-y := kpatch.lds
> > > +targets += kpatch.lds
> > > +
> > > +$(obj)/$(KPATCH_NAME).o: $(obj)/kpatch.lds
> > >
> > >   $(KPATCH_NAME)-objs += patch-hook.o output.o
> > >
> >
> > Hi Masahiro,
> >
> > Yeah this is more or less what Artem came up with:
> > https://github.com/dynup/kpatch/pull/1149
> >
> > though we hadn't added kpatch.lds to targets.  Is there documentation
> > somewhere on what effect "targets" has for out-of-tree builds?
> >
> > Thanks,
> >
> > -- Joe
> >
> 
> 
> Please try the rebuild without changing any source code.
> 
> If kpatch.lds is needlessly rebuilt, you need to add
> it to 'targets'.
> 
> 
> In linux-next, this is documented.
> 
> Documentation/kbuild/makefile.rst:
> 
>         Any target that utilizes if_changed must be listed in $(targets),
>         otherwise the command line check will fail, and the target will
>         always be built.

Ah yes, it did rebuild every time. Thank you for the pointer.

-- 
 Artem