lkml.org 
[lkml]   [2018]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 05/12] kconfig: make syncconfig update .config regardless of sym_change_count
Date
Masahiro Yamada <yamada.masahiro@socionext.com> writes:

> 2018-07-09 20:39 GMT+09:00 Dirk Gouders <dirk@gouders.net>:
>> Dirk Gouders <dirk@gouders.net> writes:
>>
>>> Dirk Gouders <dirk@gouders.net> writes:
>>>
>>>> Masahiro Yamada <yamada.masahiro@socionext.com> writes:
>>>>
>>>>> syncconfig updates the .config only when sym_change_count > 0, i.e.
>>>>> any change in config symbols has been detected.
>>>>>
>>>>> Not only symbols but also comments are contained in the .config file.
>>>>> If only comments are updated, they are not fed back to the .config,
>>>>> then the stale comments are left-over. Of course, this is just a
>>>>> matter of comments, but why not fix it.
>>>>
>>>> Hello Masahiro,
>>>>
>>>> I am currently looking at and testing this series.
>>>>
>>>> First: For this patch I would suggest to also edit the syncconfig
>>>> section of "conf --help".
>>>>
>>>> Further, on a slow laptop, I was suspecting, this patch to cause full
>>>> rebuilds of everything, each time I ran "make syncconfig" followed by
>>>> "make" but could not verify this on another machine, so perhaps I am
>>>> just (for testing purposes) removing the wrong files (modules.builtin
>>>> for example) -- I am still testing.
>>>>
>>>> But, what irritates me with testing is that (also without your
>>>> patches) two consecutive "make" produce different output, one of them
>>>> always shows a warning and this is reproducable. I just want to make
>>>> sure there is no other problem that influences my testing:
>>>>
>>>> $ make
>>>> CALL scripts/checksyscalls.sh
>>>> DESCEND objtool
>>>> CHK include/generated/compile.h
>>>> DATAREL arch/x86/boot/compressed/vmlinux
>>>> Kernel: arch/x86/boot/bzImage is ready (#15)
>>>> Building modules, stage 2.
>>>> MODPOST 211 modules
>>>>
>>>> $ make
>>>> CALL scripts/checksyscalls.sh
>>>> DESCEND objtool
>>>> CHK include/generated/compile.h
>>>> LD arch/x86/boot/compressed/vmlinux
>>>> ld: arch/x86/boot/compressed/head_64.o: warning: relocation in read-only section `.head.text'
>>>> ld: warning: creating a DT_TEXTREL in object.
>>>> ZOFFSET arch/x86/boot/zoffset.h
>>>> AS arch/x86/boot/header.o
>>>> LD arch/x86/boot/setup.elf
>>>> OBJCOPY arch/x86/boot/setup.bin
>>>> OBJCOPY arch/x86/boot/vmlinux.bin
>>>> BUILD arch/x86/boot/bzImage
>>>> Setup is 15580 bytes (padded to 15872 bytes).
>>>> System is 8069 kB
>>>> CRC e01d75ec
>>>> Kernel: arch/x86/boot/bzImage is ready (#15)
>>>> Building modules, stage 2.
>>>> MODPOST 211 modules
>>>
>>> I spent some more time with the behaviour described above and bisected
>>> to the commit after that two consecutive invocations of "make" (on an
>>> already compiled tree) seem to do different things. That commit is
>>> 98f78525371b55cc (x86/boot: Refuse to build with data relocations), so I
>>> put Kees and Ingo on CC.
>>>
>>> I did the bisecting on another system, so I'll provide the output of two
>>> consecutive "make" on an already compiled tree on that machine:
>>>
>>> $ make
>>> CALL scripts/checksyscalls.sh
>>> DESCEND objtool
>>> CHK include/generated/compile.h
>>> DATAREL arch/x86/boot/compressed/vmlinux
>>> Kernel: arch/x86/boot/bzImage is ready (#48)
>>> Building modules, stage 2.
>>> MODPOST 165 modules
>>>
>>> $ make
>>> CALL scripts/checksyscalls.sh
>>> DESCEND objtool
>>> CHK include/generated/compile.h
>>> LD arch/x86/boot/compressed/vmlinux
>>> ZOFFSET arch/x86/boot/zoffset.h
>>> AS arch/x86/boot/header.o
>>> LD arch/x86/boot/setup.elf
>>> OBJCOPY arch/x86/boot/setup.bin
>>> OBJCOPY arch/x86/boot/vmlinux.bin
>>> BUILD arch/x86/boot/bzImage
>>> Setup is 15644 bytes (padded to 15872 bytes).
>>> System is 6663 kB
>>> CRC 3eb90f40
>>> Kernel: arch/x86/boot/bzImage is ready (#48)
>>> Building modules, stage 2.
>>> MODPOST 165 modules
>>>
>>> If I comment out $(call if_changed,check_data_rel) in
>>> arch/x86/boot/compressed/Makefile, two consecutive "make" produce
>>> identical output i.e. seem to not do different things:
>>>
>>> $ make
>>> CALL scripts/checksyscalls.sh
>>> DESCEND objtool
>>> CHK include/generated/compile.h
>>> Kernel: arch/x86/boot/bzImage is ready (#49)
>>> Building modules, stage 2.
>>> MODPOST 165 modules
>>>
>>> $ make
>>> CALL scripts/checksyscalls.sh
>>> DESCEND objtool
>>> CHK include/generated/compile.h
>>> Kernel: arch/x86/boot/bzImage is ready (#49)
>>> Building modules, stage 2.
>>> MODPOST 165 modules
>>>
>>> So, I guess this different behaviour of two consecutive "make" is not
>>> intentional but I am failing to understand why it happens.
>>
>> 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)

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, 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`')'

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

Dirk
\
 
 \ /
  Last update: 2018-07-15 22:05    [W:0.102 / U:0.644 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site