diff mbox series

[7/7] kbuild: link vmlinux just once for CONFIG_TRIM_UNUSED_KSYMS

Message ID 1521045861-22418-8-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series kbuild: various fix, clean-up, improvements of CONFIG_TRIM_UNUSED_KSYMS | expand

Commit Message

Masahiro Yamada March 14, 2018, 4:44 p.m. UTC 
If CONFIG_TRIM_UNUSED_KSYMS is enabled and the kernel is built from
a pristine state, the vmlinux is linked twice.

[1] A user runs "make"

[2] First build with empty autoksyms.h

[3] adjust_autoksyms.sh updates autoksyms.h and recurses "make vmlinux"

  --------(begin sub-make)--------
  [4] Second build with new autoksyms.h

  [5] link-vmlinux.sh is invoked because "vmlinux" is missing
  ---------(end sub-make)---------

[6] link-vmlinux.sh is invoked again despite "vmlinux" is up-to-date.

The reason of [6] is probably because Make already decided to update
"vmlinux" at the time of [2] because "vmlinux" was missing when Make
generated the dependency list.

link-vmlinus.sh is costly, so it is better to not run it when unneeded.
Split CONFIG_TRIM_UNUSED_KSYMS handling to a dedicated target.

The reason of commit 2441e78b1919 ("kbuild: better abstract vmlinux
sequential prerequisites") was to cater to CONFIG_BUILD_DOCSRC, but
it was later removed by commit 184892925118 ("samples: move blackfin
gptimers-example from Documentation").

I also changed adjust_autoksyms.sh to simply exit with 1 or 0 to make
it look straightforward.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 Makefile                    | 28 ++++++++++++----------------
 scripts/adjust_autoksyms.sh |  3 +--
 2 files changed, 13 insertions(+), 18 deletions(-)

-- 
2.7.4

Comments

Nicolas Pitre March 14, 2018, 7:06 p.m. UTC | #1 
On Thu, 15 Mar 2018, Masahiro Yamada wrote:

> If CONFIG_TRIM_UNUSED_KSYMS is enabled and the kernel is built from

> a pristine state, the vmlinux is linked twice.

> 

> [1] A user runs "make"

> 

> [2] First build with empty autoksyms.h

> 

> [3] adjust_autoksyms.sh updates autoksyms.h and recurses "make vmlinux"

> 

>   --------(begin sub-make)--------

>   [4] Second build with new autoksyms.h

> 

>   [5] link-vmlinux.sh is invoked because "vmlinux" is missing

>   ---------(end sub-make)---------

> 

> [6] link-vmlinux.sh is invoked again despite "vmlinux" is up-to-date.

> 

> The reason of [6] is probably because Make already decided to update

> "vmlinux" at the time of [2] because "vmlinux" was missing when Make

> generated the dependency list.


But the dependency list for vmlinux contains FORCE so the target is 
always remade in (2) anyway. The decision to actually invoke 
link-vmlinux.sh comes from "if_changed". Why is $(call if_changed,...) 
in (6) not noticing that vmlinux is up to date?

> 

> link-vmlinus.sh is costly, so it is better to not run it when unneeded.

> Split CONFIG_TRIM_UNUSED_KSYMS handling to a dedicated target.

> 

> The reason of commit 2441e78b1919 ("krbuild: better abstract vmlinux

> sequential prerequisites") was to cater to CONFIG_BUILD_DOCSRC, but

> it was later removed by commit 184892925118 ("samples: move blackfin

> gptimers-example from Documentation").

> 

> I also changed adjust_autoksyms.sh to simply exit with 1 or 0 to make

> it look straightforward.


That is wrong. If the script fails for some reason (it runs with -e set) 
then this will trigger an endless recursive make instead of failing the 
build.

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

> ---

> 

>  Makefile                    | 28 ++++++++++++----------------

>  scripts/adjust_autoksyms.sh |  3 +--

>  2 files changed, 13 insertions(+), 18 deletions(-)

> 

> diff --git a/Makefile b/Makefile

> index 1dab647..0a7bab6 100644

> --- a/Makefile

> +++ b/Makefile

> @@ -987,21 +987,11 @@ export KBUILD_ALLDIRS := $(sort $(filter-out arch/%,$(vmlinux-alldirs)) arch Doc

>  

>  vmlinux-deps := $(KBUILD_LDS) $(KBUILD_VMLINUX_INIT) $(KBUILD_VMLINUX_MAIN) $(KBUILD_VMLINUX_LIBS)

>  

> -# Include targets which we want to execute sequentially if the rest of the

> -# kernel build went well. If CONFIG_TRIM_UNUSED_KSYMS is set, this might be

> -# evaluated more than once.

> -PHONY += vmlinux_prereq

> -vmlinux_prereq: $(vmlinux-deps) FORCE

> -ifdef CONFIG_HEADERS_CHECK

> -	$(Q)$(MAKE) -f $(srctree)/Makefile headers_check

> -endif

> -ifdef CONFIG_GDB_SCRIPTS

> -	$(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)

> -endif

> -ifdef CONFIG_TRIM_UNUSED_KSYMS

> -	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \

> -	  "$(MAKE) -f $(srctree)/Makefile vmlinux"

> -endif

> +# Recurse until adjust_autoksyms.sh is satisfied

> +PHONY += autoksyms_recursive

> +autoksyms_recursive: $(vmlinux-deps)

> +	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh || \

> +	$(MAKE) -f $(srctree)/Makefile autoksyms_recursive

>  

>  # For the kernel to actually contain only the needed exported symbols,

>  # we have to build modules as well to determine what those symbols are.

> @@ -1023,7 +1013,13 @@ cmd_link-vmlinux =                                                 \

>  	$(CONFIG_SHELL) $< $(LD) $(LDFLAGS) $(LDFLAGS_vmlinux) ;    \

>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)

>  

> -vmlinux: scripts/link-vmlinux.sh vmlinux_prereq $(vmlinux-deps) FORCE

> +vmlinux: scripts/link-vmlinux.sh autoksyms_recursive $(vmlinux-deps) FORCE

> +ifdef CONFIG_HEADERS_CHECK

> +	$(Q)$(MAKE) -f $(srctree)/Makefile headers_check

> +endif

> +ifdef CONFIG_GDB_SCRIPTS

> +	$(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)

> +endif

>  	+$(call if_changed,link-vmlinux)

>  

>  # Build samples along the rest of the kernel

> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh

> index 7bb3618..1377cf8 100755

> --- a/scripts/adjust_autoksyms.sh

> +++ b/scripts/adjust_autoksyms.sh

> @@ -95,8 +95,7 @@ if [ $changed -gt 0 ]; then

>  	info "KSYMS" "symbols: before=$old, after=$new, changed=$changed"

>  	info "UPD" "$cur_ksyms_file"

>  	mv -f "$new_ksyms_file" "$cur_ksyms_file"

> -	# Then trigger a rebuild of affected source files

> -	exec $@

> +	exit 1

>  else

>  	rm -f "$new_ksyms_file"

>  fi

> -- 

> 2.7.4

> 

>
Masahiro Yamada March 15, 2018, 8 a.m. UTC | #2 
2018-03-15 4:06 GMT+09:00 Nicolas Pitre <nicolas.pitre@linaro.org>:
> On Thu, 15 Mar 2018, Masahiro Yamada wrote:

>

>> If CONFIG_TRIM_UNUSED_KSYMS is enabled and the kernel is built from

>> a pristine state, the vmlinux is linked twice.

>>

>> [1] A user runs "make"

>>

>> [2] First build with empty autoksyms.h

>>

>> [3] adjust_autoksyms.sh updates autoksyms.h and recurses "make vmlinux"

>>

>>   --------(begin sub-make)--------

>>   [4] Second build with new autoksyms.h

>>

>>   [5] link-vmlinux.sh is invoked because "vmlinux" is missing

>>   ---------(end sub-make)---------

>>

>> [6] link-vmlinux.sh is invoked again despite "vmlinux" is up-to-date.

>>

>> The reason of [6] is probably because Make already decided to update

>> "vmlinux" at the time of [2] because "vmlinux" was missing when Make

>> generated the dependency list.

>

> But the dependency list for vmlinux contains FORCE so the target is

> always remade in (2) anyway. The decision to actually invoke

> link-vmlinux.sh comes from "if_changed". Why is $(call if_changed,...)

> in (6) not noticing that vmlinux is up to date?


if_changed (more specifically 'any-prereq')
is implemented based on '$?'.

So, this problem can be narrowed down to
how Make decides '$?'.


If you apply the first 6 patches, and put the following
debug code, you will see what has happened to $?


@@ -1024,6 +1026,7 @@ cmd_link-vmlinux =
                  \
        $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)

 vmlinux: scripts/link-vmlinux.sh vmlinux_prereq $(vmlinux-deps) FORCE
+       @echo newer deps: $?
        +$(call if_changed,link-vmlinux)

 # Build samples along the rest of the kernel



I am not familiar with Make internal, but
I can test it with simple code.


[Test Makefile]
A: B
        cp B A
        echo $?

B: C
        cp C B
        touch A

[Result]
$ rm -rf A B C
$ touch C
$ make
cp C B
touch A
cp B A
echo B
B


In the recipe of 'B', 'A' is touched,
so the dependency 'A: B' has already been met
before the recipe of 'A' is executed,
but Make does not notice that fact,
then tries to update 'A'.


The situation is the same.
vmlinux has been updated in the recursed sub-make,
but it is not noticed.




>>

>> link-vmlinus.sh is costly, so it is better to not run it when unneeded.

>> Split CONFIG_TRIM_UNUSED_KSYMS handling to a dedicated target.

>>

>> The reason of commit 2441e78b1919 ("krbuild: better abstract vmlinux

>> sequential prerequisites") was to cater to CONFIG_BUILD_DOCSRC, but

>> it was later removed by commit 184892925118 ("samples: move blackfin

>> gptimers-example from Documentation").

>>

>> I also changed adjust_autoksyms.sh to simply exit with 1 or 0 to make

>> it look straightforward.

>

> That is wrong. If the script fails for some reason (it runs with -e set)

> then this will trigger an endless recursive make instead of failing the

> build.


Sorry, I missed this case.  You are right.

I will restore the original code.



-- 
Best Regards
Masahiro Yamada
Nicolas Pitre March 15, 2018, 6:50 p.m. UTC | #3 
On Thu, 15 Mar 2018, Masahiro Yamada wrote:

> I am not familiar with Make internal, but

> I can test it with simple code.

> 

> 

> [Test Makefile]

> A: B

>         cp B A

>         echo $?

> 

> B: C

>         cp C B

>         touch A

> 

> [Result]

> $ rm -rf A B C

> $ touch C

> $ make

> cp C B

> touch A

> cp B A

> echo B

> B

> 

> 

> In the recipe of 'B', 'A' is touched,

> so the dependency 'A: B' has already been met

> before the recipe of 'A' is executed,

> but Make does not notice that fact,

> then tries to update 'A'.

> 

> 

> The situation is the same.

> vmlinux has been updated in the recursed sub-make,

> but it is not noticed.


OK, I agree.


Nicolas
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 1dab647..0a7bab6 100644
--- a/Makefile
+++ b/Makefile
@@ -987,21 +987,11 @@  export KBUILD_ALLDIRS := $(sort $(filter-out arch/%,$(vmlinux-alldirs)) arch Doc
 
 vmlinux-deps := $(KBUILD_LDS) $(KBUILD_VMLINUX_INIT) $(KBUILD_VMLINUX_MAIN) $(KBUILD_VMLINUX_LIBS)
 
-# Include targets which we want to execute sequentially if the rest of the
-# kernel build went well. If CONFIG_TRIM_UNUSED_KSYMS is set, this might be
-# evaluated more than once.
-PHONY += vmlinux_prereq
-vmlinux_prereq: $(vmlinux-deps) FORCE
-ifdef CONFIG_HEADERS_CHECK
-	$(Q)$(MAKE) -f $(srctree)/Makefile headers_check
-endif
-ifdef CONFIG_GDB_SCRIPTS
-	$(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)
-endif
-ifdef CONFIG_TRIM_UNUSED_KSYMS
-	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh \
-	  "$(MAKE) -f $(srctree)/Makefile vmlinux"
-endif
+# Recurse until adjust_autoksyms.sh is satisfied
+PHONY += autoksyms_recursive
+autoksyms_recursive: $(vmlinux-deps)
+	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/adjust_autoksyms.sh || \
+	$(MAKE) -f $(srctree)/Makefile autoksyms_recursive
 
 # For the kernel to actually contain only the needed exported symbols,
 # we have to build modules as well to determine what those symbols are.
@@ -1023,7 +1013,13 @@  cmd_link-vmlinux =                                                 \
 	$(CONFIG_SHELL) $< $(LD) $(LDFLAGS) $(LDFLAGS_vmlinux) ;    \
 	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
 
-vmlinux: scripts/link-vmlinux.sh vmlinux_prereq $(vmlinux-deps) FORCE
+vmlinux: scripts/link-vmlinux.sh autoksyms_recursive $(vmlinux-deps) FORCE
+ifdef CONFIG_HEADERS_CHECK
+	$(Q)$(MAKE) -f $(srctree)/Makefile headers_check
+endif
+ifdef CONFIG_GDB_SCRIPTS
+	$(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)
+endif
 	+$(call if_changed,link-vmlinux)
 
 # Build samples along the rest of the kernel
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
index 7bb3618..1377cf8 100755
--- a/scripts/adjust_autoksyms.sh
+++ b/scripts/adjust_autoksyms.sh
@@ -95,8 +95,7 @@  if [ $changed -gt 0 ]; then
 	info "KSYMS" "symbols: before=$old, after=$new, changed=$changed"
 	info "UPD" "$cur_ksyms_file"
 	mv -f "$new_ksyms_file" "$cur_ksyms_file"
-	# Then trigger a rebuild of affected source files
-	exec $@
+	exit 1
 else
 	rm -f "$new_ksyms_file"
 fi