diff mbox series

[5/8] kbuild: change if_changed_rule to accept multi-line recipe

Message ID 1542270435-11181-6-git-send-email-yamada.masahiro@socionext.com
State Superseded
Headers show
Series kbuild: clean-up modversion, TRIM_UNUSED_KSYMS, if_changed_rule, etc. | expand

Commit Message

Masahiro Yamada Nov. 15, 2018, 8:27 a.m. UTC 
GNU Make supports 'define' ... 'endef' directive, which can describe
a recipe that consists of multiple lines.

For example,

    all:
            @echo hello
            @echo world

... can be written as:

    define greeting
            @echo hello
            @echo world
    endif

    all:
            $(greeting)

This is useful to confine a series of shell commands into a single
macro, keeping readability.

However, if_changed_rule cannot accept multi-line recipe. As you see
rule_cc_o_c and rule_as_o_S in scripts/Makefile.build, it must be
written like this:

  define rule_cc_o_c
          @[action1] ; \
          [action2] ; \
          [action3]
  endef

This does not actually exploit the benefits of 'define' ... 'endef'
form. All shell commands must be concatenated with '; \' so that it
looks like a single command from the Makefile point of view. '@' can
only appear before the first action.

The root cause of this misfortune is the '@set -e;' in if_changed_rule.
It is easily solvable by moving '@set -e' to the 'cmd' macro.

The combo of $(call echo-cmd,*) $(cmd_*) in rule_cc_o_c and rule_as_o_S
were replaced with $(call cmd,*). The tailing back-slashes went away.

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

---

 scripts/Kbuild.include |  6 ++----
 scripts/Makefile.build | 22 +++++++++++-----------
 2 files changed, 13 insertions(+), 15 deletions(-)

-- 
2.7.4

Comments

Rasmus Villemoes Nov. 15, 2018, 9:12 a.m. UTC | #1 
On 15/11/2018 09.27, Masahiro Yamada wrote:
> GNU Make supports 'define' ... 'endef' directive, which can describe

> a recipe that consists of multiple lines.

> 

>   endef

> 

> This does not actually exploit the benefits of 'define' ... 'endef'

> form. All shell commands must be concatenated with '; \' so that it

> looks like a single command from the Makefile point of view. '@' can

> only appear before the first action.

> 

> The root cause of this misfortune is the '@set -e;' in if_changed_rule.

> It is easily solvable by moving '@set -e' to the 'cmd' macro.

> 

> The combo of $(call echo-cmd,*) $(cmd_*) in rule_cc_o_c and rule_as_o_S

> were replaced with $(call cmd,*). The tailing back-slashes went away.

> 

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

> ---

>  

>  define rule_cc_o_c

> -	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \

> -	$(call cmd_and_fixdep,cc_o_c)					  \

> -	$(cmd_gen_ksymdeps)						  \

> -	$(cmd_checkdoc)							  \

> -	$(call echo-cmd,objtool) $(cmd_objtool)				  \

> -	$(cmd_modversions_c)						  \

> -	$(call echo-cmd,record_mcount) $(cmd_record_mcount)

> +	$(call cmd,checksrc)

> +	@$(call cmd_and_fixdep,cc_o_c)

> +	$(call cmd,gen_ksymdeps)

> +	$(call cmd,checkdoc)

> +	$(call cmd,objtool)

> +	$(call cmd,modversions_c)

> +	$(call cmd,record_mcount)

>  endef


Does this mean that Make now spawns a new shell for each of these
commands, and if so, what's the performance impact? Or am I just
misreading things? If this does change the semantics (one shell instance
versus many), I think that's worth mentioning explicitly in the
changelog, regardless of whether there's no measuarable performance impact.

Rasmus
Masahiro Yamada Nov. 16, 2018, 1:37 a.m. UTC | #2 
On Thu, Nov 15, 2018 at 6:12 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>

> On 15/11/2018 09.27, Masahiro Yamada wrote:

> > GNU Make supports 'define' ... 'endef' directive, which can describe

> > a recipe that consists of multiple lines.

> >

> >   endef

> >

> > This does not actually exploit the benefits of 'define' ... 'endef'

> > form. All shell commands must be concatenated with '; \' so that it

> > looks like a single command from the Makefile point of view. '@' can

> > only appear before the first action.

> >

> > The root cause of this misfortune is the '@set -e;' in if_changed_rule.

> > It is easily solvable by moving '@set -e' to the 'cmd' macro.

> >

> > The combo of $(call echo-cmd,*) $(cmd_*) in rule_cc_o_c and rule_as_o_S

> > were replaced with $(call cmd,*). The tailing back-slashes went away.

> >

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

> > ---

> >

> >  define rule_cc_o_c

> > -     $(call echo-cmd,checksrc) $(cmd_checksrc)                         \

> > -     $(call cmd_and_fixdep,cc_o_c)                                     \

> > -     $(cmd_gen_ksymdeps)                                               \

> > -     $(cmd_checkdoc)                                                   \

> > -     $(call echo-cmd,objtool) $(cmd_objtool)                           \

> > -     $(cmd_modversions_c)                                              \

> > -     $(call echo-cmd,record_mcount) $(cmd_record_mcount)

> > +     $(call cmd,checksrc)

> > +     @$(call cmd_and_fixdep,cc_o_c)

> > +     $(call cmd,gen_ksymdeps)

> > +     $(call cmd,checkdoc)

> > +     $(call cmd,objtool)

> > +     $(call cmd,modversions_c)

> > +     $(call cmd,record_mcount)

> >  endef

>

> Does this mean that Make now spawns a new shell for each of these

> commands,



Yes and No.

Basically, each line is run in an independent sub-shell,
but things are a little bit complex.

GNU Make, if possible, runs the command directly
instead of forking a shell process.

That is what I understood according to the following document:


http://wanderinghorse.net/computing/make/book/ManagingProjectsWithGNUMake-3.1.3.pdf

See "chapter 7: commands"
  ---------->8----------
  Commands are essentially one-line shell scripts.
  In effect, make grabs each line and passes it to a subshell for execution.
  In fact, make can optimize this (relatively) expensive fork/exec algorithm
  if it can guarantee that omitting the shell will not change the behavior of
  the program. It checks this by scanning each command line for shell special
  characters, such as wildcard characters and i/o redirection. If none are
  found, make directly executes the command without passing it to a subshell.
  ---------->8----------





> and if so, what's the performance impact? Or am I just

> misreading things? If this does change the semantics (one shell instance

> versus many), I think that's worth mentioning explicitly in the

> changelog, regardless of whether there's no measuarable performance impact.



Last night, I checked 'perf stat' of x86 defconfig build,
which enables cmd_objtool.



Without this commit:


 Performance counter stats for 'make -j8' (10 runs):

     125.499068713 seconds time elapsed
          ( +-  0.10% )


With this commit:

 Performance counter stats for 'make -j8' (10 runs):

     125.618321667 seconds time elapsed
          ( +-  0.24% )



I did not get noticeable performance regression.



-- 
Best Regards
Masahiro Yamada
diff mbox series

Patch

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 1eafc85..5e47bf6 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -215,7 +215,7 @@  echo-cmd = $(if $($(quiet)cmd_$(1)),\
 	echo '  $(call escsq,$($(quiet)cmd_$(1)))$(echo-why)';)
 
 # printing commands
-cmd = @$(echo-cmd) $(cmd_$(1))
+cmd = @set -e; $(echo-cmd) $(cmd_$(1))
 
 # Add $(obj)/ for paths that are not absolute
 objectify = $(foreach o,$(1),$(if $(filter /%,$(o)),$(o),$(obj)/$(o)))
@@ -269,9 +269,7 @@  cmd_and_fixdep =                                                             \
 # Usage: $(call if_changed_rule,foo)
 # Will check if $(cmd_foo) or any of the prerequisites changed,
 # and if so will execute $(rule_foo).
-if_changed_rule = $(if $(strip $(any-prereq) $(arg-check) ),                 \
-	@set -e;                                                             \
-	$(rule_$(1)), @:)
+if_changed_rule = $(if $(strip $(any-prereq) $(arg-check)), $(rule_$(1)), @:)
 
 ###
 # why - tell why a target got built
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index e5ba9b1..5528a6a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -263,20 +263,20 @@  cmd_gen_ksymdeps = \
 endif
 
 define rule_cc_o_c
-	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
-	$(call cmd_and_fixdep,cc_o_c)					  \
-	$(cmd_gen_ksymdeps)						  \
-	$(cmd_checkdoc)							  \
-	$(call echo-cmd,objtool) $(cmd_objtool)				  \
-	$(cmd_modversions_c)						  \
-	$(call echo-cmd,record_mcount) $(cmd_record_mcount)
+	$(call cmd,checksrc)
+	@$(call cmd_and_fixdep,cc_o_c)
+	$(call cmd,gen_ksymdeps)
+	$(call cmd,checkdoc)
+	$(call cmd,objtool)
+	$(call cmd,modversions_c)
+	$(call cmd,record_mcount)
 endef
 
 define rule_as_o_S
-	$(call cmd_and_fixdep,as_o_S)					  \
-	$(cmd_gen_ksymdeps)						  \
-	$(call echo-cmd,objtool) $(cmd_objtool)				  \
-	$(cmd_modversions_S)
+	@$(call cmd_and_fixdep,as_o_S)
+	$(call cmd,gen_ksymdeps)
+	$(call cmd,objtool)
+	$(call cmd,modversions_S)
 endef
 
 # List module undefined symbols (or empty line if not enabled)