[PATCH 0/8] kbuild: clean-up modversion, TRIM_UNUSED_KSYMS, if_changed_rule, etc.

[PATCH 0/8] kbuild: clean-up modversion, TRIM_UNUSED_KSYMS, if_changed_rule, etc.

[Masahiro Yamada]

As a Kbuild maintainer, I always struggle to keep the core makefiles
clean because people tend to squeeze more and more clutter code into
the kbuild core in order to do what they want to do.

The biggest step forward in this series is to re-implement
the build trick of CONFIG_TRIM_UNUSED_KSYMS in a cleaner way.
scripts/Kbuild.include now looks nice again.
Also, in my rough estimation, building with CONFIG_TRIM_UNUSED_KSYMS
became 40-50 % faster.

Besides those, nice cleanups are here and there.



Masahiro Yamada (8):
  kbuild: remove redundant 'set -e' from filechk_* defines
  kbuild: remove redundant 'set -e' from sub_cmd_record_mcount
  kbuild: refactor modversions build rules
  kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS
  kbuild: change if_changed_rule to accept multi-line recipe
  kbuild: remove trailing semicolon from cmd_* passed to if_changed_rule
  kbuild: refactor if_changed and if_changed_dep
  kbuild: remove redundant 'set -e' from cmd_* defines

 arch/um/Makefile             |  2 +-
 include/asm-generic/export.h | 13 +++---
 include/linux/export.h       | 18 ++++----
 scripts/Kbuild.include       | 44 +++-----------------
 scripts/Makefile.build       | 98 +++++++++++++++++++-------------------------
 scripts/Makefile.lib         |  2 +-
 scripts/basic/fixdep.c       | 31 ++------------
 scripts/gen_ksymdeps.sh      | 25 +++++++++++
 scripts/package/Makefile     |  1 -
 9 files changed, 96 insertions(+), 138 deletions(-)
 create mode 100755 scripts/gen_ksymdeps.sh

-- 
2.7.4

[PATCH 1/8] kbuild: remove redundant 'set -e' from filechk_* defines

[Masahiro Yamada]

The filechk macro in scripts/Kbuild.include already sets 'set -e'.

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

 arch/um/Makefile     | 2 +-
 scripts/Makefile.lib | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/Makefile b/arch/um/Makefile
index ab1066c..71ff3d0 100644
--- a/arch/um/Makefile
+++ b/arch/um/Makefile
@@ -152,7 +152,7 @@ $(HOST_DIR)/um/user-offsets.s: __headers FORCE
 	$(Q)$(MAKE) $(build)=$(HOST_DIR)/um $@
 
 define filechk_gen-asm-offsets
-        (set -e; \
+        ( \
          echo "/*"; \
          echo " * DO NOT MODIFY."; \
          echo " *"; \
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 8fe4468..ae3ae97 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -406,7 +406,7 @@ endef
 # Use filechk to avoid rebuilds when a header changes, but the resulting file
 # does not
 define filechk_offsets
-	(set -e; \
+	( \
 	 echo "#ifndef $2"; \
 	 echo "#define $2"; \
 	 echo "/*"; \
-- 
2.7.4

[PATCH 2/8] kbuild: remove redundant 'set -e' from sub_cmd_record_mcount

[Masahiro Yamada]

This is run inside the if_changed_rule, which already sets 'set -e'.

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

 scripts/Makefile.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a8e7ba9..7af21a8 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -204,7 +204,7 @@ sub_cmd_record_mcount =					\
 recordmcount_source := $(srctree)/scripts/recordmcount.c \
 		    $(srctree)/scripts/recordmcount.h
 else
-sub_cmd_record_mcount = set -e ; perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
+sub_cmd_record_mcount = perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
 	"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
 	"$(if $(CONFIG_64BIT),64,32)" \
 	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC) $(KBUILD_CPPFLAGS) $(KBUILD_CFLAGS)" \
-- 
2.7.4

[PATCH 3/8] kbuild: refactor modversions build rules

[Masahiro Yamada]

Let $(CC) compile objects into normal files *.o instead of .tmp_*.o
whether CONFIG_MODVERSIONS is enabled or not. This will help simplify
build rules a lot.

I guess the reason of using .tmp_*.o for intermediate objects was
to avoid leaving incomplete *.o file (, whose timestamp says it is
up-to-date) when the genksyms tool failed for some reasons.

It no longer matters because any targets are deleted on errors since
commit 9c2af1c7377a ("kbuild: add .DELETE_ON_ERROR special target").

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

 scripts/Makefile.build | 54 ++++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 37 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 7af21a8..7f3ca6e 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -154,35 +154,30 @@ $(obj)/%.ll: $(src)/%.c FORCE
 # (See cmd_cc_o_c + relevant part of rule_cc_o_c)
 
 quiet_cmd_cc_o_c = CC $(quiet_modtag)  $@
+      cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $<
 
-ifndef CONFIG_MODVERSIONS
-cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $<
-
-else
+ifdef CONFIG_MODVERSIONS
 # When module versioning is enabled the following steps are executed:
-# o compile a .tmp_<file>.o from <file>.c
-# o if .tmp_<file>.o doesn't contain a __ksymtab version, i.e. does
-#   not export symbols, we just rename .tmp_<file>.o to <file>.o and
-#   are done.
+# o compile a <file>.o from <file>.c
+# o if <file>.o doesn't contain a __ksymtab version, i.e. does
+#   not export symbols, it's done.
 # o otherwise, we calculate symbol versions using the good old
 #   genksyms on the preprocessed source and postprocess them in a way
 #   that they are usable as a linker script
-# o generate <file>.o from .tmp_<file>.o using the linker to
+# o generate .tmp_<file>.o from <file>.o using the linker to
 #   replace the unresolved symbols __crc_exported_symbol with
 #   the actual value of the checksum generated by genksyms
-
-cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
+# o remove .tmp_<file>.o to <file>.o
 
 cmd_modversions_c =								\
-	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\
+	if $(OBJDUMP) -h $@ | grep -q __ksymtab; then				\
 		$(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
 		    > $(@D)/.tmp_$(@F:.o=.ver);					\
 										\
-		$(LD) $(KBUILD_LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) 		\
+		$(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@ 		\
 			-T $(@D)/.tmp_$(@F:.o=.ver);				\
-		rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver);		\
-	else									\
 		mv -f $(@D)/.tmp_$(@F) $@;					\
+		rm -f $(@D)/.tmp_$(@F:.o=.ver);					\
 	fi;
 endif
 
@@ -241,19 +236,12 @@ ifneq ($(RETPOLINE_CFLAGS),)
 endif
 endif
 
-
-ifdef CONFIG_MODVERSIONS
-objtool_o = $(@D)/.tmp_$(@F)
-else
-objtool_o = $(@)
-endif
-
 # 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory
 # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file
 # 'OBJECT_FILES_NON_STANDARD_foo.o := 'n': override directory skip for a file
 cmd_objtool = $(if $(patsubst y%,, \
 	$(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
-	$(__objtool_obj) $(objtool_args) "$(objtool_o)";)
+	$(__objtool_obj) $(objtool_args) $@;)
 objtool_obj = $(if $(patsubst y%,, \
 	$(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
 	$(__objtool_obj))
@@ -357,34 +345,26 @@ $(obj)/%.s: $(src)/%.S FORCE
 	$(call if_changed_dep,cpp_s_S)
 
 quiet_cmd_as_o_S = AS $(quiet_modtag)  $@
+      cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
 
-ifndef CONFIG_MODVERSIONS
-cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
-
-else
+ifdef CONFIG_MODVERSIONS
 
 ASM_PROTOTYPES := $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/asm-prototypes.h)
 
-ifeq ($(ASM_PROTOTYPES),)
-cmd_as_o_S = $(CC) $(a_flags) -c -o $@ $<
-
-else
+ifneq ($(ASM_PROTOTYPES),)
 
 # versioning matches the C process described above, with difference that
 # we parse asm-prototypes.h C header to get function definitions.
 
-cmd_as_o_S = $(CC) $(a_flags) -c -o $(@D)/.tmp_$(@F) $<
-
 cmd_modversions_S =								\
-	if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then		\
+	if $(OBJDUMP) -h $@ | grep -q __ksymtab; then				\
 		$(call cmd_gensymtypes_S,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
 		    > $(@D)/.tmp_$(@F:.o=.ver);					\
 										\
-		$(LD) $(KBUILD_LDFLAGS) -r -o $@ $(@D)/.tmp_$(@F) 		\
+		$(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@ 		\
 			-T $(@D)/.tmp_$(@F:.o=.ver);				\
-		rm -f $(@D)/.tmp_$(@F) $(@D)/.tmp_$(@F:.o=.ver);		\
-	else									\
 		mv -f $(@D)/.tmp_$(@F) $@;					\
+		rm -f $(@D)/.tmp_$(@F:.o=.ver);					\
 	fi;
 endif
 endif
-- 
2.7.4

Re: [PATCH 3/8] kbuild: refactor modversions build rules

[Sam Ravnborg]

Hi Masahiro

On Thu, Nov 15, 2018 at 05:27:10PM +0900, Masahiro Yamada wrote:
> Let $(CC) compile objects into normal files *.o instead of .tmp_*.o
> whether CONFIG_MODVERSIONS is enabled or not. This will help simplify
> build rules a lot.

Another approach would be to move more of the build stuff to
one or a few build scripts.

Using build scripts makes is much easier to add comments and
still keep it readable.
And when the build logic list a set of serialized actions
they thay can be included in a build script nicely.

But that said - I also like the simplifications you made.
Everytime you trim the core kbuild files it is a win for
readability.

	Sam

Re: [PATCH 3/8] kbuild: refactor modversions build rules

[Masahiro Yamada]

Hi Sam,


On Sat, Nov 17, 2018 at 5:01 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Masahiro
>
> On Thu, Nov 15, 2018 at 05:27:10PM +0900, Masahiro Yamada wrote:
> > Let $(CC) compile objects into normal files *.o instead of .tmp_*.o
> > whether CONFIG_MODVERSIONS is enabled or not. This will help simplify
> > build rules a lot.
>
> Another approach would be to move more of the build stuff to
> one or a few build scripts.

Shifting code from Makefile to a shell script
will be another clean-up.


This patch is addressing a different problem.

See this code in Malefile.build

ifdef CONFIG_MODVERSIONS
objtool_o = $(@D)/.tmp_$(@F)
else
objtool_o = $(@)
endif



cmd_cc_o_c writes an object into a different file name
depending CONFIG_MODVERSIONS, which makes difficult
for other actions to manipulate the object in a
consistent manner.

I will clarify this in the commit log in v2.

Thanks.





> Using build scripts makes is much easier to add comments and
> still keep it readable.
> And when the build logic list a set of serialized actions
> they thay can be included in a build script nicely.
>
> But that said - I also like the simplifications you made.
> Everytime you trim the core kbuild files it is a win for
> readability.
>
>         Sam



-- 
Best Regards
Masahiro Yamada

[PATCH 4/8] kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS

[Masahiro Yamada]

My main motivation of this commit is to clean up scripts/Kbuild.include
and scripts/Makefile.build.

Currently, CONFIG_TRIM_UNUSED_KSYMS works with a tricky gimmick;
possibly exported symbols are detected by letting $(CPP) replace
EXPORT_SYMBOL* with a special string '=== __KSYM_*===', which is
post-processed by sed, and passed to fixdep. The extra preprocessing
is costly, and hacking cmd_and_fixdep is ugly.

I came up with a new way to find exported symbols; insert a dummy
symbol __ksym_marker_* to each potentially exported symbol. Those
dummy symbols are picked up by $(NM), post-processed by sed, then
appended to .*.cmd files. I collected the post-process part to a
new shell script scripts/gen_ksymdeps.sh for readability. The dummy
symbols are put into the .discard.* section so that the linker
script rips them off the final vmlinux or modules.

A nice side-effect is building with CONFIG_TRIM_UNUSED_KSYMS will
be much faster.

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

 include/asm-generic/export.h | 13 ++++++++-----
 include/linux/export.h       | 18 +++++++++---------
 scripts/Kbuild.include       | 29 -----------------------------
 scripts/Makefile.build       | 10 ++++++++++
 scripts/basic/fixdep.c       | 31 ++++---------------------------
 scripts/gen_ksymdeps.sh      | 25 +++++++++++++++++++++++++
 6 files changed, 56 insertions(+), 70 deletions(-)
 create mode 100755 scripts/gen_ksymdeps.sh

diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
index 4d73e6e..294d6ae 100644
--- a/include/asm-generic/export.h
+++ b/include/asm-generic/export.h
@@ -59,16 +59,19 @@ __kcrctab_\name:
 .endm
 #undef __put
 
-#if defined(__KSYM_DEPS__)
-
-#define __EXPORT_SYMBOL(sym, val, sec)	=== __KSYM_##sym ===
-
-#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
+#if defined(CONFIG_TRIM_UNUSED_KSYMS)
 
 #include <linux/kconfig.h>
 #include <generated/autoksyms.h>
 
+.macro __ksym_marker sym
+	.section ".discard.ksym","a"
+__ksym_marker_\sym:
+	 .previous
+.endm
+
 #define __EXPORT_SYMBOL(sym, val, sec)				\
+	__ksym_marker sym;					\
 	__cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
 #define __cond_export_sym(sym, val, sec, conf)			\
 	___cond_export_sym(sym, val, sec, conf)
diff --git a/include/linux/export.h b/include/linux/export.h
index ce764a5..0413a3d 100644
--- a/include/linux/export.h
+++ b/include/linux/export.h
@@ -92,22 +92,22 @@ struct kernel_symbol {
  */
 #define __EXPORT_SYMBOL(sym, sec)
 
-#elif defined(__KSYM_DEPS__)
+#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
+
+#include <generated/autoksyms.h>
 
 /*
  * For fine grained build dependencies, we want to tell the build system
  * about each possible exported symbol even if they're not actually exported.
- * We use a string pattern that is unlikely to be valid code that the build
- * system filters out from the preprocessor output (see ksym_dep_filter
- * in scripts/Kbuild.include).
+ * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
+ * from the $(NM) output (see scripts/gen_ksymdep.sh). These symbols are
+ * discarded in the final link stage.
  */
-#define __EXPORT_SYMBOL(sym, sec)	=== __KSYM_##sym ===
-
-#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
-
-#include <generated/autoksyms.h>
+#define __ksym_marker(sym)	\
+	static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
 
 #define __EXPORT_SYMBOL(sym, sec)				\
+	__ksym_marker(sym);					\
 	__cond_export_sym(sym, sec, __is_defined(__KSYM_##sym))
 #define __cond_export_sym(sym, sec, conf)			\
 	___cond_export_sym(sym, sec, conf)
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index bb01555..1eafc85 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -260,41 +260,12 @@ if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ),                  \
 	@set -e;                                                             \
 	$(cmd_and_fixdep), @:)
 
-ifndef CONFIG_TRIM_UNUSED_KSYMS
-
 cmd_and_fixdep =                                                             \
 	$(echo-cmd) $(cmd_$(1));                                             \
 	scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
 	rm -f $(depfile);                                                    \
 	mv -f $(dot-target).tmp $(dot-target).cmd;
 
-else
-
-# Filter out exported kernel symbol names from the preprocessor output.
-# See also __KSYM_DEPS__ in include/linux/export.h.
-# We disable the depfile generation here, so as not to overwrite the existing
-# depfile while fixdep is parsing it.
-flags_nodeps = $(filter-out -Wp$(comma)-M%, $($(1)))
-ksym_dep_filter =                                                            \
-	case "$(1)" in                                                       \
-	  cc_*_c|cpp_i_c)                                                    \
-	    $(CPP) $(call flags_nodeps,c_flags) -D__KSYM_DEPS__ $< ;;        \
-	  as_*_S|cpp_s_S)                                                    \
-	    $(CPP) $(call flags_nodeps,a_flags) -D__KSYM_DEPS__ $< ;;        \
-	  boot*|build*|cpp_its_S|*cpp_lds_S|dtc|host*|vdso*) : ;;            \
-	  *) echo "Don't know how to preprocess $(1)" >&2; false ;;          \
-	esac | tr ";" "\n" | sed -n 's/^.*=== __KSYM_\(.*\) ===.*$$/_\1/p'
-
-cmd_and_fixdep =                                                             \
-	$(echo-cmd) $(cmd_$(1));                                             \
-	$(ksym_dep_filter) |                                                 \
-		scripts/basic/fixdep -e $(depfile) $@ '$(make-cmd)'          \
-			> $(dot-target).tmp;	                             \
-	rm -f $(depfile);                                                    \
-	mv -f $(dot-target).tmp $(dot-target).cmd;
-
-endif
-
 # Usage: $(call if_changed_rule,foo)
 # Will check if $(cmd_foo) or any of the prerequisites changed,
 # and if so will execute $(rule_foo).
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 7f3ca6e..e5ba9b1 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -254,9 +254,18 @@ objtool_dep = $(objtool_obj)					\
 	      $(wildcard include/config/orc/unwinder.h		\
 			 include/config/stack/validation.h)
 
+ifdef CONFIG_TRIM_UNUSED_KSYMS
+cmd_gen_ksymdeps = \
+	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
+	cat $(dot-target).tmp >> $(dot-target).cmd; \
+	rm -f $(dot-target).tmp;
+
+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)						  \
@@ -265,6 +274,7 @@ 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)
 endef
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 850966f3..facbd60 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -105,8 +105,7 @@
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: fixdep [-e] <depfile> <target> <cmdline>\n");
-	fprintf(stderr, " -e  insert extra dependencies given on stdin\n");
+	fprintf(stderr, "Usage: fixdep <depfile> <target> <cmdline>\n");
 	exit(1);
 }
 
@@ -131,21 +130,6 @@ static void print_dep(const char *m, int slen, const char *dir)
 	printf(".h) \\\n");
 }
 
-static void do_extra_deps(void)
-{
-	char buf[80];
-
-	while (fgets(buf, sizeof(buf), stdin)) {
-		int len = strlen(buf);
-
-		if (len < 2 || buf[len - 1] != '\n') {
-			fprintf(stderr, "fixdep: bad data on stdin\n");
-			exit(1);
-		}
-		print_dep(buf, len - 1, "include/ksym");
-	}
-}
-
 struct item {
 	struct item	*next;
 	unsigned int	len;
@@ -293,7 +277,7 @@ static int is_ignored_file(const char *s, int len)
  * assignments are parsed not only by make, but also by the rather simple
  * parser in scripts/mod/sumversion.c.
  */
-static void parse_dep_file(char *m, const char *target, int insert_extra_deps)
+static void parse_dep_file(char *m, const char *target)
 {
 	char *p;
 	int is_last, is_target;
@@ -369,9 +353,6 @@ static void parse_dep_file(char *m, const char *target, int insert_extra_deps)
 		exit(1);
 	}
 
-	if (insert_extra_deps)
-		do_extra_deps();
-
 	printf("\n%s: $(deps_%s)\n\n", target, target);
 	printf("$(deps_%s):\n", target);
 }
@@ -379,13 +360,9 @@ static void parse_dep_file(char *m, const char *target, int insert_extra_deps)
 int main(int argc, char *argv[])
 {
 	const char *depfile, *target, *cmdline;
-	int insert_extra_deps = 0;
 	void *buf;
 
-	if (argc == 5 && !strcmp(argv[1], "-e")) {
-		insert_extra_deps = 1;
-		argv++;
-	} else if (argc != 4)
+	if (argc != 4)
 		usage();
 
 	depfile = argv[1];
@@ -395,7 +372,7 @@ int main(int argc, char *argv[])
 	printf("cmd_%s := %s\n\n", target, cmdline);
 
 	buf = read_file(depfile);
-	parse_dep_file(buf, target, insert_extra_deps);
+	parse_dep_file(buf, target);
 	free(buf);
 
 	return 0;
diff --git a/scripts/gen_ksymdeps.sh b/scripts/gen_ksymdeps.sh
new file mode 100755
index 0000000..1324986
--- /dev/null
+++ b/scripts/gen_ksymdeps.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+# List of exported symbols
+ksyms=$($NM $1 | sed -n 's/.*__ksym_marker_\(.*\)/\1/p' | tr A-Z a-z)
+
+if [ -z "$ksyms" ]; then
+	exit 0
+fi
+
+echo
+echo "ksymdeps_$1 := \\"
+
+for s in $ksyms
+do
+	echo $s | sed -e 's:^_*:    $(wildcard include/ksym/:' \
+			-e 's:__*:/:g' -e 's/$/.h) \\/'
+done
+
+echo
+echo "$1: \$(ksymdeps_$1)"
+echo
+echo "\$(ksymdeps_$1):"
-- 
2.7.4

Re: [PATCH 4/8] kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS

[Nicolas Pitre]

On Thu, 15 Nov 2018, Masahiro Yamada wrote:

> My main motivation of this commit is to clean up scripts/Kbuild.include
> and scripts/Makefile.build.
> 
> Currently, CONFIG_TRIM_UNUSED_KSYMS works with a tricky gimmick;
> possibly exported symbols are detected by letting $(CPP) replace
> EXPORT_SYMBOL* with a special string '=== __KSYM_*===', which is
> post-processed by sed, and passed to fixdep. The extra preprocessing
> is costly, and hacking cmd_and_fixdep is ugly.
> 
> I came up with a new way to find exported symbols; insert a dummy
> symbol __ksym_marker_* to each potentially exported symbol. Those
> dummy symbols are picked up by $(NM), post-processed by sed, then
> appended to .*.cmd files. I collected the post-process part to a
> new shell script scripts/gen_ksymdeps.sh for readability. The dummy
> symbols are put into the .discard.* section so that the linker
> script rips them off the final vmlinux or modules.

Brilliant!  I really like it.

Minor comments below.

> diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> index 4d73e6e..294d6ae 100644
> --- a/include/asm-generic/export.h
> +++ b/include/asm-generic/export.h
> @@ -59,16 +59,19 @@ __kcrctab_\name:
>  .endm
>  #undef __put
>  
> -#if defined(__KSYM_DEPS__)
> -
> -#define __EXPORT_SYMBOL(sym, val, sec)	=== __KSYM_##sym ===
> -
> -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> +#if defined(CONFIG_TRIM_UNUSED_KSYMS)
>  
>  #include <linux/kconfig.h>
>  #include <generated/autoksyms.h>
>  
> +.macro __ksym_marker sym
> +	.section ".discard.ksym","a"
> +__ksym_marker_\sym:
> +	 .previous

Does this work as intended? I have vague memories about having problems 
with sections being discarded when they don't allocate any space.

> +.endm
> +
>  #define __EXPORT_SYMBOL(sym, val, sec)				\
> +	__ksym_marker sym;					\
>  	__cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
>  #define __cond_export_sym(sym, val, sec, conf)			\
>  	___cond_export_sym(sym, val, sec, conf)
> diff --git a/include/linux/export.h b/include/linux/export.h
> index ce764a5..0413a3d 100644
> --- a/include/linux/export.h
> +++ b/include/linux/export.h
> @@ -92,22 +92,22 @@ struct kernel_symbol {
>   */
>  #define __EXPORT_SYMBOL(sym, sec)
>  
> -#elif defined(__KSYM_DEPS__)
> +#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> +
> +#include <generated/autoksyms.h>
>  
>  /*
>   * For fine grained build dependencies, we want to tell the build system
>   * about each possible exported symbol even if they're not actually exported.
> - * We use a string pattern that is unlikely to be valid code that the build
> - * system filters out from the preprocessor output (see ksym_dep_filter
> - * in scripts/Kbuild.include).
> + * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
> + * from the $(NM) output (see scripts/gen_ksymdep.sh). These symbols are
> + * discarded in the final link stage.
>   */
> -#define __EXPORT_SYMBOL(sym, sec)	=== __KSYM_##sym ===
> -
> -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> -
> -#include <generated/autoksyms.h>
> +#define __ksym_marker(sym)	\
> +	static int __ksym_marker_##sym[0] __section(".discard.ksym") __used

Even if this is discarded during the final link, maybe this could save 
a tiny amount of disk space by using a char instead?

> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 7f3ca6e..e5ba9b1 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -254,9 +254,18 @@ objtool_dep = $(objtool_obj)					\
>  	      $(wildcard include/config/orc/unwinder.h		\
>  			 include/config/stack/validation.h)
>  
> +ifdef CONFIG_TRIM_UNUSED_KSYMS
> +cmd_gen_ksymdeps = \
> +	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
> +	cat $(dot-target).tmp >> $(dot-target).cmd; \
> +	rm -f $(dot-target).tmp;

Why don't you append to $(dot-target).cmd directly?


Nicolas

Re: [PATCH 4/8] kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS

[Masahiro Yamada]

On Fri, Nov 16, 2018 at 2:13 PM Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
>
> On Thu, 15 Nov 2018, Masahiro Yamada wrote:
>
> > My main motivation of this commit is to clean up scripts/Kbuild.include
> > and scripts/Makefile.build.
> >
> > Currently, CONFIG_TRIM_UNUSED_KSYMS works with a tricky gimmick;
> > possibly exported symbols are detected by letting $(CPP) replace
> > EXPORT_SYMBOL* with a special string '=== __KSYM_*===', which is
> > post-processed by sed, and passed to fixdep. The extra preprocessing
> > is costly, and hacking cmd_and_fixdep is ugly.
> >
> > I came up with a new way to find exported symbols; insert a dummy
> > symbol __ksym_marker_* to each potentially exported symbol. Those
> > dummy symbols are picked up by $(NM), post-processed by sed, then
> > appended to .*.cmd files. I collected the post-process part to a
> > new shell script scripts/gen_ksymdeps.sh for readability. The dummy
> > symbols are put into the .discard.* section so that the linker
> > script rips them off the final vmlinux or modules.
>
> Brilliant!  I really like it.
>
> Minor comments below.
>
> > diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> > index 4d73e6e..294d6ae 100644
> > --- a/include/asm-generic/export.h
> > +++ b/include/asm-generic/export.h
> > @@ -59,16 +59,19 @@ __kcrctab_\name:
> >  .endm
> >  #undef __put
> >
> > -#if defined(__KSYM_DEPS__)
> > -
> > -#define __EXPORT_SYMBOL(sym, val, sec)       === __KSYM_##sym ===
> > -
> > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > +#if defined(CONFIG_TRIM_UNUSED_KSYMS)
> >
> >  #include <linux/kconfig.h>
> >  #include <generated/autoksyms.h>
> >
> > +.macro __ksym_marker sym
> > +     .section ".discard.ksym","a"
> > +__ksym_marker_\sym:
> > +      .previous
>
> Does this work as intended? I have vague memories about having problems
> with sections being discarded when they don't allocate any space.

What I can tell is, this patch produces the same size kernel
(after dropping debug info by 'strip' command).


> > +.endm
> > +
> >  #define __EXPORT_SYMBOL(sym, val, sec)                               \
> > +     __ksym_marker sym;                                      \
> >       __cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
> >  #define __cond_export_sym(sym, val, sec, conf)                       \
> >       ___cond_export_sym(sym, val, sec, conf)
> > diff --git a/include/linux/export.h b/include/linux/export.h
> > index ce764a5..0413a3d 100644
> > --- a/include/linux/export.h
> > +++ b/include/linux/export.h
> > @@ -92,22 +92,22 @@ struct kernel_symbol {
> >   */
> >  #define __EXPORT_SYMBOL(sym, sec)
> >
> > -#elif defined(__KSYM_DEPS__)
> > +#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > +
> > +#include <generated/autoksyms.h>
> >
> >  /*
> >   * For fine grained build dependencies, we want to tell the build system
> >   * about each possible exported symbol even if they're not actually exported.
> > - * We use a string pattern that is unlikely to be valid code that the build
> > - * system filters out from the preprocessor output (see ksym_dep_filter
> > - * in scripts/Kbuild.include).
> > + * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
> > + * from the $(NM) output (see scripts/gen_ksymdep.sh). These symbols are
> > + * discarded in the final link stage.
> >   */
> > -#define __EXPORT_SYMBOL(sym, sec)    === __KSYM_##sym ===
> > -
> > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > -
> > -#include <generated/autoksyms.h>
> > +#define __ksym_marker(sym)   \
> > +     static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
>
> Even if this is discarded during the final link, maybe this could save
> a tiny amount of disk space by using a char instead?


I am afraid you missed '[0]' after the symbol name.
This is actually zero-length array.

No memory allocated for this dummy section.

As far as I tested, this is working.





> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index 7f3ca6e..e5ba9b1 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -254,9 +254,18 @@ objtool_dep = $(objtool_obj)                                     \
> >             $(wildcard include/config/orc/unwinder.h          \
> >                        include/config/stack/validation.h)
> >
> > +ifdef CONFIG_TRIM_UNUSED_KSYMS
> > +cmd_gen_ksymdeps = \
> > +     $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
> > +     cat $(dot-target).tmp >> $(dot-target).cmd; \
> > +     rm -f $(dot-target).tmp;
>
> Why don't you append to $(dot-target).cmd directly?


If scripts/gen_ksymdeps.sh fails for some reasons,
it will error out immediately thanks to 'set -e' flag.

Appending incomplete portion might end up with a corrupted .*.cmd file.

Probably, that would not happen, but I just wanted to ensure it.




>
> Nicolas



--
Best Regards
Masahiro Yamada

Re: [PATCH 4/8] kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS

[Nicolas Pitre]

On Fri, 16 Nov 2018, Masahiro Yamada wrote:

> On Fri, Nov 16, 2018 at 2:13 PM Nicolas Pitre <nicolas.pitre@linaro.org> wrote:
> >
> > On Thu, 15 Nov 2018, Masahiro Yamada wrote:
> >
> > > My main motivation of this commit is to clean up scripts/Kbuild.include
> > > and scripts/Makefile.build.
> > >
> > > Currently, CONFIG_TRIM_UNUSED_KSYMS works with a tricky gimmick;
> > > possibly exported symbols are detected by letting $(CPP) replace
> > > EXPORT_SYMBOL* with a special string '=== __KSYM_*===', which is
> > > post-processed by sed, and passed to fixdep. The extra preprocessing
> > > is costly, and hacking cmd_and_fixdep is ugly.
> > >
> > > I came up with a new way to find exported symbols; insert a dummy
> > > symbol __ksym_marker_* to each potentially exported symbol. Those
> > > dummy symbols are picked up by $(NM), post-processed by sed, then
> > > appended to .*.cmd files. I collected the post-process part to a
> > > new shell script scripts/gen_ksymdeps.sh for readability. The dummy
> > > symbols are put into the .discard.* section so that the linker
> > > script rips them off the final vmlinux or modules.
> >
> > Brilliant!  I really like it.
> >
> > Minor comments below.
> >
> > > diff --git a/include/asm-generic/export.h b/include/asm-generic/export.h
> > > index 4d73e6e..294d6ae 100644
> > > --- a/include/asm-generic/export.h
> > > +++ b/include/asm-generic/export.h
> > > @@ -59,16 +59,19 @@ __kcrctab_\name:
> > >  .endm
> > >  #undef __put
> > >
> > > -#if defined(__KSYM_DEPS__)
> > > -
> > > -#define __EXPORT_SYMBOL(sym, val, sec)       === __KSYM_##sym ===
> > > -
> > > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > > +#if defined(CONFIG_TRIM_UNUSED_KSYMS)
> > >
> > >  #include <linux/kconfig.h>
> > >  #include <generated/autoksyms.h>
> > >
> > > +.macro __ksym_marker sym
> > > +     .section ".discard.ksym","a"
> > > +__ksym_marker_\sym:
> > > +      .previous
> >
> > Does this work as intended? I have vague memories about having problems
> > with sections being discarded when they don't allocate any space.
> 
> What I can tell is, this patch produces the same size kernel
> (after dropping debug info by 'strip' command).
> 
> 
> > > +.endm
> > > +
> > >  #define __EXPORT_SYMBOL(sym, val, sec)                               \
> > > +     __ksym_marker sym;                                      \
> > >       __cond_export_sym(sym, val, sec, __is_defined(__KSYM_##sym))
> > >  #define __cond_export_sym(sym, val, sec, conf)                       \
> > >       ___cond_export_sym(sym, val, sec, conf)
> > > diff --git a/include/linux/export.h b/include/linux/export.h
> > > index ce764a5..0413a3d 100644
> > > --- a/include/linux/export.h
> > > +++ b/include/linux/export.h
> > > @@ -92,22 +92,22 @@ struct kernel_symbol {
> > >   */
> > >  #define __EXPORT_SYMBOL(sym, sec)
> > >
> > > -#elif defined(__KSYM_DEPS__)
> > > +#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > > +
> > > +#include <generated/autoksyms.h>
> > >
> > >  /*
> > >   * For fine grained build dependencies, we want to tell the build system
> > >   * about each possible exported symbol even if they're not actually exported.
> > > - * We use a string pattern that is unlikely to be valid code that the build
> > > - * system filters out from the preprocessor output (see ksym_dep_filter
> > > - * in scripts/Kbuild.include).
> > > + * We use a symbol pattern __ksym_marker_<symbol> that the build system filters
> > > + * from the $(NM) output (see scripts/gen_ksymdep.sh). These symbols are
> > > + * discarded in the final link stage.
> > >   */
> > > -#define __EXPORT_SYMBOL(sym, sec)    === __KSYM_##sym ===
> > > -
> > > -#elif defined(CONFIG_TRIM_UNUSED_KSYMS)
> > > -
> > > -#include <generated/autoksyms.h>
> > > +#define __ksym_marker(sym)   \
> > > +     static int __ksym_marker_##sym[0] __section(".discard.ksym") __used
> >
> > Even if this is discarded during the final link, maybe this could save
> > a tiny amount of disk space by using a char instead?
> 
> 
> I am afraid you missed '[0]' after the symbol name.
> This is actually zero-length array.

Ah, indeed.

> No memory allocated for this dummy section.

Right, and that makes it identical to the assembly case.

> As far as I tested, this is working.

Yes, it does.

The problem I was alluding to has to do with symbols generated by the 
linker directly which for some reason behaves differently in that case.

> > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > index 7f3ca6e..e5ba9b1 100644
> > > --- a/scripts/Makefile.build
> > > +++ b/scripts/Makefile.build
> > > @@ -254,9 +254,18 @@ objtool_dep = $(objtool_obj)                                     \
> > >             $(wildcard include/config/orc/unwinder.h          \
> > >                        include/config/stack/validation.h)
> > >
> > > +ifdef CONFIG_TRIM_UNUSED_KSYMS
> > > +cmd_gen_ksymdeps = \
> > > +     $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
> > > +     cat $(dot-target).tmp >> $(dot-target).cmd; \
> > > +     rm -f $(dot-target).tmp;
> >
> > Why don't you append to $(dot-target).cmd directly?
> 
> 
> If scripts/gen_ksymdeps.sh fails for some reasons,
> it will error out immediately thanks to 'set -e' flag.
> 
> Appending incomplete portion might end up with a corrupted .*.cmd file.
> 
> Probably, that would not happen, but I just wanted to ensure it.

Well, strictly speaking, if scripts/gen_ksymdeps.sh fails and its output 
isn't appended at all to the .*.cmd file, then that .*.cmd file is 
already corrupted as it is missing necessary dependencies. Would be 
better to delete the .*.cmd file entirely in that case.


Nicolas

Re: [PATCH 4/8] kbuild: simplify dependency generation for CONFIG_TRIM_UNUSED_KSYMS

[Masahiro Yamada]

Hi Nicolas,


On Sat, Nov 17, 2018 at 2:50 AM Nicolas Pitre <nicolas.pitre@linaro.org> wrote:

> > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > > index 7f3ca6e..e5ba9b1 100644
> > > > --- a/scripts/Makefile.build
> > > > +++ b/scripts/Makefile.build
> > > > @@ -254,9 +254,18 @@ objtool_dep = $(objtool_obj)                                     \
> > > >             $(wildcard include/config/orc/unwinder.h          \
> > > >                        include/config/stack/validation.h)
> > > >
> > > > +ifdef CONFIG_TRIM_UNUSED_KSYMS
> > > > +cmd_gen_ksymdeps = \
> > > > +     $(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
> > > > +     cat $(dot-target).tmp >> $(dot-target).cmd; \
> > > > +     rm -f $(dot-target).tmp;
> > >
> > > Why don't you append to $(dot-target).cmd directly?
> >
> >
> > If scripts/gen_ksymdeps.sh fails for some reasons,
> > it will error out immediately thanks to 'set -e' flag.
> >
> > Appending incomplete portion might end up with a corrupted .*.cmd file.
> >
> > Probably, that would not happen, but I just wanted to ensure it.
>
> Well, strictly speaking, if scripts/gen_ksymdeps.sh fails and its output
> isn't appended at all to the .*.cmd file, then that .*.cmd file is
> already corrupted as it is missing necessary dependencies. Would be
> better to delete the .*.cmd file entirely in that case.


More strictly speaking, missing necessary dependencies is not a big deal.

Now, scripts/Kbuild.include specifies .DELETE_ON_ERROR

Any error in fixdep, gen_ksymdeps.sh, or whatever
will delete *.o file anyway.

So, I change the course
so that fixdep and gen_ksymdeps.sh directly write to .*.cmd files.


I wrote detailed explanation here:
https://patchwork.kernel.org/patch/10689697/




-- 
Best Regards
Masahiro Yamada

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

[Masahiro Yamada]

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(-)

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)
-- 
2.7.4

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

[Rasmus Villemoes]

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

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

[Masahiro Yamada]

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

[PATCH 6/8] kbuild: remove trailing semicolon from cmd_* passed to if_changed_rule

[Masahiro Yamada]

All commands passed into rule_cc_o_c / rule_as_o_S needed to end with
a semicolon because the old if_changed_rule implementation only
accepted a single shell command. To pass in multiple shell commands,
they must be concatenated with '; \'.

With the if_changed_rule change in the last commit, this restriction
does not exist any more. Rip off unneeded semicolons.

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

 scripts/Makefile.build | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 5528a6a..78e647f 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -75,14 +75,14 @@ __build: $(if $(KBUILD_BUILTIN),$(builtin-target) $(lib-target) $(extra-y)) \
 # Linus' kernel sanity checking tool
 ifeq ($(KBUILD_CHECKSRC),1)
   quiet_cmd_checksrc       = CHECK   $<
-        cmd_checksrc       = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+        cmd_checksrc       = $(CHECK) $(CHECKFLAGS) $(c_flags) $<
 else ifeq ($(KBUILD_CHECKSRC),2)
   quiet_cmd_force_checksrc = CHECK   $<
-        cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $< ;
+        cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $<
 endif
 
 ifneq ($(KBUILD_ENABLE_EXTRA_GCC_CHECKS),)
-  cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $< ;
+  cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
 endif
 
 # Do section mismatch analysis for each module/built-in.a
@@ -178,7 +178,7 @@ cmd_modversions_c =								\
 			-T $(@D)/.tmp_$(@F:.o=.ver);				\
 		mv -f $(@D)/.tmp_$(@F) $@;					\
 		rm -f $(@D)/.tmp_$(@F:.o=.ver);					\
-	fi;
+	fi
 endif
 
 ifdef CONFIG_FTRACE_MCOUNT_RECORD
@@ -211,7 +211,7 @@ cmd_record_mcount =						\
 	if [ "$(findstring $(CC_FLAGS_FTRACE),$(_c_flags))" =	\
 	     "$(CC_FLAGS_FTRACE)" ]; then			\
 		$(sub_cmd_record_mcount)			\
-	fi;
+	fi
 endif # CC_USING_RECORD_MCOUNT
 endif # CONFIG_FTRACE_MCOUNT_RECORD
 
@@ -241,7 +241,7 @@ endif
 # 'OBJECT_FILES_NON_STANDARD_foo.o := 'n': override directory skip for a file
 cmd_objtool = $(if $(patsubst y%,, \
 	$(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
-	$(__objtool_obj) $(objtool_args) $@;)
+	$(__objtool_obj) $(objtool_args) $@)
 objtool_obj = $(if $(patsubst y%,, \
 	$(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
 	$(__objtool_obj))
@@ -258,7 +258,7 @@ ifdef CONFIG_TRIM_UNUSED_KSYMS
 cmd_gen_ksymdeps = \
 	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ > $(dot-target).tmp; \
 	cat $(dot-target).tmp >> $(dot-target).cmd; \
-	rm -f $(dot-target).tmp;
+	rm -f $(dot-target).tmp
 
 endif
 
@@ -375,7 +375,7 @@ cmd_modversions_S =								\
 			-T $(@D)/.tmp_$(@F:.o=.ver);				\
 		mv -f $(@D)/.tmp_$(@F) $@;					\
 		rm -f $(@D)/.tmp_$(@F:.o=.ver);					\
-	fi;
+	fi
 endif
 endif
 
-- 
2.7.4

[PATCH 7/8] kbuild: refactor if_changed and if_changed_dep

[Masahiro Yamada]

'@set -e; $(echo-cmd) $(cmd_$(1)' can be replaced with '$(cmd)'.
More cleanups.

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

 scripts/Kbuild.include | 9 +++------
 scripts/Makefile.build | 4 ++--
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 5e47bf6..e4b77ef7 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -251,17 +251,14 @@ any-prereq = $(filter-out $(PHONY),$?) $(filter-out $(PHONY) $(wildcard $^),$^)
 
 # Execute command if command has changed or prerequisite(s) are updated.
 if_changed = $(if $(strip $(any-prereq) $(arg-check)),                       \
-	@set -e;                                                             \
-	$(echo-cmd) $(cmd_$(1));                                             \
+	$(cmd);                                                              \
 	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
 
 # Execute the command and also postprocess generated .d dependencies file.
-if_changed_dep = $(if $(strip $(any-prereq) $(arg-check) ),                  \
-	@set -e;                                                             \
-	$(cmd_and_fixdep), @:)
+if_changed_dep = $(if $(strip $(any-prereq) $(arg-check)), $(cmd_and_fixdep), @:)
 
 cmd_and_fixdep =                                                             \
-	$(echo-cmd) $(cmd_$(1));                                             \
+	$(cmd);                                                              \
 	scripts/basic/fixdep $(depfile) $@ '$(make-cmd)' > $(dot-target).tmp;\
 	rm -f $(depfile);                                                    \
 	mv -f $(dot-target).tmp $(dot-target).cmd;
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 78e647f..0f28df2 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -264,7 +264,7 @@ endif
 
 define rule_cc_o_c
 	$(call cmd,checksrc)
-	@$(call cmd_and_fixdep,cc_o_c)
+	$(call cmd_and_fixdep,cc_o_c)
 	$(call cmd,gen_ksymdeps)
 	$(call cmd,checkdoc)
 	$(call cmd,objtool)
@@ -273,7 +273,7 @@ define rule_cc_o_c
 endef
 
 define rule_as_o_S
-	@$(call cmd_and_fixdep,as_o_S)
+	$(call cmd_and_fixdep,as_o_S)
 	$(call cmd,gen_ksymdeps)
 	$(call cmd,objtool)
 	$(call cmd,modversions_S)
-- 
2.7.4

[PATCH 8/8] kbuild: remove redundant 'set -e' from cmd_* defines

[Masahiro Yamada]

These three cmd_* are invoked in the $(call cmd,*) form.

Now that 'set -e' moved to the 'cmd' macro, they do not need to
explicitly give 'set -e'.

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

 scripts/Makefile.build   | 2 --
 scripts/package/Makefile | 1 -
 2 files changed, 3 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 0f28df2..0bb8d9a 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -134,7 +134,6 @@ cmd_gensymtypes_c =                                                         \
 
 quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
 cmd_cc_symtypes_c =                                                         \
-    set -e;                                                                 \
     $(call cmd_gensymtypes_c,true,$@) >/dev/null;                           \
     test -s $@ || rm -f $@
 
@@ -340,7 +339,6 @@ cmd_gensymtypes_S =                                                         \
 
 quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
 cmd_cc_symtypes_S =                                                         \
-    set -e;                                                                 \
     $(call cmd_gensymtypes_S,true,$@) >/dev/null;                           \
     test -s $@ || rm -f $@
 
diff --git a/scripts/package/Makefile b/scripts/package/Makefile
index 73503eb..453fece 100644
--- a/scripts/package/Makefile
+++ b/scripts/package/Makefile
@@ -33,7 +33,6 @@ MKSPEC     := $(srctree)/scripts/package/mkspec
 
 quiet_cmd_src_tar = TAR     $(2).tar.gz
       cmd_src_tar = \
-set -e; \
 if test "$(objtree)" != "$(srctree)"; then \
 	echo >&2; \
 	echo >&2 "  ERROR:"; \
-- 
2.7.4