All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 00/10] kbuild: misc cleanups
@ 2022-04-05 11:33 Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S} Masahiro Yamada
                   ` (9 more replies)
  0 siblings, 10 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Alexander Lobakin, Michal Marek,
	Nathan Chancellor, Nick Desaulniers, Nicolas Schier,
	Rasmus Villemoes, Sami Tolvanen, llvm


This is a series of prerequisite cleanups of my next work.


Masahiro Yamada (10):
  kbuild: factor out genksyms command from cmd_gensymtypes_{c,S}
  kbuild: do not remove empty *.symtypes explicitly
  modpost: remove useless export_from_sec()
  modpost: move export_from_secname() call to more relevant place
  modpost: remove redundant initializes for static variables
  modpost: remove annoying namespace_from_kstrtabns()
  kbuild: get rid of duplication in the first line of *.mod files
  kbuild: split the second line of *.mod into *.usyms
  kbuild: refactor cmd_modversions_c
  kbuild: refactor cmd_modversions_S

 .gitignore                  |  1 +
 Makefile                    |  2 +-
 scripts/Makefile.build      | 86 ++++++++++++++++---------------------
 scripts/adjust_autoksyms.sh |  2 +-
 scripts/gen_autoksyms.sh    | 18 +++++---
 scripts/mod/modpost.c       | 49 ++++++---------------
 scripts/mod/modpost.h       |  4 --
 scripts/mod/sumversion.c    | 11 +----
 8 files changed, 64 insertions(+), 109 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S}
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 16:16   ` Nick Desaulniers
  2022-04-05 20:00   ` Nicolas Schier
  2022-04-05 11:33 ` [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly Masahiro Yamada
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

The genksyms command part in cmd_gensymtypes_{c,S} is duplicated.
Factor it out into the 'genksyms' macro.

For the readability, I slightly refactor the arguments to genksyms.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - Fix the location of the closing parenthesis

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

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 9717e6f6fb31..31e0e33dfe5d 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -125,13 +125,14 @@ cmd_cpp_i_c       = $(CPP) $(c_flags) -o $@ $<
 $(obj)/%.i: $(src)/%.c FORCE
 	$(call if_changed_dep,cpp_i_c)
 
+genksyms = scripts/genksyms/genksyms		\
+	$(if $(1), -T $(2))			\
+	$(if $(CONFIG_MODULE_REL_CRCS), -R)	\
+	$(if $(KBUILD_PRESERVE), -p)		\
+	-r $(or $(wildcard $(2:.symtypes=.symref)), /dev/null)
+
 # These mirror gensymtypes_S and co below, keep them in synch.
-cmd_gensymtypes_c =                                                         \
-    $(CPP) -D__GENKSYMS__ $(c_flags) $< |                                   \
-    scripts/genksyms/genksyms $(if $(1), -T $(2))                           \
-     $(patsubst y,-R,$(CONFIG_MODULE_REL_CRCS))                             \
-     $(if $(KBUILD_PRESERVE),-p)                                            \
-     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
+cmd_gensymtypes_c = $(CPP) -D__GENKSYMS__ $(c_flags) $< | $(genksyms)
 
 quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
 cmd_cc_symtypes_c =                                                         \
@@ -344,11 +345,7 @@ cmd_gensymtypes_S =                                                         \
     $(CPP) $(a_flags) $< |                                                  \
      grep "\<___EXPORT_SYMBOL\>" |                                          \
      sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ; } | \
-    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
-    scripts/genksyms/genksyms $(if $(1), -T $(2))                           \
-     $(patsubst y,-R,$(CONFIG_MODULE_REL_CRCS))                             \
-     $(if $(KBUILD_PRESERVE),-p)                                            \
-     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
+    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
 
 quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
 cmd_cc_symtypes_S =                                                         \
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S} Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 16:18   ` Nick Desaulniers
  2022-04-05 20:01   ` Nicolas Schier
  2022-04-05 11:33 ` [PATCH v2 03/10] modpost: remove useless export_from_sec() Masahiro Yamada
                   ` (7 subsequent siblings)
  9 siblings, 2 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

Presumably, 'test -s $@ || rm -f $@' intends to remove the output when
the genksyms command fails.

It is unneeded because .DELETE_ON_ERROR automatically removes the output
on failure.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - Fix accidental drop of '> /dev/null'

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

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 31e0e33dfe5d..3ef2373f0a57 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -135,9 +135,7 @@ genksyms = scripts/genksyms/genksyms		\
 cmd_gensymtypes_c = $(CPP) -D__GENKSYMS__ $(c_flags) $< | $(genksyms)
 
 quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
-cmd_cc_symtypes_c =                                                         \
-    $(call cmd_gensymtypes_c,true,$@) >/dev/null;                           \
-    test -s $@ || rm -f $@
+      cmd_cc_symtypes_c = $(call cmd_gensymtypes_c,true,$@) >/dev/null
 
 $(obj)/%.symtypes : $(src)/%.c FORCE
 	$(call cmd,cc_symtypes_c)
@@ -348,9 +346,7 @@ cmd_gensymtypes_S =                                                         \
     $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
 
 quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
-cmd_cc_symtypes_S =                                                         \
-    $(call cmd_gensymtypes_S,true,$@) >/dev/null;                           \
-    test -s $@ || rm -f $@
+      cmd_cc_symtypes_S = $(call cmd_gensymtypes_S,true,$@) >/dev/null
 
 $(obj)/%.symtypes : $(src)/%.S FORCE
 	$(call cmd,cc_symtypes_S)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 03/10] modpost: remove useless export_from_sec()
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S} Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 04/10] modpost: move export_from_secname() call to more relevant place Masahiro Yamada
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Nick Desaulniers, Michal Marek

With commit 1743694eb235 ("modpost: stop symbol preloading for
modversion CRC") applied, now export_from_sec() is useless.

handle_symbol() is called for every symbol in the ELF.

When 'symname' does not start with "__ksymtab", export_from_sec() is
called, and the returned value is stored in 'export'.

It is used in the last part of handle_symbol():

    if (strstarts(symname, "__ksymtab_")) {
            name = symname + strlen("__ksymtab_");
            sym_add_exported(name, mod, export);
    }

'export' is used only when 'symname' starts with "__ksymtab_".

So, the value returned by export_from_sec() is never used.

Remove useless export_from_sec(). This makes further cleanups possible.

I put the temporary code:

    export = export_unknown;

Otherwise, I would get the compiler warning:

    warning: 'export' may be used uninitialized in this function [-Wmaybe-uninitialized]

This is apparently false positive because

    if (strstarts(symname, "__ksymtab_")

... is a stronger condition than:

    if (strstarts(symname, "__ksymtab")

Anyway, this part will be cleaned up by the next commit.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

Changes in v2:
  - Fix compiler warning

 scripts/mod/modpost.c | 17 ++---------------
 scripts/mod/modpost.h |  4 ----
 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index ed9d056d2108..eebb32689816 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -369,16 +369,6 @@ static enum export export_from_secname(struct elf_info *elf, unsigned int sec)
 		return export_unknown;
 }
 
-static enum export export_from_sec(struct elf_info *elf, unsigned int sec)
-{
-	if (sec == elf->export_sec)
-		return export_plain;
-	else if (sec == elf->export_gpl_sec)
-		return export_gpl;
-	else
-		return export_unknown;
-}
-
 static const char *namespace_from_kstrtabns(const struct elf_info *info,
 					    const Elf_Sym *sym)
 {
@@ -576,10 +566,7 @@ static int parse_elf(struct elf_info *info, const char *filename)
 				fatal("%s has NOBITS .modinfo\n", filename);
 			info->modinfo = (void *)hdr + sechdrs[i].sh_offset;
 			info->modinfo_len = sechdrs[i].sh_size;
-		} else if (strcmp(secname, "__ksymtab") == 0)
-			info->export_sec = i;
-		else if (strcmp(secname, "__ksymtab_gpl") == 0)
-			info->export_gpl_sec = i;
+		}
 
 		if (sechdrs[i].sh_type == SHT_SYMTAB) {
 			unsigned int sh_link_idx;
@@ -703,7 +690,7 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 	if (strstarts(symname, "__ksymtab"))
 		export = export_from_secname(info, get_secindex(info, sym));
 	else
-		export = export_from_sec(info, get_secindex(info, sym));
+		export = export_unknown;
 
 	switch (sym->st_shndx) {
 	case SHN_COMMON:
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 0c47ff95c0e2..a85dcec3669a 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -25,7 +25,6 @@
 #define Elf_Sym     Elf32_Sym
 #define Elf_Addr    Elf32_Addr
 #define Elf_Sword   Elf64_Sword
-#define Elf_Section Elf32_Half
 #define ELF_ST_BIND ELF32_ST_BIND
 #define ELF_ST_TYPE ELF32_ST_TYPE
 
@@ -40,7 +39,6 @@
 #define Elf_Sym     Elf64_Sym
 #define Elf_Addr    Elf64_Addr
 #define Elf_Sword   Elf64_Sxword
-#define Elf_Section Elf64_Half
 #define ELF_ST_BIND ELF64_ST_BIND
 #define ELF_ST_TYPE ELF64_ST_TYPE
 
@@ -138,8 +136,6 @@ struct elf_info {
 	Elf_Shdr     *sechdrs;
 	Elf_Sym      *symtab_start;
 	Elf_Sym      *symtab_stop;
-	Elf_Section  export_sec;
-	Elf_Section  export_gpl_sec;
 	char         *strtab;
 	char	     *modinfo;
 	unsigned int modinfo_len;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 04/10] modpost: move export_from_secname() call to more relevant place
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (2 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 03/10] modpost: remove useless export_from_sec() Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 05/10] modpost: remove redundant initializes for static variables Masahiro Yamada
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Nick Desaulniers, Michal Marek

The assigned 'export' is only used when

    if (strstarts(symname, "__ksymtab_"))

is met. The else-part of the assignment is the dead code.

Move the export_from_secname() call to where it is used.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

(no changes since v1)

 scripts/mod/modpost.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index eebb32689816..f9e54247ae1d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -684,14 +684,8 @@ static void handle_modversion(const struct module *mod,
 static void handle_symbol(struct module *mod, struct elf_info *info,
 			  const Elf_Sym *sym, const char *symname)
 {
-	enum export export;
 	const char *name;
 
-	if (strstarts(symname, "__ksymtab"))
-		export = export_from_secname(info, get_secindex(info, sym));
-	else
-		export = export_unknown;
-
 	switch (sym->st_shndx) {
 	case SHN_COMMON:
 		if (strstarts(symname, "__gnu_lto_")) {
@@ -726,7 +720,11 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 	default:
 		/* All exported symbols */
 		if (strstarts(symname, "__ksymtab_")) {
+			enum export export;
+
 			name = symname + strlen("__ksymtab_");
+			export = export_from_secname(info,
+						     get_secindex(info, sym));
 			sym_add_exported(name, mod, export);
 		}
 		if (strcmp(symname, "init_module") == 0)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 05/10] modpost: remove redundant initializes for static variables
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (3 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 04/10] modpost: move export_from_secname() call to more relevant place Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 16:21   ` Nick Desaulniers
  2022-04-05 11:33 ` [PATCH v2 06/10] modpost: remove annoying namespace_from_kstrtabns() Masahiro Yamada
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

These are initialized with zeros without explicit initializes.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - New

 scripts/mod/modpost.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f9e54247ae1d..2a202764ff48 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -23,15 +23,15 @@
 #include "../../include/linux/license.h"
 
 /* Are we using CONFIG_MODVERSIONS? */
-static int modversions = 0;
+static int modversions;
 /* Is CONFIG_MODULE_SRCVERSION_ALL set? */
-static int all_versions = 0;
+static int all_versions;
 /* If we are modposting external module set to 1 */
-static int external_module = 0;
+static int external_module;
 /* Only warn about unresolved symbols */
-static int warn_unresolved = 0;
+static int warn_unresolved;
 /* How a symbol is exported */
-static int sec_mismatch_count = 0;
+static int sec_mismatch_count;
 static int sec_mismatch_warn_only = true;
 /* ignore missing files */
 static int ignore_missing_files;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 06/10] modpost: remove annoying namespace_from_kstrtabns()
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (4 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 05/10] modpost: remove redundant initializes for static variables Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 16:26   ` Nick Desaulniers
  2022-04-05 11:33 ` [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files Masahiro Yamada
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

There are two call sites for sym_update_namespace().

When the symbol has no namespace, s->namespace is set to NULL,
but the conversion from "" to NULL is done in two different places.

[1] read_symbols()

  This gets the namespace from __kstrtabns_<symbol>. If the symbol has
  no namespace, sym_get_data(info, sym) returns the empty string "".
  namespace_from_kstrtabns() converts it to NULL before it is passed to
  sym_update_namespace().

[2] read_dump()

  This gets the namespace from the dump file, *.symvers. If the symbol
  has no namespace, the 'namespace' is the empty string "", which is
  directly passed into sym_update_namespace(). The conversion from
  "" to NULL is done in sym_update_namespace().

namespace_from_kstrtabns() exists only for creating this inconsistency.

By removing it, sym_update_namespace() is consistently passed with ""
when the symbol has no namespace.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - new

 scripts/mod/modpost.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 2a202764ff48..522d5249d196 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -369,13 +369,6 @@ static enum export export_from_secname(struct elf_info *elf, unsigned int sec)
 		return export_unknown;
 }
 
-static const char *namespace_from_kstrtabns(const struct elf_info *info,
-					    const Elf_Sym *sym)
-{
-	const char *value = sym_get_data(info, sym);
-	return value[0] ? value : NULL;
-}
-
 static void sym_update_namespace(const char *symname, const char *namespace)
 {
 	struct symbol *s = find_symbol(symname);
@@ -391,8 +384,7 @@ static void sym_update_namespace(const char *symname, const char *namespace)
 	}
 
 	free(s->namespace);
-	s->namespace =
-		namespace && namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
+	s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
 }
 
 /**
@@ -2049,9 +2041,7 @@ static void read_symbols(const char *modname)
 		/* Apply symbol namespaces from __kstrtabns_<symbol> entries. */
 		if (strstarts(symname, "__kstrtabns_"))
 			sym_update_namespace(symname + strlen("__kstrtabns_"),
-					     namespace_from_kstrtabns(&info,
-								      sym));
-
+					     sym_get_data(&info, sym));
 		if (strstarts(symname, "__crc_"))
 			handle_modversion(mod, &info, sym,
 					  symname + strlen("__crc_"));
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (5 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 06/10] modpost: remove annoying namespace_from_kstrtabns() Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 20:16   ` Nicolas Schier
  2022-04-05 11:33 ` [PATCH v2 08/10] kbuild: split the second line of *.mod into *.usyms Masahiro Yamada
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

The first line of *.mod lists the member objects of the module.
This list may contain duplication if the same object is added multiple
times, like this:

  obj-m := foo.o
  foo-$(CONFIG_FOO1_X) += foo1.o
  foo-$(CONFIG_FOO1_Y) += foo1.o
  foo-$(CONFIG_FOO2_X) += foo2.o
  foo-$(CONFIG_FOO2_Y) += foo2.o

This is probably not a big deal. As far as I know, the only small
problem is scripts/mod/sumversion.c parses the same file over again.
This can be avoided by adding $(sort ...). It has a side-effect that
sorts the objects alphabetically, but it is not a big deal, either.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - new

 scripts/Makefile.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 3ef2373f0a57..63625877aeae 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -307,8 +307,10 @@ $(obj)/%.prelink.o: $(obj)/%.o FORCE
 	$(call if_changed,cc_prelink_modules)
 endif
 
+multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
+
 cmd_mod = { \
-	echo $(if $($*-objs)$($*-y)$($*-m), $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)), $(@:.mod=.o)); \
+	echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \
 	$(undefined_syms) echo; \
 	} > $@
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 08/10] kbuild: split the second line of *.mod into *.usyms
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (6 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 20:30   ` Nicolas Schier
  2022-04-05 11:33 ` [PATCH v2 09/10] kbuild: refactor cmd_modversions_c Masahiro Yamada
  2022-04-05 11:33 ` [PATCH v2 10/10] kbuild: refactor cmd_modversions_S Masahiro Yamada
  9 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Alexander Lobakin, Michal Marek,
	Nick Desaulniers, Nicolas Schier, Rasmus Villemoes,
	Sami Tolvanen

The *.mod files have two lines; the first line lists the member objects
of the module, and the second line, if CONFIG_TRIM_UNUSED_KSYMS=y, lists
the undefined symbols.

These two are orthogonal. For further cleanups, lets' split the second
line out to separate *.usyms files, which are generated only when
CONFIG_TRIM_UNUSED_KSYMS=y.

Previously, the list of undefined symbols ended up with a very long
line, but now symbols are split by new lines.

Use 'sed' like we did before commit 7d32358be8ac ("kbuild: avoid split
lines in .mod files").

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - new

 .gitignore                  |  1 +
 Makefile                    |  2 +-
 scripts/Makefile.build      | 17 +++++++++--------
 scripts/adjust_autoksyms.sh |  2 +-
 scripts/gen_autoksyms.sh    | 18 +++++++++++-------
 scripts/mod/sumversion.c    | 11 ++---------
 6 files changed, 25 insertions(+), 26 deletions(-)

diff --git a/.gitignore b/.gitignore
index 7afd412dadd2..265959544978 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,6 +45,7 @@
 *.symversions
 *.tab.[ch]
 *.tar
+*.usyms
 *.xz
 *.zst
 Module.symvers
diff --git a/Makefile b/Makefile
index d9336e783be3..82ee893909e9 100644
--- a/Makefile
+++ b/Makefile
@@ -1848,7 +1848,7 @@ clean: $(clean-dirs)
 		-o -name '*.ko.*' \
 		-o -name '*.dtb' -o -name '*.dtbo' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
 		-o -name '*.dwo' -o -name '*.lst' \
-		-o -name '*.su' -o -name '*.mod' \
+		-o -name '*.su' -o -name '*.mod' -o -name '*.usyms' \
 		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
 		-o -name '*.lex.c' -o -name '*.tab.[ch]' \
 		-o -name '*.asn1.[ch]' \
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 63625877aeae..d934bdf84de4 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -85,7 +85,8 @@ ifdef need-builtin
 targets-for-builtin += $(obj)/built-in.a
 endif
 
-targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
+targets-for-modules := $(foreach suffix, mod $(if $(CONFIG_TRIM_UNUSED_KSYMS), usyms), \
+				$(patsubst %.o, %.$(suffix), $(filter %.o, $(obj-m))))
 
 ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
@@ -260,9 +261,6 @@ endif
 ifdef CONFIG_TRIM_UNUSED_KSYMS
 cmd_gen_ksymdeps = \
 	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
-
-# List module undefined symbols
-undefined_syms = $(NM) $< | $(AWK) '$$1 == "U" { printf("%s%s", x++ ? " " : "", $$2) }';
 endif
 
 define rule_cc_o_c
@@ -309,14 +307,17 @@ endif
 
 multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
 
-cmd_mod = { \
-	echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \
-	$(undefined_syms) echo; \
-	} > $@
+cmd_mod = echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)) > $@
 
 $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
 	$(call if_changed,mod)
 
+# List module undefined symbols
+cmd_undefined_syms = $(NM) $< | sed -n 's/^  *U //p' > $@
+
+$(obj)/%.usyms: $(obj)/%$(mod-prelink-ext).o FORCE
+	$(call if_changed,undefined_syms)
+
 quiet_cmd_cc_lst_c = MKLST   $@
       cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
 		     $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
index 59fdb875e818..f1b5ac818411 100755
--- a/scripts/adjust_autoksyms.sh
+++ b/scripts/adjust_autoksyms.sh
@@ -35,7 +35,7 @@ case "$KBUILD_VERBOSE" in
 esac
 
 # Generate a new symbol list file
-$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh "$new_ksyms_file"
+$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh --modorder "$new_ksyms_file"
 
 # Extract changes between old and new list and touch corresponding
 # dependency files.
diff --git a/scripts/gen_autoksyms.sh b/scripts/gen_autoksyms.sh
index 120225c541c5..faacf7062122 100755
--- a/scripts/gen_autoksyms.sh
+++ b/scripts/gen_autoksyms.sh
@@ -2,13 +2,10 @@
 # SPDX-License-Identifier: GPL-2.0-only
 
 # Create an autoksyms.h header file from the list of all module's needed symbols
-# as recorded on the second line of *.mod files and the user-provided symbol
-# whitelist.
+# as recorded in *.usyms files and the user-provided symbol whitelist.
 
 set -e
 
-output_file="$1"
-
 # Use "make V=1" to debug this script.
 case "$KBUILD_VERBOSE" in
 *1*)
@@ -16,6 +13,15 @@ case "$KBUILD_VERBOSE" in
 	;;
 esac
 
+read_modorder=
+
+if [ "$1" = --modorder ]; then
+	shift
+	read_modorder=1
+fi
+
+output_file="$1"
+
 needed_symbols=
 
 # Special case for modversions (see modpost.c)
@@ -41,10 +47,8 @@ cat > "$output_file" << EOT
 
 EOT
 
-[ -f modules.order ] && modlist=modules.order || modlist=/dev/null
-
 {
-	sed 's/ko$/mod/' $modlist | xargs -n1 sed -n -e '2p'
+	[ -n "${read_modorder}" ] && sed 's/ko$/usyms/' modules.order | xargs cat
 	echo "$needed_symbols"
 	[ -n "$ksym_wl" ] && cat "$ksym_wl"
 } | sed -e 's/ /\n/g' | sed -n -e '/^$/!p' |
diff --git a/scripts/mod/sumversion.c b/scripts/mod/sumversion.c
index 905c0ec291e1..0125698f2037 100644
--- a/scripts/mod/sumversion.c
+++ b/scripts/mod/sumversion.c
@@ -387,7 +387,7 @@ static int parse_source_files(const char *objfile, struct md4_ctx *md)
 /* Calc and record src checksum. */
 void get_src_version(const char *modname, char sum[], unsigned sumlen)
 {
-	char *buf, *pos, *firstline;
+	char *buf;
 	struct md4_ctx md;
 	char *fname;
 	char filelist[PATH_MAX + 1];
@@ -397,15 +397,8 @@ void get_src_version(const char *modname, char sum[], unsigned sumlen)
 
 	buf = read_text_file(filelist);
 
-	pos = buf;
-	firstline = get_line(&pos);
-	if (!firstline) {
-		warn("bad ending versions file for %s\n", modname);
-		goto free;
-	}
-
 	md4_init(&md);
-	while ((fname = strsep(&firstline, " "))) {
+	while ((fname = strsep(&buf, " \n"))) {
 		if (!*fname)
 			continue;
 		if (!(is_static_library(fname)) &&
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 09/10] kbuild: refactor cmd_modversions_c
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (7 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 08/10] kbuild: split the second line of *.mod into *.usyms Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 16:42   ` Nick Desaulniers
  2022-04-05 11:33 ` [PATCH v2 10/10] kbuild: refactor cmd_modversions_S Masahiro Yamada
  9 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nathan Chancellor,
	Nick Desaulniers, llvm

cmd_modversions_c implements two parts; run genksyms to calculate CRCs
of exported symbols, run $(LD) to update the object with the CRCs. The
latter is not executed for CONFIG_LTO_CLANG=y since the object is not
ELF but LLVM bit code at this point.

The first part can be unified because we can always use $(NM) instead
of "$(OBJDUMP) -h" to dump the symbols.

Split the code into the two macros, cmd_gen_symversions_c and
cmd_modversions.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
 - new

 scripts/Makefile.build | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index d934bdf84de4..ba2be555f942 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -169,29 +169,25 @@ ifdef CONFIG_MODVERSIONS
 #   the actual value of the checksum generated by genksyms
 # o remove .tmp_<file>.o to <file>.o
 
-ifdef CONFIG_LTO_CLANG
 # Generate .o.symversions files for each .o with exported symbols, and link these
 # to the kernel and/or modules at the end.
-cmd_modversions_c =								\
+gen_symversions =								\
 	if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then			\
-		$(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
+		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
 		    > $@.symversions;						\
 	else									\
 		rm -f $@.symversions;						\
-	fi;
-else
-cmd_modversions_c =								\
-	if $(OBJDUMP) -h $@ | grep -q __ksymtab; then				\
-		$(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))	\
-		    > $(@D)/.tmp_$(@F:.o=.ver);					\
-										\
+	fi
+
+cmd_gen_symversions_c =	$(call gen_symversions,c)
+
+cmd_modversions =								\
+	if [ -r $@.symversions ]; then						\
 		$(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@ 		\
-			-T $(@D)/.tmp_$(@F:.o=.ver);				\
+			-T $@.symversions;					\
 		mv -f $(@D)/.tmp_$(@F) $@;					\
-		rm -f $(@D)/.tmp_$(@F:.o=.ver);					\
 	fi
 endif
-endif
 
 ifdef CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT
 # compiler will not generate __mcount_loc use recordmcount or recordmcount.pl
@@ -269,7 +265,8 @@ define rule_cc_o_c
 	$(call cmd,checksrc)
 	$(call cmd,checkdoc)
 	$(call cmd,gen_objtooldep)
-	$(call cmd,modversions_c)
+	$(call cmd,gen_symversions_c)
+	$(if $(CONFIG_LTO_CLANG),,$(call cmd,modversions))
 	$(call cmd,record_mcount)
 endef
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 10/10] kbuild: refactor cmd_modversions_S
  2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
                   ` (8 preceding siblings ...)
  2022-04-05 11:33 ` [PATCH v2 09/10] kbuild: refactor cmd_modversions_c Masahiro Yamada
@ 2022-04-05 11:33 ` Masahiro Yamada
  2022-04-05 16:47   ` Nick Desaulniers
  9 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 11:33 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

Split the code into two macros, cmd_gen_symversions_S for running
genksyms, and cmd_modversions for running $(LD) to update the object
with CRCs.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - new

 scripts/Makefile.build | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index ba2be555f942..58be0997c5dd 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -274,7 +274,8 @@ define rule_as_o_S
 	$(call cmd_and_fixdep,as_o_S)
 	$(call cmd,gen_ksymdeps)
 	$(call cmd,gen_objtooldep)
-	$(call cmd,modversions_S)
+	$(call cmd,gen_symversions_S)
+	$(call cmd,modversions)
 endef
 
 # Built-in and composite module parts
@@ -366,16 +367,8 @@ ifdef CONFIG_ASM_MODVERSIONS
 # versioning matches the C process described above, with difference that
 # we parse asm-prototypes.h C header to get function definitions.
 
-cmd_modversions_S =								\
-	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) $@ 		\
-			-T $(@D)/.tmp_$(@F:.o=.ver);				\
-		mv -f $(@D)/.tmp_$(@F) $@;					\
-		rm -f $(@D)/.tmp_$(@F:.o=.ver);					\
-	fi
+cmd_gen_symversions_S = $(call gen_symversions,S)
+
 endif
 
 $(obj)/%.o: $(src)/%.S FORCE
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S}
  2022-04-05 11:33 ` [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S} Masahiro Yamada
@ 2022-04-05 16:16   ` Nick Desaulniers
  2022-04-05 20:00   ` Nicolas Schier
  1 sibling, 0 replies; 24+ messages in thread
From: Nick Desaulniers @ 2022-04-05 16:16 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Michal Marek

On Tue, Apr 5, 2022 at 4:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> The genksyms command part in cmd_gensymtypes_{c,S} is duplicated.
> Factor it out into the 'genksyms' macro.
>
> For the readability, I slightly refactor the arguments to genksyms.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

It looks like you may have sent v2 twice; apologies if I comment on
BOTH threads rather than 1.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
> Changes in v2:
>   - Fix the location of the closing parenthesis
>
>  scripts/Makefile.build | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 9717e6f6fb31..31e0e33dfe5d 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -125,13 +125,14 @@ cmd_cpp_i_c       = $(CPP) $(c_flags) -o $@ $<
>  $(obj)/%.i: $(src)/%.c FORCE
>         $(call if_changed_dep,cpp_i_c)
>
> +genksyms = scripts/genksyms/genksyms           \
> +       $(if $(1), -T $(2))                     \
> +       $(if $(CONFIG_MODULE_REL_CRCS), -R)     \
> +       $(if $(KBUILD_PRESERVE), -p)            \
> +       -r $(or $(wildcard $(2:.symtypes=.symref)), /dev/null)
> +
>  # These mirror gensymtypes_S and co below, keep them in synch.
> -cmd_gensymtypes_c =                                                         \
> -    $(CPP) -D__GENKSYMS__ $(c_flags) $< |                                   \
> -    scripts/genksyms/genksyms $(if $(1), -T $(2))                           \
> -     $(patsubst y,-R,$(CONFIG_MODULE_REL_CRCS))                             \
> -     $(if $(KBUILD_PRESERVE),-p)                                            \
> -     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
> +cmd_gensymtypes_c = $(CPP) -D__GENKSYMS__ $(c_flags) $< | $(genksyms)
>
>  quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
>  cmd_cc_symtypes_c =                                                         \
> @@ -344,11 +345,7 @@ cmd_gensymtypes_S =                                                         \
>      $(CPP) $(a_flags) $< |                                                  \
>       grep "\<___EXPORT_SYMBOL\>" |                                          \
>       sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ; } | \
> -    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
> -    scripts/genksyms/genksyms $(if $(1), -T $(2))                           \
> -     $(patsubst y,-R,$(CONFIG_MODULE_REL_CRCS))                             \
> -     $(if $(KBUILD_PRESERVE),-p)                                            \
> -     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
> +    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
>
>  quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
>  cmd_cc_symtypes_S =                                                         \
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly
  2022-04-05 11:33 ` [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly Masahiro Yamada
@ 2022-04-05 16:18   ` Nick Desaulniers
  2022-04-05 20:01   ` Nicolas Schier
  1 sibling, 0 replies; 24+ messages in thread
From: Nick Desaulniers @ 2022-04-05 16:18 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Michal Marek

On Tue, Apr 5, 2022 at 4:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Presumably, 'test -s $@ || rm -f $@' intends to remove the output when
> the genksyms command fails.
>
> It is unneeded because .DELETE_ON_ERROR automatically removes the output
> on failure.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
> Changes in v2:
>   - Fix accidental drop of '> /dev/null'
>
>  scripts/Makefile.build | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 31e0e33dfe5d..3ef2373f0a57 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -135,9 +135,7 @@ genksyms = scripts/genksyms/genksyms                \
>  cmd_gensymtypes_c = $(CPP) -D__GENKSYMS__ $(c_flags) $< | $(genksyms)
>
>  quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
> -cmd_cc_symtypes_c =                                                         \
> -    $(call cmd_gensymtypes_c,true,$@) >/dev/null;                           \
> -    test -s $@ || rm -f $@
> +      cmd_cc_symtypes_c = $(call cmd_gensymtypes_c,true,$@) >/dev/null
>
>  $(obj)/%.symtypes : $(src)/%.c FORCE
>         $(call cmd,cc_symtypes_c)
> @@ -348,9 +346,7 @@ cmd_gensymtypes_S =                                                         \
>      $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
>
>  quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
> -cmd_cc_symtypes_S =                                                         \
> -    $(call cmd_gensymtypes_S,true,$@) >/dev/null;                           \
> -    test -s $@ || rm -f $@
> +      cmd_cc_symtypes_S = $(call cmd_gensymtypes_S,true,$@) >/dev/null
>
>  $(obj)/%.symtypes : $(src)/%.S FORCE
>         $(call cmd,cc_symtypes_S)
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 05/10] modpost: remove redundant initializes for static variables
  2022-04-05 11:33 ` [PATCH v2 05/10] modpost: remove redundant initializes for static variables Masahiro Yamada
@ 2022-04-05 16:21   ` Nick Desaulniers
  0 siblings, 0 replies; 24+ messages in thread
From: Nick Desaulniers @ 2022-04-05 16:21 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Michal Marek

On Tue, Apr 5, 2022 at 4:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> These are initialized with zeros without explicit initializes.

s/initializes/initializers/

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v2:
>   - New
>
>  scripts/mod/modpost.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f9e54247ae1d..2a202764ff48 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -23,15 +23,15 @@
>  #include "../../include/linux/license.h"
>
>  /* Are we using CONFIG_MODVERSIONS? */
> -static int modversions = 0;
> +static int modversions;
>  /* Is CONFIG_MODULE_SRCVERSION_ALL set? */
> -static int all_versions = 0;
> +static int all_versions;
>  /* If we are modposting external module set to 1 */
> -static int external_module = 0;
> +static int external_module;
>  /* Only warn about unresolved symbols */
> -static int warn_unresolved = 0;
> +static int warn_unresolved;
>  /* How a symbol is exported */
> -static int sec_mismatch_count = 0;
> +static int sec_mismatch_count;
>  static int sec_mismatch_warn_only = true;
>  /* ignore missing files */
>  static int ignore_missing_files;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 06/10] modpost: remove annoying namespace_from_kstrtabns()
  2022-04-05 11:33 ` [PATCH v2 06/10] modpost: remove annoying namespace_from_kstrtabns() Masahiro Yamada
@ 2022-04-05 16:26   ` Nick Desaulniers
  0 siblings, 0 replies; 24+ messages in thread
From: Nick Desaulniers @ 2022-04-05 16:26 UTC (permalink / raw)
  To: Masahiro Yamada; +Cc: linux-kbuild, linux-kernel, Michal Marek

On Tue, Apr 5, 2022 at 4:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> There are two call sites for sym_update_namespace().
>
> When the symbol has no namespace, s->namespace is set to NULL,
> but the conversion from "" to NULL is done in two different places.
>
> [1] read_symbols()
>
>   This gets the namespace from __kstrtabns_<symbol>. If the symbol has
>   no namespace, sym_get_data(info, sym) returns the empty string "".
>   namespace_from_kstrtabns() converts it to NULL before it is passed to
>   sym_update_namespace().
>
> [2] read_dump()
>
>   This gets the namespace from the dump file, *.symvers. If the symbol
>   has no namespace, the 'namespace' is the empty string "", which is
>   directly passed into sym_update_namespace(). The conversion from
>   "" to NULL is done in sym_update_namespace().
>
> namespace_from_kstrtabns() exists only for creating this inconsistency.
>
> By removing it, sym_update_namespace() is consistently passed with ""
> when the symbol has no namespace.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
> Changes in v2:
>   - new
>
>  scripts/mod/modpost.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 2a202764ff48..522d5249d196 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -369,13 +369,6 @@ static enum export export_from_secname(struct elf_info *elf, unsigned int sec)
>                 return export_unknown;
>  }
>
> -static const char *namespace_from_kstrtabns(const struct elf_info *info,
> -                                           const Elf_Sym *sym)
> -{
> -       const char *value = sym_get_data(info, sym);
> -       return value[0] ? value : NULL;
> -}
> -
>  static void sym_update_namespace(const char *symname, const char *namespace)
>  {
>         struct symbol *s = find_symbol(symname);
> @@ -391,8 +384,7 @@ static void sym_update_namespace(const char *symname, const char *namespace)
>         }
>
>         free(s->namespace);
> -       s->namespace =
> -               namespace && namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
> +       s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
>  }
>
>  /**
> @@ -2049,9 +2041,7 @@ static void read_symbols(const char *modname)
>                 /* Apply symbol namespaces from __kstrtabns_<symbol> entries. */
>                 if (strstarts(symname, "__kstrtabns_"))
>                         sym_update_namespace(symname + strlen("__kstrtabns_"),
> -                                            namespace_from_kstrtabns(&info,
> -                                                                     sym));
> -
> +                                            sym_get_data(&info, sym));
>                 if (strstarts(symname, "__crc_"))
>                         handle_modversion(mod, &info, sym,
>                                           symname + strlen("__crc_"));
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 09/10] kbuild: refactor cmd_modversions_c
  2022-04-05 11:33 ` [PATCH v2 09/10] kbuild: refactor cmd_modversions_c Masahiro Yamada
@ 2022-04-05 16:42   ` Nick Desaulniers
  0 siblings, 0 replies; 24+ messages in thread
From: Nick Desaulniers @ 2022-04-05 16:42 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Michal Marek, Nathan Chancellor,
	llvm, Sami Tolvanen, Kees Cook

On Tue, Apr 5, 2022 at 4:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> cmd_modversions_c implements two parts; run genksyms to calculate CRCs
> of exported symbols, run $(LD) to update the object with the CRCs. The
> latter is not executed for CONFIG_LTO_CLANG=y since the object is not
> ELF but LLVM bit code at this point.
>
> The first part can be unified because we can always use $(NM) instead
> of "$(OBJDUMP) -h" to dump the symbols.
>
> Split the code into the two macros, cmd_gen_symversions_c and
> cmd_modversions.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
> Changes in v2:
>  - new
>
>  scripts/Makefile.build | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index d934bdf84de4..ba2be555f942 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -169,29 +169,25 @@ ifdef CONFIG_MODVERSIONS
>  #   the actual value of the checksum generated by genksyms
>  # o remove .tmp_<file>.o to <file>.o
>
> -ifdef CONFIG_LTO_CLANG
>  # Generate .o.symversions files for each .o with exported symbols, and link these
>  # to the kernel and/or modules at the end.
> -cmd_modversions_c =                                                            \
> +gen_symversions =                                                              \
>         if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then                       \
> -               $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))  \
> +               $(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
>                     > $@.symversions;                                           \
>         else                                                                    \
>                 rm -f $@.symversions;                                           \
> -       fi;
> -else
> -cmd_modversions_c =                                                            \
> -       if $(OBJDUMP) -h $@ | grep -q __ksymtab; then                           \
> -               $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))  \
> -                   > $(@D)/.tmp_$(@F:.o=.ver);                                 \
> -                                                                               \
> +       fi
> +
> +cmd_gen_symversions_c =        $(call gen_symversions,c)
> +
> +cmd_modversions =                                                              \
> +       if [ -r $@.symversions ]; then                                          \
>                 $(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@               \
> -                       -T $(@D)/.tmp_$(@F:.o=.ver);                            \
> +                       -T $@.symversions;                                      \
>                 mv -f $(@D)/.tmp_$(@F) $@;                                      \
> -               rm -f $(@D)/.tmp_$(@F:.o=.ver);                                 \
>         fi
>  endif
> -endif
>
>  ifdef CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT
>  # compiler will not generate __mcount_loc use recordmcount or recordmcount.pl
> @@ -269,7 +265,8 @@ define rule_cc_o_c
>         $(call cmd,checksrc)
>         $(call cmd,checkdoc)
>         $(call cmd,gen_objtooldep)
> -       $(call cmd,modversions_c)
> +       $(call cmd,gen_symversions_c)
> +       $(if $(CONFIG_LTO_CLANG),,$(call cmd,modversions))
>         $(call cmd,record_mcount)
>  endef
>
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 10/10] kbuild: refactor cmd_modversions_S
  2022-04-05 11:33 ` [PATCH v2 10/10] kbuild: refactor cmd_modversions_S Masahiro Yamada
@ 2022-04-05 16:47   ` Nick Desaulniers
  0 siblings, 0 replies; 24+ messages in thread
From: Nick Desaulniers @ 2022-04-05 16:47 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Michal Marek, Sami Tolvanen,
	Kees Cook, Nathan Chancellor

On Tue, Apr 5, 2022 at 4:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Split the code into two macros, cmd_gen_symversions_S for running
> genksyms, and cmd_modversions for running $(LD) to update the object
> with CRCs.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Thanks for the patch!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
> Changes in v2:
>   - new
>
>  scripts/Makefile.build | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index ba2be555f942..58be0997c5dd 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -274,7 +274,8 @@ define rule_as_o_S
>         $(call cmd_and_fixdep,as_o_S)
>         $(call cmd,gen_ksymdeps)
>         $(call cmd,gen_objtooldep)
> -       $(call cmd,modversions_S)
> +       $(call cmd,gen_symversions_S)
> +       $(call cmd,modversions)
>  endef
>
>  # Built-in and composite module parts
> @@ -366,16 +367,8 @@ ifdef CONFIG_ASM_MODVERSIONS
>  # versioning matches the C process described above, with difference that
>  # we parse asm-prototypes.h C header to get function definitions.
>
> -cmd_modversions_S =                                                            \
> -       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) $@               \
> -                       -T $(@D)/.tmp_$(@F:.o=.ver);                            \
> -               mv -f $(@D)/.tmp_$(@F) $@;                                      \
> -               rm -f $(@D)/.tmp_$(@F:.o=.ver);                                 \
> -       fi
> +cmd_gen_symversions_S = $(call gen_symversions,S)
> +
>  endif
>
>  $(obj)/%.o: $(src)/%.S FORCE
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S}
  2022-04-05 11:33 ` [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S} Masahiro Yamada
  2022-04-05 16:16   ` Nick Desaulniers
@ 2022-04-05 20:00   ` Nicolas Schier
  1 sibling, 0 replies; 24+ messages in thread
From: Nicolas Schier @ 2022-04-05 20:00 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Michal Marek, Nick Desaulniers

On Tue, Apr 05, 2022 at 08:33:49PM +0900 Masahiro Yamada wrote:
> The genksyms command part in cmd_gensymtypes_{c,S} is duplicated.
> Factor it out into the 'genksyms' macro.
> 
> For the readability, I slightly refactor the arguments to genksyms.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

> 
> Changes in v2:
>   - Fix the location of the closing parenthesis
> 
>  scripts/Makefile.build | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 9717e6f6fb31..31e0e33dfe5d 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -125,13 +125,14 @@ cmd_cpp_i_c       = $(CPP) $(c_flags) -o $@ $<
>  $(obj)/%.i: $(src)/%.c FORCE
>  	$(call if_changed_dep,cpp_i_c)
>  
> +genksyms = scripts/genksyms/genksyms		\
> +	$(if $(1), -T $(2))			\
> +	$(if $(CONFIG_MODULE_REL_CRCS), -R)	\
> +	$(if $(KBUILD_PRESERVE), -p)		\
> +	-r $(or $(wildcard $(2:.symtypes=.symref)), /dev/null)
> +
>  # These mirror gensymtypes_S and co below, keep them in synch.
> -cmd_gensymtypes_c =                                                         \
> -    $(CPP) -D__GENKSYMS__ $(c_flags) $< |                                   \
> -    scripts/genksyms/genksyms $(if $(1), -T $(2))                           \
> -     $(patsubst y,-R,$(CONFIG_MODULE_REL_CRCS))                             \
> -     $(if $(KBUILD_PRESERVE),-p)                                            \
> -     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
> +cmd_gensymtypes_c = $(CPP) -D__GENKSYMS__ $(c_flags) $< | $(genksyms)
>  
>  quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
>  cmd_cc_symtypes_c =                                                         \
> @@ -344,11 +345,7 @@ cmd_gensymtypes_S =                                                         \
>      $(CPP) $(a_flags) $< |                                                  \
>       grep "\<___EXPORT_SYMBOL\>" |                                          \
>       sed 's/.*___EXPORT_SYMBOL[[:space:]]*\([a-zA-Z0-9_]*\)[[:space:]]*,.*/EXPORT_SYMBOL(\1);/' ; } | \
> -    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - |                                \
> -    scripts/genksyms/genksyms $(if $(1), -T $(2))                           \
> -     $(patsubst y,-R,$(CONFIG_MODULE_REL_CRCS))                             \
> -     $(if $(KBUILD_PRESERVE),-p)                                            \
> -     -r $(firstword $(wildcard $(2:.symtypes=.symref) /dev/null))
> +    $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
>  
>  quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
>  cmd_cc_symtypes_S =                                                         \
> -- 
> 2.32.0
> 
> 

-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly
  2022-04-05 11:33 ` [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly Masahiro Yamada
  2022-04-05 16:18   ` Nick Desaulniers
@ 2022-04-05 20:01   ` Nicolas Schier
  1 sibling, 0 replies; 24+ messages in thread
From: Nicolas Schier @ 2022-04-05 20:01 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Michal Marek, Nick Desaulniers

On Tue, Apr 05, 2022 at 08:33:50PM +0900 Masahiro Yamada wrote:
> Presumably, 'test -s $@ || rm -f $@' intends to remove the output when
> the genksyms command fails.
> 
> It is unneeded because .DELETE_ON_ERROR automatically removes the output
> on failure.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

> 
> Changes in v2:
>   - Fix accidental drop of '> /dev/null'
> 
>  scripts/Makefile.build | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 31e0e33dfe5d..3ef2373f0a57 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -135,9 +135,7 @@ genksyms = scripts/genksyms/genksyms		\
>  cmd_gensymtypes_c = $(CPP) -D__GENKSYMS__ $(c_flags) $< | $(genksyms)
>  
>  quiet_cmd_cc_symtypes_c = SYM $(quiet_modtag) $@
> -cmd_cc_symtypes_c =                                                         \
> -    $(call cmd_gensymtypes_c,true,$@) >/dev/null;                           \
> -    test -s $@ || rm -f $@
> +      cmd_cc_symtypes_c = $(call cmd_gensymtypes_c,true,$@) >/dev/null
>  
>  $(obj)/%.symtypes : $(src)/%.c FORCE
>  	$(call cmd,cc_symtypes_c)
> @@ -348,9 +346,7 @@ cmd_gensymtypes_S =                                                         \
>      $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
>  
>  quiet_cmd_cc_symtypes_S = SYM $(quiet_modtag) $@
> -cmd_cc_symtypes_S =                                                         \
> -    $(call cmd_gensymtypes_S,true,$@) >/dev/null;                           \
> -    test -s $@ || rm -f $@
> +      cmd_cc_symtypes_S = $(call cmd_gensymtypes_S,true,$@) >/dev/null
>  
>  $(obj)/%.symtypes : $(src)/%.S FORCE
>  	$(call cmd,cc_symtypes_S)
> -- 
> 2.32.0
> 
> 

-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files
  2022-04-05 11:33 ` [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files Masahiro Yamada
@ 2022-04-05 20:16   ` Nicolas Schier
  2022-04-06  5:28     ` Masahiro Yamada
  0 siblings, 1 reply; 24+ messages in thread
From: Nicolas Schier @ 2022-04-05 20:16 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Michal Marek, Nick Desaulniers

On Tue, Apr 05, 2022 at 08:33:55PM +0900 Masahiro Yamada wrote:
> The first line of *.mod lists the member objects of the module.
> This list may contain duplication if the same object is added multiple
> times, like this:
> 
>   obj-m := foo.o
>   foo-$(CONFIG_FOO1_X) += foo1.o
>   foo-$(CONFIG_FOO1_Y) += foo1.o
>   foo-$(CONFIG_FOO2_X) += foo2.o
>   foo-$(CONFIG_FOO2_Y) += foo2.o
> 
> This is probably not a big deal. As far as I know, the only small
> problem is scripts/mod/sumversion.c parses the same file over again.
> This can be avoided by adding $(sort ...). It has a side-effect that
> sorts the objects alphabetically, but it is not a big deal, either.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
> Changes in v2:
>   - new
> 
>  scripts/Makefile.build | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 3ef2373f0a57..63625877aeae 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -307,8 +307,10 @@ $(obj)/%.prelink.o: $(obj)/%.o FORCE
>  	$(call if_changed,cc_prelink_modules)
>  endif
>  
> +multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
> +
>  cmd_mod = { \
> -	echo $(if $($*-objs)$($*-y)$($*-m), $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)), $(@:.mod=.o)); \
> +	echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \

I'd rather expected to see $(or) here, too, as in commit 5c8166419acf ("kbuild:
replace $(if A,A,B) with $(or A,B)").

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

>  	$(undefined_syms) echo; \
>  	} > $@
>  
> -- 
> 2.32.0
> 
> 

-- 
epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
     -- frykten for herren er opphav til kunnskap --

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 08/10] kbuild: split the second line of *.mod into *.usyms
  2022-04-05 11:33 ` [PATCH v2 08/10] kbuild: split the second line of *.mod into *.usyms Masahiro Yamada
@ 2022-04-05 20:30   ` Nicolas Schier
  0 siblings, 0 replies; 24+ messages in thread
From: Nicolas Schier @ 2022-04-05 20:30 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-kbuild, linux-kernel, Alexander Lobakin, Michal Marek,
	Nick Desaulniers, Nicolas Schier, Rasmus Villemoes,
	Sami Tolvanen

On Tue, Apr 05, 2022 at 08:33:56PM +0900 Masahiro Yamada wrote:
> The *.mod files have two lines; the first line lists the member objects
> of the module, and the second line, if CONFIG_TRIM_UNUSED_KSYMS=y, lists
> the undefined symbols.
> 
> These two are orthogonal. For further cleanups, lets' split the second

s/lets'/let's/  ?

> line out to separate *.usyms files, which are generated only when
> CONFIG_TRIM_UNUSED_KSYMS=y.
> 
> Previously, the list of undefined symbols ended up with a very long
> line, but now symbols are split by new lines.
> 
> Use 'sed' like we did before commit 7d32358be8ac ("kbuild: avoid split
> lines in .mod files").
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
> Changes in v2:
>   - new
> 
>  .gitignore                  |  1 +
>  Makefile                    |  2 +-
>  scripts/Makefile.build      | 17 +++++++++--------
>  scripts/adjust_autoksyms.sh |  2 +-
>  scripts/gen_autoksyms.sh    | 18 +++++++++++-------
>  scripts/mod/sumversion.c    | 11 ++---------
>  6 files changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index 7afd412dadd2..265959544978 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -45,6 +45,7 @@
>  *.symversions
>  *.tab.[ch]
>  *.tar
> +*.usyms
>  *.xz
>  *.zst
>  Module.symvers
> diff --git a/Makefile b/Makefile
> index d9336e783be3..82ee893909e9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1848,7 +1848,7 @@ clean: $(clean-dirs)
>  		-o -name '*.ko.*' \
>  		-o -name '*.dtb' -o -name '*.dtbo' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
>  		-o -name '*.dwo' -o -name '*.lst' \
> -		-o -name '*.su' -o -name '*.mod' \
> +		-o -name '*.su' -o -name '*.mod' -o -name '*.usyms' \
>  		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
>  		-o -name '*.lex.c' -o -name '*.tab.[ch]' \
>  		-o -name '*.asn1.[ch]' \
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 63625877aeae..d934bdf84de4 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -85,7 +85,8 @@ ifdef need-builtin
>  targets-for-builtin += $(obj)/built-in.a
>  endif
>  
> -targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> +targets-for-modules := $(foreach suffix, mod $(if $(CONFIG_TRIM_UNUSED_KSYMS), usyms), \
> +				$(patsubst %.o, %.$(suffix), $(filter %.o, $(obj-m))))
>  
>  ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
>  targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
> @@ -260,9 +261,6 @@ endif
>  ifdef CONFIG_TRIM_UNUSED_KSYMS
>  cmd_gen_ksymdeps = \
>  	$(CONFIG_SHELL) $(srctree)/scripts/gen_ksymdeps.sh $@ >> $(dot-target).cmd
> -
> -# List module undefined symbols
> -undefined_syms = $(NM) $< | $(AWK) '$$1 == "U" { printf("%s%s", x++ ? " " : "", $$2) }';
>  endif
>  
>  define rule_cc_o_c
> @@ -309,14 +307,17 @@ endif
>  
>  multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
>  
> -cmd_mod = { \
> -	echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \
> -	$(undefined_syms) echo; \
> -	} > $@
> +cmd_mod = echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)) > $@
>  
>  $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
>  	$(call if_changed,mod)
>  
> +# List module undefined symbols
> +cmd_undefined_syms = $(NM) $< | sed -n 's/^  *U //p' > $@
> +
> +$(obj)/%.usyms: $(obj)/%$(mod-prelink-ext).o FORCE
> +	$(call if_changed,undefined_syms)
> +
>  quiet_cmd_cc_lst_c = MKLST   $@
>        cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
>  		     $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> index 59fdb875e818..f1b5ac818411 100755
> --- a/scripts/adjust_autoksyms.sh
> +++ b/scripts/adjust_autoksyms.sh
> @@ -35,7 +35,7 @@ case "$KBUILD_VERBOSE" in
>  esac
>  
>  # Generate a new symbol list file
> -$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh "$new_ksyms_file"
> +$CONFIG_SHELL $srctree/scripts/gen_autoksyms.sh --modorder "$new_ksyms_file"
>  
>  # Extract changes between old and new list and touch corresponding
>  # dependency files.
> diff --git a/scripts/gen_autoksyms.sh b/scripts/gen_autoksyms.sh
> index 120225c541c5..faacf7062122 100755
> --- a/scripts/gen_autoksyms.sh
> +++ b/scripts/gen_autoksyms.sh
> @@ -2,13 +2,10 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
>  # Create an autoksyms.h header file from the list of all module's needed symbols
> -# as recorded on the second line of *.mod files and the user-provided symbol
> -# whitelist.
> +# as recorded in *.usyms files and the user-provided symbol whitelist.
>  
>  set -e
>  
> -output_file="$1"
> -
>  # Use "make V=1" to debug this script.
>  case "$KBUILD_VERBOSE" in
>  *1*)
> @@ -16,6 +13,15 @@ case "$KBUILD_VERBOSE" in
>  	;;
>  esac
>  
> +read_modorder=
> +
> +if [ "$1" = --modorder ]; then
> +	shift
> +	read_modorder=1
> +fi
> +
> +output_file="$1"
> +
>  needed_symbols=
>  
>  # Special case for modversions (see modpost.c)
> @@ -41,10 +47,8 @@ cat > "$output_file" << EOT
>  
>  EOT
>  
> -[ -f modules.order ] && modlist=modules.order || modlist=/dev/null
> -
>  {
> -	sed 's/ko$/mod/' $modlist | xargs -n1 sed -n -e '2p'
> +	[ -n "${read_modorder}" ] && sed 's/ko$/usyms/' modules.order | xargs cat
>  	echo "$needed_symbols"
>  	[ -n "$ksym_wl" ] && cat "$ksym_wl"
>  } | sed -e 's/ /\n/g' | sed -n -e '/^$/!p' |
> diff --git a/scripts/mod/sumversion.c b/scripts/mod/sumversion.c
> index 905c0ec291e1..0125698f2037 100644
> --- a/scripts/mod/sumversion.c
> +++ b/scripts/mod/sumversion.c
> @@ -387,7 +387,7 @@ static int parse_source_files(const char *objfile, struct md4_ctx *md)
>  /* Calc and record src checksum. */
>  void get_src_version(const char *modname, char sum[], unsigned sumlen)
>  {
> -	char *buf, *pos, *firstline;
> +	char *buf;
>  	struct md4_ctx md;
>  	char *fname;
>  	char filelist[PATH_MAX + 1];
> @@ -397,15 +397,8 @@ void get_src_version(const char *modname, char sum[], unsigned sumlen)
>  
>  	buf = read_text_file(filelist);
>  
> -	pos = buf;
> -	firstline = get_line(&pos);
> -	if (!firstline) {
> -		warn("bad ending versions file for %s\n", modname);
> -		goto free;
> -	}
> -
>  	md4_init(&md);
> -	while ((fname = strsep(&firstline, " "))) {
> +	while ((fname = strsep(&buf, " \n"))) {
>  		if (!*fname)
>  			continue;
>  		if (!(is_static_library(fname)) &&
> -- 
> 2.32.0
> 
> 

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files
  2022-04-05 20:16   ` Nicolas Schier
@ 2022-04-06  5:28     ` Masahiro Yamada
  2022-04-06 14:49       ` Masahiro Yamada
  0 siblings, 1 reply; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-06  5:28 UTC (permalink / raw)
  To: Nicolas Schier
  Cc: Linux Kbuild mailing list, Linux Kernel Mailing List,
	Michal Marek, Nick Desaulniers

On Wed, Apr 6, 2022 at 5:16 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Tue, Apr 05, 2022 at 08:33:55PM +0900 Masahiro Yamada wrote:
> > The first line of *.mod lists the member objects of the module.
> > This list may contain duplication if the same object is added multiple
> > times, like this:
> >
> >   obj-m := foo.o
> >   foo-$(CONFIG_FOO1_X) += foo1.o
> >   foo-$(CONFIG_FOO1_Y) += foo1.o
> >   foo-$(CONFIG_FOO2_X) += foo2.o
> >   foo-$(CONFIG_FOO2_Y) += foo2.o
> >
> > This is probably not a big deal. As far as I know, the only small
> > problem is scripts/mod/sumversion.c parses the same file over again.
> > This can be avoided by adding $(sort ...). It has a side-effect that
> > sorts the objects alphabetically, but it is not a big deal, either.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > Changes in v2:
> >   - new
> >
> >  scripts/Makefile.build | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index 3ef2373f0a57..63625877aeae 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -307,8 +307,10 @@ $(obj)/%.prelink.o: $(obj)/%.o FORCE
> >       $(call if_changed,cc_prelink_modules)
> >  endif
> >
> > +multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
> > +
> >  cmd_mod = { \
> > -     echo $(if $($*-objs)$($*-y)$($*-m), $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)), $(@:.mod=.o)); \
> > +     echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \
>
> I'd rather expected to see $(or) here, too, as in commit 5c8166419acf ("kbuild:
> replace $(if A,A,B) with $(or A,B)").

Ah, good catch.

I fixed it up locally.

Thanks for the review.


>
> Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
>
> >       $(undefined_syms) echo; \
> >       } > $@
> >
> > --
> > 2.32.0
> >
> >
>
> --
> epost|xmpp: nicolas@fjasle.eu          irc://oftc.net/nsc
> ↳ gpg: 18ed 52db e34f 860e e9fb  c82b 7d97 0932 55a0 ce7f
>      -- frykten for herren er opphav til kunnskap --



-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files
  2022-04-06  5:28     ` Masahiro Yamada
@ 2022-04-06 14:49       ` Masahiro Yamada
  0 siblings, 0 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-06 14:49 UTC (permalink / raw)
  To: Nicolas Schier
  Cc: Linux Kbuild mailing list, Linux Kernel Mailing List,
	Michal Marek, Nick Desaulniers

On Wed, Apr 6, 2022 at 2:28 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Apr 6, 2022 at 5:16 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Tue, Apr 05, 2022 at 08:33:55PM +0900 Masahiro Yamada wrote:
> > > The first line of *.mod lists the member objects of the module.
> > > This list may contain duplication if the same object is added multiple
> > > times, like this:
> > >
> > >   obj-m := foo.o
> > >   foo-$(CONFIG_FOO1_X) += foo1.o
> > >   foo-$(CONFIG_FOO1_Y) += foo1.o
> > >   foo-$(CONFIG_FOO2_X) += foo2.o
> > >   foo-$(CONFIG_FOO2_Y) += foo2.o
> > >
> > > This is probably not a big deal. As far as I know, the only small
> > > problem is scripts/mod/sumversion.c parses the same file over again.
> > > This can be avoided by adding $(sort ...). It has a side-effect that
> > > sorts the objects alphabetically, but it is not a big deal, either.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > > Changes in v2:
> > >   - new
> > >
> > >  scripts/Makefile.build | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > index 3ef2373f0a57..63625877aeae 100644
> > > --- a/scripts/Makefile.build
> > > +++ b/scripts/Makefile.build
> > > @@ -307,8 +307,10 @@ $(obj)/%.prelink.o: $(obj)/%.o FORCE
> > >       $(call if_changed,cc_prelink_modules)
> > >  endif
> > >
> > > +multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
> > > +
> > >  cmd_mod = { \
> > > -     echo $(if $($*-objs)$($*-y)$($*-m), $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)), $(@:.mod=.o)); \
> > > +     echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \
> >
> > I'd rather expected to see $(or) here, too, as in commit 5c8166419acf ("kbuild:
> > replace $(if A,A,B) with $(or A,B)").
>
> Ah, good catch.
>
> I fixed it up locally.


I changed my mind.
I will throw away 07 and 08.

I will send different patches later.




>
> Thanks for the review.


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files
  2022-04-05 14:02 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
@ 2022-04-05 14:02 ` Masahiro Yamada
  0 siblings, 0 replies; 24+ messages in thread
From: Masahiro Yamada @ 2022-04-05 14:02 UTC (permalink / raw)
  To: linux-kbuild
  Cc: linux-kernel, Masahiro Yamada, Michal Marek, Nick Desaulniers

The first line of *.mod lists the object files that consist of the
module. This may contain duplication if the same object is added
multiple times, like this:

  obj-m := foo.o
  foo-$(CONFIG_FOO1_X) += foo1.o
  foo-$(CONFIG_FOO1_Y) += foo1.o
  foo-$(CONFIG_FOO2_X) += foo2.o
  foo-$(CONFIG_FOO2_Y) += foo2.o

This is probably not a big deal. As far as I know, the only small
problem is scripts/mod/sumversion.c parses the same file over again.
This can be avoided by adding $(sort ...). It has a side-effect that
sorts the objects alphabetically, but it is not a big deal, either.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - new

 scripts/Makefile.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 3ef2373f0a57..63625877aeae 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -307,8 +307,10 @@ $(obj)/%.prelink.o: $(obj)/%.o FORCE
 	$(call if_changed,cc_prelink_modules)
 endif
 
+multi-m-prereqs = $(sort $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)))
+
 cmd_mod = { \
-	echo $(if $($*-objs)$($*-y)$($*-m), $(addprefix $(obj)/, $($*-objs) $($*-y) $($*-m)), $(@:.mod=.o)); \
+	echo $(if $(multi-m-prereqs), $(multi-m-prereqs), $(@:.mod=.o)); \
 	$(undefined_syms) echo; \
 	} > $@
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2022-04-06 17:02 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-05 11:33 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
2022-04-05 11:33 ` [PATCH v2 01/10] kbuild: factor out genksyms command from cmd_gensymtypes_{c,S} Masahiro Yamada
2022-04-05 16:16   ` Nick Desaulniers
2022-04-05 20:00   ` Nicolas Schier
2022-04-05 11:33 ` [PATCH v2 02/10] kbuild: do not remove empty *.symtypes explicitly Masahiro Yamada
2022-04-05 16:18   ` Nick Desaulniers
2022-04-05 20:01   ` Nicolas Schier
2022-04-05 11:33 ` [PATCH v2 03/10] modpost: remove useless export_from_sec() Masahiro Yamada
2022-04-05 11:33 ` [PATCH v2 04/10] modpost: move export_from_secname() call to more relevant place Masahiro Yamada
2022-04-05 11:33 ` [PATCH v2 05/10] modpost: remove redundant initializes for static variables Masahiro Yamada
2022-04-05 16:21   ` Nick Desaulniers
2022-04-05 11:33 ` [PATCH v2 06/10] modpost: remove annoying namespace_from_kstrtabns() Masahiro Yamada
2022-04-05 16:26   ` Nick Desaulniers
2022-04-05 11:33 ` [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files Masahiro Yamada
2022-04-05 20:16   ` Nicolas Schier
2022-04-06  5:28     ` Masahiro Yamada
2022-04-06 14:49       ` Masahiro Yamada
2022-04-05 11:33 ` [PATCH v2 08/10] kbuild: split the second line of *.mod into *.usyms Masahiro Yamada
2022-04-05 20:30   ` Nicolas Schier
2022-04-05 11:33 ` [PATCH v2 09/10] kbuild: refactor cmd_modversions_c Masahiro Yamada
2022-04-05 16:42   ` Nick Desaulniers
2022-04-05 11:33 ` [PATCH v2 10/10] kbuild: refactor cmd_modversions_S Masahiro Yamada
2022-04-05 16:47   ` Nick Desaulniers
2022-04-05 14:02 [PATCH v2 00/10] kbuild: misc cleanups Masahiro Yamada
2022-04-05 14:02 ` [PATCH v2 07/10] kbuild: get rid of duplication in the first line of *.mod files Masahiro Yamada

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.