Message-ID: <ghefg6ffzw.fsf@lena.gouders.net>
Date:   Sat, 14 Jul 2018 09:12:03 +0200
From:   Dirk Gouders <dirk@...ders.net>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Ulf Magnusson <ulfalizer@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Ingo Molnar <mingo@...nel.org>,
        Michal Simek <monstr@...str.eu>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v3 05/12] kconfig: make syncconfig update .config regardless of sym_change_count

Masahiro Yamada <yamada.masahiro@...ionext.com> writes:

> 2018-07-12 20:32 GMT+09:00 Dirk Gouders <dirk@...ders.net>:
>> Masahiro Yamada <yamada.masahiro@...ionext.com> writes:
>>
>>> 2018-07-09 20:39 GMT+09:00 Dirk Gouders <dirk@...ders.net>:
>>>> Dirk Gouders <dirk@...ders.net> writes:
>>>>
>>>> I think, I solved the puzzle and perhaps, that saves others some time:
>>>>
>>>> The problem is that "if_changed" was not designed for multiple use
>>>> inside a recipe and in the case of compressed/vmlinux, the 2-fold use
>>>> created a kind of flip-flop for situations when nothing has to be done
>>>> to build the target.
>>>>
>>>> Because each of the two users of "if_changed" stores it's footprint in
>>>> .vmlinux.cmd but that file then isn't re-read, one of the two
>>>> "if_changed" calculates that nothing has to be done wheras the other one
>>>> recognizes a change in the commandline, because it sees the command-line
>>>> for the other part of the reciepe.
>>>>
>>>> In the next make, the roles flip, because the previously satisfied
>>>> "if_changed" now sees the command-line of the other one.  And so on...
>>>>
>>>> I am not a Kbuild expert but the attached patch fixes that problem by
>>>> introducing "if_changed_multi" that accepts two commands -- one whose
>>>> commandline should be checked and a second one that should be
>>>> executed.
>>>
>>>
>>> if_changed should not appear multiple times in one target.
>>>
>>> I think the simplest fix-up is to
>>> create a new command that combines
>>> 'cmd_check_data_rel' and 'cmd_ld'.
>>>
>>>
>>> quiet_cmd_link-vmlinux = LD      $@
>>>       cmd_link-vmlinux = $(cmd_check_data_rel); $(cmd_ld)
>>>
>>> $(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>>>         $(call if_changed,link-vmlinux)
>>>
>>> Kbuild also supports if_changed_rule,
>>> but the usage is more complex.
>>>
>>> There are only a few usages:
>>> https://github.com/torvalds/linux/blob/v4.17/scripts/Makefile.build#L288
>>
>> Just for completeness I will copy in part of a reply from Kees that
>> shows how double-colon rules can also avoid multiple use of if_changed
>> for one target:
>>
>> -$(obj)/vmlinux: $(vmlinux-objs-y) FORCE
>> -       $(call if_changed,check_data_rel)
>> +$(obj)/vmlinux:: $(vmlinux-objs-y)
>> +       $(call cmd,check_data_rel)
>> +$(obj)/vmlinux:: $(vmlinux-objs-y) FORCE
>>         $(call if_changed,ld)
>
> It is difficult to use double-colon rules in a _sane_ way.
>
> The first one just checks data_rel,
> but does not actually generate anything.
>
> Such targets should be marked as .PHONY,
> but $(obj)/vmlinux is not a phony target.
> This is strange.
>
>> The combined command seems to have the advantage that every command to
>> build the target gets recorded in the .cmd file
>>
>> A search showed me that we have two more users that use if_changed more
>> than once for a single target:
>>
>>           arch/microblaze/boot/Makefile                 (fourfold)
>>           arch/sparc/boot/Makefile               (2 times twofold)
>>
>> The sparc case seems to apply to any of the two suggested fixes,
>
> Neither is correct.
>
>
> $(obj)/uImage: $(obj)/image.gz
>         $(call if_changed,uimage)
>         $(call if_changed,uimage.o)
>
>
> should be split into two targets.
>
>
> $(obj)/uImage: $(obj)/image.gz FORCE
>         $(call if_changed,uimage)
>
> $(obj)/uImage.o: $(obj)/uImage FORCE
>         $(call if_changed,uimage.o)
>
>
> It is wrong in multiple ways.  FORCE is missing too.
>
>  but
>> microblaze uses if_changed in a pattern rule and also makes use of
>> parameter arguments in the sub-commands:
>>
>> $(obj)/simpleImage.%: vmlinux FORCE
>>         $(call if_changed,cp,.unstrip)
>>         $(call if_changed,objcopy)
>>         $(call if_changed,uimage)
>>         $(call if_changed,strip,.strip)
>>         @echo 'Kernel: $(UIMAGE_OUT) is ready' ' (#'`cat .version`')'
>
>
> Probably, this is the same.
>
> Create a target for each step.
>
>
>> In this case, double colons would have a different meaning and the
>> combined command solution would result in a change of the sub-commands,
>> as well.  I note this in case Michal perhaps has other preferences.
>>
>>
>> In addition to extend the documentation, we could modify if_changed to
>> warn about it is being used more than once for a target:
>>
>> # Execute command if command has changed or prerequisite(s) are updated.
>> if_changed = $(if $(filter-out undefined,$(origin if_changed_$@)),           \
>>         @set -e;                                                             \
>>         echo "Warning: $@: multiple use of if_changed!" >&2; ,               \
>>         @set -e $(eval if_changed_$@ := 1) ; )                               \
>>         $(if $(strip $(any-prereq) $(arg-check)),                            \
>>         $(echo-cmd) $(cmd_$(1));                                             \
>>         printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, :)
>>
>> But this fires only if if_changed is actually called and it defines many
>> variables for just that purpose, so this is perhaps not what we want...
>>
>
> I do not want to mess up Makefile.
>
>
> Please do this check in scripts/checkpatch.pl
> if you want.

Thank you for spending your time in the detailed explanation.
I will use it to assemble that all to a fix and then send it for review.

Dirk

Powered by blists - more mailing lists