[PATCH 00/27] kbuild: yet another series of cleanups (modpost and LTO)

[PATCH 00/27] kbuild: yet another series of cleanups (modpost and LTO)

[Masahiro Yamada]

This is the third batch of cleanups in this development cycle.

This weekend, I wrote up the code I have been planning.

After a bunch of modpost refactoring, I got rid of the ugly code
in Makefiles.

With this, Kbuild will get back much simpler and cleaner.



Masahiro Yamada (27):
  modpost: use snprintf() instead of sprintf() for safety
  modpost: do not write out any file when error occurred
  modpost: remove stale comment about sym_add_exported()
  modpost: add a separate error for exported symbols without definition
  modpost: retrieve the module dependency and CRCs in check_exports()
  modpost: use bool type where appropriate
  modpost: import include/linux/list.h
  modpost: traverse modules in order
  modpost: add sym_add_unresolved() helper
  modpost: traverse unresolved symbols in order
  modpost: use doubly linked list for dump_lists
  modpost: move struct namespace_list to modpost.c
  modpost: traverse the namespace_list in order
  modpost: dump Module.symvers in the same order of modules.order
  modpost: move static EXPORT_SYMBOL check to check_exports()
  modpost: make multiple export error
  modpost: make sym_add_exported() always allocate a new symbol
  modpost: make sym_add_exported() a void function
  modpost: use hlist for hash table implementation
  modpost: mitigate false-negatives for static EXPORT_SYMBOL checks
  kbuild: record symbol versions in *.cmd files
  kbuild: generate a list of objects in vmlinux
  modpost: retrieve symbol versions by parsing *.cmd files
  modpost: generate linker script to collect symbol versions
  kbuild: embed symbol versions at final link of vmlinux or modules
  kbuild: stop generating *.symversions
  kbuild: do not create *.prelink.o for Clang LTO or IBT

 .gitignore                |   1 +
 Makefile                  |   1 +
 scripts/Kbuild.include    |   4 +
 scripts/Makefile.build    | 107 ++------
 scripts/Makefile.lib      |   7 -
 scripts/Makefile.modfinal |   6 +-
 scripts/Makefile.modpost  |   9 +-
 scripts/link-vmlinux.sh   |  38 ++-
 scripts/mod/file2alias.c  |   2 -
 scripts/mod/list.h        | 336 ++++++++++++++++++++++++
 scripts/mod/modpost.c     | 529 +++++++++++++++++++++++---------------
 scripts/mod/modpost.h     |  27 +-
 12 files changed, 731 insertions(+), 336 deletions(-)
 create mode 100644 scripts/mod/list.h

-- 
2.32.0

[PATCH 01/27] modpost: use snprintf() instead of sprintf() for safety

[Masahiro Yamada]

Use snprintf() to avoid the potential buffer overflow, and also
check the return value to detect the too long path.

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

 scripts/mod/modpost.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 522d5249d196..141370ebbfd3 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2560,6 +2560,7 @@ int main(int argc, char **argv)
 
 	for (mod = modules; mod; mod = mod->next) {
 		char fname[PATH_MAX];
+		int ret;
 
 		if (mod->is_vmlinux || mod->from_dump)
 			continue;
@@ -2578,7 +2579,12 @@ int main(int argc, char **argv)
 		add_moddevtable(&buf, mod);
 		add_srcversion(&buf, mod);
 
-		sprintf(fname, "%s.mod.c", mod->name);
+		ret = snprintf(fname, sizeof(fname), "%s.mod.c", mod->name);
+		if (ret >= sizeof(fname)) {
+			error("%s: too long path was truncated\n", fname);
+			continue;
+		}
+
 		write_if_changed(&buf, fname);
 	}
 
-- 
2.32.0

Re: [PATCH 01/27] modpost: use snprintf() instead of sprintf() for safety

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Use snprintf() to avoid the potential buffer overflow, and also
> check the return value to detect the too long path.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
>
>  scripts/mod/modpost.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 522d5249d196..141370ebbfd3 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2560,6 +2560,7 @@ int main(int argc, char **argv)
>
>         for (mod = modules; mod; mod = mod->next) {
>                 char fname[PATH_MAX];
> +               int ret;
>
>                 if (mod->is_vmlinux || mod->from_dump)
>                         continue;
> @@ -2578,7 +2579,12 @@ int main(int argc, char **argv)
>                 add_moddevtable(&buf, mod);
>                 add_srcversion(&buf, mod);
>
> -               sprintf(fname, "%s.mod.c", mod->name);
> +               ret = snprintf(fname, sizeof(fname), "%s.mod.c", mod->name);
> +               if (ret >= sizeof(fname)) {
> +                       error("%s: too long path was truncated\n", fname);
> +                       continue;
> +               }
> +
>                 write_if_changed(&buf, fname);
>         }
>
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 02/27] modpost: do not write out any file when error occurred

[Masahiro Yamada]

If an error occurs, modpost will fail anyway. Do not write out
any content (, which might be invalid).

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

 scripts/mod/modpost.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 141370ebbfd3..f0d48f65fb33 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2333,6 +2333,9 @@ static void write_buf(struct buffer *b, const char *fname)
 {
 	FILE *file;
 
+	if (error_occurred)
+		return;
+
 	file = fopen(fname, "w");
 	if (!file) {
 		perror(fname);
-- 
2.32.0

Re: [PATCH 02/27] modpost: do not write out any file when error occurred

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> If an error occurs, modpost will fail anyway. Do not write out
> any content (, which might be invalid).
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

The callers could probably do less work before calling write_buf, too,
but this is ok.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
>  scripts/mod/modpost.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 141370ebbfd3..f0d48f65fb33 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2333,6 +2333,9 @@ static void write_buf(struct buffer *b, const char *fname)
>  {
>         FILE *file;
>
> +       if (error_occurred)
> +               return;
> +
>         file = fopen(fname, "w");
>         if (!file) {
>                 perror(fname);
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 02/27] modpost: do not write out any file when error occurred

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:15 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > If an error occurs, modpost will fail anyway. Do not write out
> > any content (, which might be invalid).
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> The callers could probably do less work before calling write_buf, too,
> but this is ok.

There are two call-sites for write_buf().

I thought it is better to add this if-conditional
in the callee instead of the callers.





> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> > ---
> >
> >  scripts/mod/modpost.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 141370ebbfd3..f0d48f65fb33 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -2333,6 +2333,9 @@ static void write_buf(struct buffer *b, const char *fname)
> >  {
> >         FILE *file;
> >
> > +       if (error_occurred)
> > +               return;
> > +
> >         file = fopen(fname, "w");
> >         if (!file) {
> >                 perror(fname);
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH 03/27] modpost: remove stale comment about sym_add_exported()

[Masahiro Yamada]

The description,

  it may have already been added without a
  CRC, in this case just update the CRC

... is no longer valid.

In very old days, this function was used to update the CRC as well.

Commit 040fcc819a2e ("kbuild: improved modversioning support for
external modules") started to use a separate function (sym_update_crc)
for updating the CRC.

The first part, "Add an exported symbol" is correct, but it is too
obvious from the function name. Drop this comment entirely.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f0d48f65fb33..c7cfeeb088f7 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -387,10 +387,6 @@ static void sym_update_namespace(const char *symname, const char *namespace)
 	s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
 }
 
-/**
- * Add an exported symbol - it may have already been added without a
- * CRC, in this case just update the CRC
- **/
 static struct symbol *sym_add_exported(const char *name, struct module *mod,
 				       enum export export)
 {
-- 
2.32.0

Re: [PATCH 03/27] modpost: remove stale comment about sym_add_exported()

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> The description,
>
>   it may have already been added without a
>   CRC, in this case just update the CRC
>
> ... is no longer valid.
>
> In very old days, this function was used to update the CRC as well.
>
> Commit 040fcc819a2e ("kbuild: improved modversioning support for
> external modules") started to use a separate function (sym_update_crc)
> for updating the CRC.
>
> The first part, "Add an exported symbol" is correct, but it is too
> obvious from the function name. Drop this comment entirely.

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

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f0d48f65fb33..c7cfeeb088f7 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -387,10 +387,6 @@ static void sym_update_namespace(const char *symname, const char *namespace)
>         s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
>  }
>
> -/**
> - * Add an exported symbol - it may have already been added without a
> - * CRC, in this case just update the CRC
> - **/
>  static struct symbol *sym_add_exported(const char *name, struct module *mod,
>                                        enum export export)
>  {
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 04/27] modpost: add a separate error for exported symbols without definition

[Masahiro Yamada]

It took me a while to understand the intent of "exp->module == mod".

This code goes back to 2003 (pre-git era).

The commit is not in this git repository, and might be worth a little
explanation.

You can add EXPORT_SYMBOL() with no definition in the same file (but you
need to put a declaration).

  int foo(void);
  EXPORT_SYMBOL(foo);

This is typical when EXPORT_SYMBOL() is defined in a C file, but the
actual implementation is in a separate assembly file. In old days,
EXPORT_SYMBOL() were only available in C files (but this limitation
does not exist any more).

Add a separate, clearer message if an exported symbol has no definition.
It should be an error even if KBUILD_MODPOST_WARN is given.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=2763b6bcb96e6a38a2fe31108fe5759ec5bcc80a

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

 scripts/mod/modpost.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index c7cfeeb088f7..969a081dba62 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2147,13 +2147,18 @@ static void check_exports(struct module *mod)
 	for (s = mod->unres; s; s = s->next) {
 		const char *basename;
 		exp = find_symbol(s->name);
-		if (!exp || exp->module == mod) {
+		if (!exp) {
 			if (!s->weak && nr_unresolved++ < MAX_UNRESOLVED_REPORTS)
 				modpost_log(warn_unresolved ? LOG_WARN : LOG_ERROR,
 					    "\"%s\" [%s.ko] undefined!\n",
 					    s->name, mod->name);
 			continue;
 		}
+		if (exp->module == mod) {
+			error("\"%s\" [%s.ko] was exported without definition\n",
+			      s->name, mod->name);
+			continue;
+		}
 		basename = strrchr(mod->name, '/');
 		if (basename)
 			basename++;
-- 
2.32.0

Re: [PATCH 04/27] modpost: add a separate error for exported symbols without definition

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> It took me a while to understand the intent of "exp->module == mod".
>
> This code goes back to 2003 (pre-git era).
>
> The commit is not in this git repository, and might be worth a little
> explanation.
>
> You can add EXPORT_SYMBOL() with no definition in the same file (but you
> need to put a declaration).
>
>   int foo(void);
>   EXPORT_SYMBOL(foo);
>
> This is typical when EXPORT_SYMBOL() is defined in a C file, but the
> actual implementation is in a separate assembly file. In old days,
> EXPORT_SYMBOL() were only available in C files (but this limitation
> does not exist any more).
>
> Add a separate, clearer message if an exported symbol has no definition.
> It should be an error even if KBUILD_MODPOST_WARN is given.
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=2763b6bcb96e6a38a2fe31108fe5759ec5bcc80a

Differentiating between the two (and your explanation of how to reach
such a situation) is a nice touch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index c7cfeeb088f7..969a081dba62 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2147,13 +2147,18 @@ static void check_exports(struct module *mod)
>         for (s = mod->unres; s; s = s->next) {
>                 const char *basename;
>                 exp = find_symbol(s->name);
> -               if (!exp || exp->module == mod) {
> +               if (!exp) {
>                         if (!s->weak && nr_unresolved++ < MAX_UNRESOLVED_REPORTS)
>                                 modpost_log(warn_unresolved ? LOG_WARN : LOG_ERROR,
>                                             "\"%s\" [%s.ko] undefined!\n",
>                                             s->name, mod->name);
>                         continue;
>                 }
> +               if (exp->module == mod) {
> +                       error("\"%s\" [%s.ko] was exported without definition\n",
> +                             s->name, mod->name);
> +                       continue;
> +               }
>                 basename = strrchr(mod->name, '/');
>                 if (basename)
>                         basename++;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 05/27] modpost: retrieve the module dependency and CRCs in check_exports()

[Masahiro Yamada]

Do not repeat the similar code.

It is simpler to do this in check_exports() instead of add_versions().

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 969a081dba62..f9cbb6b6b7a5 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2159,6 +2159,11 @@ static void check_exports(struct module *mod)
 			      s->name, mod->name);
 			continue;
 		}
+
+		s->module = exp->module;
+		s->crc_valid = exp->crc_valid;
+		s->crc = exp->crc;
+
 		basename = strrchr(mod->name, '/');
 		if (basename)
 			basename++;
@@ -2251,16 +2256,7 @@ static void add_staging_flag(struct buffer *b, const char *name)
  **/
 static void add_versions(struct buffer *b, struct module *mod)
 {
-	struct symbol *s, *exp;
-
-	for (s = mod->unres; s; s = s->next) {
-		exp = find_symbol(s->name);
-		if (!exp || exp->module == mod)
-			continue;
-		s->module = exp->module;
-		s->crc_valid = exp->crc_valid;
-		s->crc = exp->crc;
-	}
+	struct symbol *s;
 
 	if (!modversions)
 		return;
-- 
2.32.0

Re: [PATCH 05/27] modpost: retrieve the module dependency and CRCs in check_exports()

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Do not repeat the similar code.
>
> It is simpler to do this in check_exports() instead of add_versions().

The 2 loops have been fused into 1. :)
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 969a081dba62..f9cbb6b6b7a5 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2159,6 +2159,11 @@ static void check_exports(struct module *mod)
>                               s->name, mod->name);
>                         continue;
>                 }
> +
> +               s->module = exp->module;
> +               s->crc_valid = exp->crc_valid;
> +               s->crc = exp->crc;
> +
>                 basename = strrchr(mod->name, '/');
>                 if (basename)
>                         basename++;
> @@ -2251,16 +2256,7 @@ static void add_staging_flag(struct buffer *b, const char *name)
>   **/
>  static void add_versions(struct buffer *b, struct module *mod)
>  {
> -       struct symbol *s, *exp;
> -
> -       for (s = mod->unres; s; s = s->next) {
> -               exp = find_symbol(s->name);
> -               if (!exp || exp->module == mod)
> -                       continue;
> -               s->module = exp->module;
> -               s->crc_valid = exp->crc_valid;
> -               s->crc = exp->crc;
> -       }
> +       struct symbol *s;
>
>         if (!modversions)
>                 return;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 06/27] modpost: use bool type where appropriate

[Masahiro Yamada]

Use 'bool' to clarify that the valid value is true or false.

Here is a small note for the conversion.

Strictly speaking, module::gpl_compatible was not boolean because
new_module() initialized it to -1. Maybe, -1 was used to represent the
license is 'unknown', but it is not useful.

Since commit 1d6cd3929360 ("modpost: turn missing MODULE_LICENSE() into
error"), unknown module license is not allowed anyway.

I changed the initializer "= -1" to "= true". This has no functional
change.

The current code:

    if (!mod->gpl_compatible)
            check_for_gpl_usage(exp->export, basename, exp->name);

... only checks whether gpl_compabilt is zero or not:

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

 scripts/mod/modpost.c | 60 +++++++++++++++++++++----------------------
 scripts/mod/modpost.h | 10 ++++----
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f9cbb6b6b7a5..52dd07a36379 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -23,20 +23,20 @@
 #include "../../include/linux/license.h"
 
 /* Are we using CONFIG_MODVERSIONS? */
-static int modversions;
+static bool modversions;
 /* Is CONFIG_MODULE_SRCVERSION_ALL set? */
-static int all_versions;
+static bool all_versions;
 /* If we are modposting external module set to 1 */
-static int external_module;
+static bool external_module;
 /* Only warn about unresolved symbols */
-static int warn_unresolved;
+static bool warn_unresolved;
 /* How a symbol is exported */
 static int sec_mismatch_count;
-static int sec_mismatch_warn_only = true;
+static bool sec_mismatch_warn_only = true;
 /* ignore missing files */
-static int ignore_missing_files;
+static bool ignore_missing_files;
 /* If set to 1, only warn (instead of error) about missing ns imports */
-static int allow_missing_ns_imports;
+static bool allow_missing_ns_imports;
 
 static bool error_occurred;
 
@@ -187,7 +187,7 @@ static struct module *new_module(const char *modname)
 	/* add to list */
 	strcpy(mod->name, modname);
 	mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
-	mod->gpl_compatible = -1;
+	mod->gpl_compatible = true;
 	mod->next = modules;
 	modules = mod;
 
@@ -203,10 +203,10 @@ struct symbol {
 	struct symbol *next;
 	struct module *module;
 	unsigned int crc;
-	int crc_valid;
+	bool crc_valid;
 	char *namespace;
-	unsigned int weak:1;
-	unsigned int is_static:1;  /* 1 if symbol is not global */
+	bool weak;
+	bool is_static;		/* true if symbol is not global */
 	enum export  export;       /* Type of export */
 	char name[];
 };
@@ -230,7 +230,7 @@ static inline unsigned int tdb_hash(const char *name)
  * Allocate a new symbols for use in the hash of exported symbols or
  * the list of unresolved symbols per module
  **/
-static struct symbol *alloc_symbol(const char *name, unsigned int weak,
+static struct symbol *alloc_symbol(const char *name, bool weak,
 				   struct symbol *next)
 {
 	struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
@@ -239,7 +239,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
 	strcpy(s->name, name);
 	s->weak = weak;
 	s->next = next;
-	s->is_static = 1;
+	s->is_static = true;
 	return s;
 }
 
@@ -250,7 +250,7 @@ static struct symbol *new_symbol(const char *name, struct module *module,
 	unsigned int hash;
 
 	hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
-	symbolhash[hash] = alloc_symbol(name, 0, symbolhash[hash]);
+	symbolhash[hash] = alloc_symbol(name, false, symbolhash[hash]);
 
 	return symbolhash[hash];
 }
@@ -419,7 +419,7 @@ static void sym_set_crc(const char *name, unsigned int crc)
 		return;
 
 	s->crc = crc;
-	s->crc_valid = 1;
+	s->crc_valid = true;
 }
 
 static void *grab_file(const char *filename, size_t *size)
@@ -716,9 +716,9 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 			sym_add_exported(name, mod, export);
 		}
 		if (strcmp(symname, "init_module") == 0)
-			mod->has_init = 1;
+			mod->has_init = true;
 		if (strcmp(symname, "cleanup_module") == 0)
-			mod->has_cleanup = 1;
+			mod->has_cleanup = true;
 		break;
 	}
 }
@@ -2008,9 +2008,9 @@ static void read_symbols(const char *modname)
 			error("missing MODULE_LICENSE() in %s\n", modname);
 		while (license) {
 			if (license_is_gpl_compatible(license))
-				mod->gpl_compatible = 1;
+				mod->gpl_compatible = true;
 			else {
-				mod->gpl_compatible = 0;
+				mod->gpl_compatible = false;
 				break;
 			}
 			license = get_next_modinfo(&info, "license", license);
@@ -2053,7 +2053,7 @@ static void read_symbols(const char *modname)
 						       sym->st_name));
 
 			if (s)
-				s->is_static = 0;
+				s->is_static = false;
 		}
 	}
 
@@ -2073,7 +2073,7 @@ static void read_symbols(const char *modname)
 	 * the automatic versioning doesn't pick it up, but it's really
 	 * important anyhow */
 	if (modversions)
-		mod->unres = alloc_symbol("module_layout", 0, mod->unres);
+		mod->unres = alloc_symbol("module_layout", false, mod->unres);
 }
 
 static void read_symbols_from_files(const char *filename)
@@ -2305,7 +2305,7 @@ static void add_depends(struct buffer *b, struct module *mod)
 		if (s->module->seen)
 			continue;
 
-		s->module->seen = 1;
+		s->module->seen = true;
 		p = strrchr(s->module->name, '/');
 		if (p)
 			p++;
@@ -2422,10 +2422,10 @@ static void read_dump(const char *fname)
 		mod = find_module(modname);
 		if (!mod) {
 			mod = new_module(modname);
-			mod->from_dump = 1;
+			mod->from_dump = true;
 		}
 		s = sym_add_exported(symname, mod, export_no(export));
-		s->is_static = 0;
+		s->is_static = false;
 		sym_set_crc(symname, crc);
 		sym_update_namespace(symname, namespace);
 	}
@@ -2503,7 +2503,7 @@ int main(int argc, char **argv)
 	while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
 		switch (opt) {
 		case 'e':
-			external_module = 1;
+			external_module = true;
 			break;
 		case 'i':
 			*dump_read_iter =
@@ -2512,28 +2512,28 @@ int main(int argc, char **argv)
 			dump_read_iter = &(*dump_read_iter)->next;
 			break;
 		case 'm':
-			modversions = 1;
+			modversions = true;
 			break;
 		case 'n':
-			ignore_missing_files = 1;
+			ignore_missing_files = true;
 			break;
 		case 'o':
 			dump_write = optarg;
 			break;
 		case 'a':
-			all_versions = 1;
+			all_versions = true;
 			break;
 		case 'T':
 			files_source = optarg;
 			break;
 		case 'w':
-			warn_unresolved = 1;
+			warn_unresolved = true;
 			break;
 		case 'E':
 			sec_mismatch_warn_only = false;
 			break;
 		case 'N':
-			allow_missing_ns_imports = 1;
+			allow_missing_ns_imports = true;
 			break;
 		case 'd':
 			missing_namespace_deps = optarg;
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index a85dcec3669a..4085bf5b33aa 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -1,4 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
+#include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
@@ -116,11 +117,10 @@ struct module {
 	struct module *next;
 	int gpl_compatible;
 	struct symbol *unres;
-	int from_dump;  /* 1 if module was loaded from *.symvers */
-	int is_vmlinux;
-	int seen;
-	int has_init;
-	int has_cleanup;
+	bool from_dump;		/* true if module was loaded from *.symvers */
+	bool is_vmlinux;
+	bool seen;
+	bool has_init, has_cleanup;
 	struct buffer dev_table_buf;
 	char	     srcversion[25];
 	// Missing namespace dependencies
-- 
2.32.0

Re: [PATCH 06/27] modpost: use bool type where appropriate

[Nick Desaulniers]

/On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Use 'bool' to clarify that the valid value is true or false.
>
> Here is a small note for the conversion.
>
> Strictly speaking, module::gpl_compatible was not boolean because
> new_module() initialized it to -1. Maybe, -1 was used to represent the
> license is 'unknown', but it is not useful.
>
> Since commit 1d6cd3929360 ("modpost: turn missing MODULE_LICENSE() into
> error"), unknown module license is not allowed anyway.
>
> I changed the initializer "= -1" to "= true". This has no functional
> change.
>
> The current code:
>
>     if (!mod->gpl_compatible)
>             check_for_gpl_usage(exp->export, basename, exp->name);
>
> ... only checks whether gpl_compabilt is zero or not:

s/gpl_compabilt/gpl_compatible/

Also the trailing `:` should perhaps be `.`.

Shouldn't gpl_compatible default to false, until proven otherwise?
What happens if you default to false? Perhaps an identifier like
`maybe_gpl_compatible` would be more descriptive?

Also, if we're going to rename a few vars, consider using prefixes
like is_*, has_*, or should_* for some of these to improve the
readability for boolean variables.

Otherwise LGTM.

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 60 +++++++++++++++++++++----------------------
>  scripts/mod/modpost.h | 10 ++++----
>  2 files changed, 35 insertions(+), 35 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f9cbb6b6b7a5..52dd07a36379 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -23,20 +23,20 @@
>  #include "../../include/linux/license.h"
>
>  /* Are we using CONFIG_MODVERSIONS? */
> -static int modversions;
> +static bool modversions;
>  /* Is CONFIG_MODULE_SRCVERSION_ALL set? */
> -static int all_versions;
> +static bool all_versions;
>  /* If we are modposting external module set to 1 */
> -static int external_module;
> +static bool external_module;
>  /* Only warn about unresolved symbols */
> -static int warn_unresolved;
> +static bool warn_unresolved;
>  /* How a symbol is exported */
>  static int sec_mismatch_count;
> -static int sec_mismatch_warn_only = true;
> +static bool sec_mismatch_warn_only = true;
>  /* ignore missing files */
> -static int ignore_missing_files;
> +static bool ignore_missing_files;
>  /* If set to 1, only warn (instead of error) about missing ns imports */
> -static int allow_missing_ns_imports;
> +static bool allow_missing_ns_imports;
>
>  static bool error_occurred;
>
> @@ -187,7 +187,7 @@ static struct module *new_module(const char *modname)
>         /* add to list */
>         strcpy(mod->name, modname);
>         mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
> -       mod->gpl_compatible = -1;
> +       mod->gpl_compatible = true;
>         mod->next = modules;
>         modules = mod;
>
> @@ -203,10 +203,10 @@ struct symbol {
>         struct symbol *next;
>         struct module *module;
>         unsigned int crc;
> -       int crc_valid;
> +       bool crc_valid;
>         char *namespace;
> -       unsigned int weak:1;
> -       unsigned int is_static:1;  /* 1 if symbol is not global */
> +       bool weak;
> +       bool is_static;         /* true if symbol is not global */
>         enum export  export;       /* Type of export */
>         char name[];
>  };
> @@ -230,7 +230,7 @@ static inline unsigned int tdb_hash(const char *name)
>   * Allocate a new symbols for use in the hash of exported symbols or
>   * the list of unresolved symbols per module
>   **/
> -static struct symbol *alloc_symbol(const char *name, unsigned int weak,
> +static struct symbol *alloc_symbol(const char *name, bool weak,
>                                    struct symbol *next)
>  {
>         struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
> @@ -239,7 +239,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
>         strcpy(s->name, name);
>         s->weak = weak;
>         s->next = next;
> -       s->is_static = 1;
> +       s->is_static = true;
>         return s;
>  }
>
> @@ -250,7 +250,7 @@ static struct symbol *new_symbol(const char *name, struct module *module,
>         unsigned int hash;
>
>         hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
> -       symbolhash[hash] = alloc_symbol(name, 0, symbolhash[hash]);
> +       symbolhash[hash] = alloc_symbol(name, false, symbolhash[hash]);
>
>         return symbolhash[hash];
>  }
> @@ -419,7 +419,7 @@ static void sym_set_crc(const char *name, unsigned int crc)
>                 return;
>
>         s->crc = crc;
> -       s->crc_valid = 1;
> +       s->crc_valid = true;
>  }
>
>  static void *grab_file(const char *filename, size_t *size)
> @@ -716,9 +716,9 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
>                         sym_add_exported(name, mod, export);
>                 }
>                 if (strcmp(symname, "init_module") == 0)
> -                       mod->has_init = 1;
> +                       mod->has_init = true;
>                 if (strcmp(symname, "cleanup_module") == 0)
> -                       mod->has_cleanup = 1;
> +                       mod->has_cleanup = true;
>                 break;
>         }
>  }
> @@ -2008,9 +2008,9 @@ static void read_symbols(const char *modname)
>                         error("missing MODULE_LICENSE() in %s\n", modname);
>                 while (license) {
>                         if (license_is_gpl_compatible(license))
> -                               mod->gpl_compatible = 1;
> +                               mod->gpl_compatible = true;
>                         else {
> -                               mod->gpl_compatible = 0;
> +                               mod->gpl_compatible = false;
>                                 break;
>                         }
>                         license = get_next_modinfo(&info, "license", license);
> @@ -2053,7 +2053,7 @@ static void read_symbols(const char *modname)
>                                                        sym->st_name));
>
>                         if (s)
> -                               s->is_static = 0;
> +                               s->is_static = false;
>                 }
>         }
>
> @@ -2073,7 +2073,7 @@ static void read_symbols(const char *modname)
>          * the automatic versioning doesn't pick it up, but it's really
>          * important anyhow */
>         if (modversions)
> -               mod->unres = alloc_symbol("module_layout", 0, mod->unres);
> +               mod->unres = alloc_symbol("module_layout", false, mod->unres);
>  }
>
>  static void read_symbols_from_files(const char *filename)
> @@ -2305,7 +2305,7 @@ static void add_depends(struct buffer *b, struct module *mod)
>                 if (s->module->seen)
>                         continue;
>
> -               s->module->seen = 1;
> +               s->module->seen = true;
>                 p = strrchr(s->module->name, '/');
>                 if (p)
>                         p++;
> @@ -2422,10 +2422,10 @@ static void read_dump(const char *fname)
>                 mod = find_module(modname);
>                 if (!mod) {
>                         mod = new_module(modname);
> -                       mod->from_dump = 1;
> +                       mod->from_dump = true;
>                 }
>                 s = sym_add_exported(symname, mod, export_no(export));
> -               s->is_static = 0;
> +               s->is_static = false;
>                 sym_set_crc(symname, crc);
>                 sym_update_namespace(symname, namespace);
>         }
> @@ -2503,7 +2503,7 @@ int main(int argc, char **argv)
>         while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
>                 switch (opt) {
>                 case 'e':
> -                       external_module = 1;
> +                       external_module = true;
>                         break;
>                 case 'i':
>                         *dump_read_iter =
> @@ -2512,28 +2512,28 @@ int main(int argc, char **argv)
>                         dump_read_iter = &(*dump_read_iter)->next;
>                         break;
>                 case 'm':
> -                       modversions = 1;
> +                       modversions = true;
>                         break;
>                 case 'n':
> -                       ignore_missing_files = 1;
> +                       ignore_missing_files = true;
>                         break;
>                 case 'o':
>                         dump_write = optarg;
>                         break;
>                 case 'a':
> -                       all_versions = 1;
> +                       all_versions = true;
>                         break;
>                 case 'T':
>                         files_source = optarg;
>                         break;
>                 case 'w':
> -                       warn_unresolved = 1;
> +                       warn_unresolved = true;
>                         break;
>                 case 'E':
>                         sec_mismatch_warn_only = false;
>                         break;
>                 case 'N':
> -                       allow_missing_ns_imports = 1;
> +                       allow_missing_ns_imports = true;
>                         break;
>                 case 'd':
>                         missing_namespace_deps = optarg;
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index a85dcec3669a..4085bf5b33aa 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -1,4 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <stdarg.h>
> @@ -116,11 +117,10 @@ struct module {
>         struct module *next;
>         int gpl_compatible;
>         struct symbol *unres;
> -       int from_dump;  /* 1 if module was loaded from *.symvers */
> -       int is_vmlinux;
> -       int seen;
> -       int has_init;
> -       int has_cleanup;
> +       bool from_dump;         /* true if module was loaded from *.symvers */
> +       bool is_vmlinux;
> +       bool seen;
> +       bool has_init, has_cleanup;
>         struct buffer dev_table_buf;
>         char         srcversion[25];
>         // Missing namespace dependencies
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 06/27] modpost: use bool type where appropriate

[Nick Desaulniers]

On Mon, Apr 25, 2022 at 11:34 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> /On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 60 +++++++++++++++++++++----------------------
> >  scripts/mod/modpost.h | 10 ++++----
> >  2 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index f9cbb6b6b7a5..52dd07a36379 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -203,10 +203,10 @@ struct symbol {
> >         struct symbol *next;
> >         struct module *module;
> >         unsigned int crc;
> > -       int crc_valid;
> > +       bool crc_valid;
> >         char *namespace;
> > -       unsigned int weak:1;
> > -       unsigned int is_static:1;  /* 1 if symbol is not global */
> > +       bool weak;
> > +       bool is_static;         /* true if symbol is not global */
> >         enum export  export;       /* Type of export */
> >         char name[];
> >  };

This will change the sizeof(struct symbol).  I'm guessing we have lots
of symbols to process? If we have many live at once, perhaps it would
be better to keep these as bitfields, but additionally move them to
the end of the struct definition so as to save space?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 06/27] modpost: use bool type where appropriate

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:56 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Apr 25, 2022 at 11:34 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > /On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > >  scripts/mod/modpost.c | 60 +++++++++++++++++++++----------------------
> > >  scripts/mod/modpost.h | 10 ++++----
> > >  2 files changed, 35 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > > index f9cbb6b6b7a5..52dd07a36379 100644
> > > --- a/scripts/mod/modpost.c
> > > +++ b/scripts/mod/modpost.c
> > > @@ -203,10 +203,10 @@ struct symbol {
> > >         struct symbol *next;
> > >         struct module *module;
> > >         unsigned int crc;
> > > -       int crc_valid;
> > > +       bool crc_valid;
> > >         char *namespace;
> > > -       unsigned int weak:1;
> > > -       unsigned int is_static:1;  /* 1 if symbol is not global */
> > > +       bool weak;
> > > +       bool is_static;         /* true if symbol is not global */
> > >         enum export  export;       /* Type of export */
> > >         char name[];
> > >  };
>
> This will change the sizeof(struct symbol).  I'm guessing we have lots
> of symbols to process? If we have many live at once, perhaps it would
> be better to keep these as bitfields, but additionally move them to
> the end of the struct definition so as to save space?


Not really.
On 64 bit systems, sizeof(struct symbol) is still 40 bytes.

We may save some space by using bit fields and pahole analysis,
but I do not think that is a big deal for userspace programs.

I tend to prioritize code readability.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 06/27] modpost: use bool type where appropriate

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:34 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> /On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Use 'bool' to clarify that the valid value is true or false.
> >
> > Here is a small note for the conversion.
> >
> > Strictly speaking, module::gpl_compatible was not boolean because
> > new_module() initialized it to -1. Maybe, -1 was used to represent the
> > license is 'unknown', but it is not useful.
> >
> > Since commit 1d6cd3929360 ("modpost: turn missing MODULE_LICENSE() into
> > error"), unknown module license is not allowed anyway.
> >
> > I changed the initializer "= -1" to "= true". This has no functional
> > change.
> >
> > The current code:
> >
> >     if (!mod->gpl_compatible)
> >             check_for_gpl_usage(exp->export, basename, exp->name);
> >
> > ... only checks whether gpl_compabilt is zero or not:
>
> s/gpl_compabilt/gpl_compatible/
>
> Also the trailing `:` should perhaps be `.`.
>
> Shouldn't gpl_compatible default to false, until proven otherwise?
> What happens if you default to false? Perhaps an identifier like
> `maybe_gpl_compatible` would be more descriptive?


The init value should be 'true'.

In v2, I split this change into a separate patch,
and added some comments.

https://patchwork.kernel.org/project/linux-kbuild/patch/20220501084032.1025918-3-masahiroy@kernel.org/

I hope it answered your question.




> Also, if we're going to rename a few vars, consider using prefixes
> like is_*, has_*, or should_* for some of these to improve the
> readability for boolean variables.


OK, I renamed it to is_gpl_compatible in v2.






> Otherwise LGTM.
>
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 60 +++++++++++++++++++++----------------------
> >  scripts/mod/modpost.h | 10 ++++----
> >  2 files changed, 35 insertions(+), 35 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index f9cbb6b6b7a5..52dd07a36379 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -23,20 +23,20 @@
> >  #include "../../include/linux/license.h"
> >
> >  /* Are we using CONFIG_MODVERSIONS? */
> > -static int modversions;
> > +static bool modversions;
> >  /* Is CONFIG_MODULE_SRCVERSION_ALL set? */
> > -static int all_versions;
> > +static bool all_versions;
> >  /* If we are modposting external module set to 1 */
> > -static int external_module;
> > +static bool external_module;
> >  /* Only warn about unresolved symbols */
> > -static int warn_unresolved;
> > +static bool warn_unresolved;
> >  /* How a symbol is exported */
> >  static int sec_mismatch_count;
> > -static int sec_mismatch_warn_only = true;
> > +static bool sec_mismatch_warn_only = true;
> >  /* ignore missing files */
> > -static int ignore_missing_files;
> > +static bool ignore_missing_files;
> >  /* If set to 1, only warn (instead of error) about missing ns imports */
> > -static int allow_missing_ns_imports;
> > +static bool allow_missing_ns_imports;
> >
> >  static bool error_occurred;
> >
> > @@ -187,7 +187,7 @@ static struct module *new_module(const char *modname)
> >         /* add to list */
> >         strcpy(mod->name, modname);
> >         mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
> > -       mod->gpl_compatible = -1;
> > +       mod->gpl_compatible = true;
> >         mod->next = modules;
> >         modules = mod;
> >
> > @@ -203,10 +203,10 @@ struct symbol {
> >         struct symbol *next;
> >         struct module *module;
> >         unsigned int crc;
> > -       int crc_valid;
> > +       bool crc_valid;
> >         char *namespace;
> > -       unsigned int weak:1;
> > -       unsigned int is_static:1;  /* 1 if symbol is not global */
> > +       bool weak;
> > +       bool is_static;         /* true if symbol is not global */
> >         enum export  export;       /* Type of export */
> >         char name[];
> >  };
> > @@ -230,7 +230,7 @@ static inline unsigned int tdb_hash(const char *name)
> >   * Allocate a new symbols for use in the hash of exported symbols or
> >   * the list of unresolved symbols per module
> >   **/
> > -static struct symbol *alloc_symbol(const char *name, unsigned int weak,
> > +static struct symbol *alloc_symbol(const char *name, bool weak,
> >                                    struct symbol *next)
> >  {
> >         struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
> > @@ -239,7 +239,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
> >         strcpy(s->name, name);
> >         s->weak = weak;
> >         s->next = next;
> > -       s->is_static = 1;
> > +       s->is_static = true;
> >         return s;
> >  }
> >
> > @@ -250,7 +250,7 @@ static struct symbol *new_symbol(const char *name, struct module *module,
> >         unsigned int hash;
> >
> >         hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
> > -       symbolhash[hash] = alloc_symbol(name, 0, symbolhash[hash]);
> > +       symbolhash[hash] = alloc_symbol(name, false, symbolhash[hash]);
> >
> >         return symbolhash[hash];
> >  }
> > @@ -419,7 +419,7 @@ static void sym_set_crc(const char *name, unsigned int crc)
> >                 return;
> >
> >         s->crc = crc;
> > -       s->crc_valid = 1;
> > +       s->crc_valid = true;
> >  }
> >
> >  static void *grab_file(const char *filename, size_t *size)
> > @@ -716,9 +716,9 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
> >                         sym_add_exported(name, mod, export);
> >                 }
> >                 if (strcmp(symname, "init_module") == 0)
> > -                       mod->has_init = 1;
> > +                       mod->has_init = true;
> >                 if (strcmp(symname, "cleanup_module") == 0)
> > -                       mod->has_cleanup = 1;
> > +                       mod->has_cleanup = true;
> >                 break;
> >         }
> >  }
> > @@ -2008,9 +2008,9 @@ static void read_symbols(const char *modname)
> >                         error("missing MODULE_LICENSE() in %s\n", modname);
> >                 while (license) {
> >                         if (license_is_gpl_compatible(license))
> > -                               mod->gpl_compatible = 1;
> > +                               mod->gpl_compatible = true;
> >                         else {
> > -                               mod->gpl_compatible = 0;
> > +                               mod->gpl_compatible = false;
> >                                 break;
> >                         }
> >                         license = get_next_modinfo(&info, "license", license);
> > @@ -2053,7 +2053,7 @@ static void read_symbols(const char *modname)
> >                                                        sym->st_name));
> >
> >                         if (s)
> > -                               s->is_static = 0;
> > +                               s->is_static = false;
> >                 }
> >         }
> >
> > @@ -2073,7 +2073,7 @@ static void read_symbols(const char *modname)
> >          * the automatic versioning doesn't pick it up, but it's really
> >          * important anyhow */
> >         if (modversions)
> > -               mod->unres = alloc_symbol("module_layout", 0, mod->unres);
> > +               mod->unres = alloc_symbol("module_layout", false, mod->unres);
> >  }
> >
> >  static void read_symbols_from_files(const char *filename)
> > @@ -2305,7 +2305,7 @@ static void add_depends(struct buffer *b, struct module *mod)
> >                 if (s->module->seen)
> >                         continue;
> >
> > -               s->module->seen = 1;
> > +               s->module->seen = true;
> >                 p = strrchr(s->module->name, '/');
> >                 if (p)
> >                         p++;
> > @@ -2422,10 +2422,10 @@ static void read_dump(const char *fname)
> >                 mod = find_module(modname);
> >                 if (!mod) {
> >                         mod = new_module(modname);
> > -                       mod->from_dump = 1;
> > +                       mod->from_dump = true;
> >                 }
> >                 s = sym_add_exported(symname, mod, export_no(export));
> > -               s->is_static = 0;
> > +               s->is_static = false;
> >                 sym_set_crc(symname, crc);
> >                 sym_update_namespace(symname, namespace);
> >         }
> > @@ -2503,7 +2503,7 @@ int main(int argc, char **argv)
> >         while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
> >                 switch (opt) {
> >                 case 'e':
> > -                       external_module = 1;
> > +                       external_module = true;
> >                         break;
> >                 case 'i':
> >                         *dump_read_iter =
> > @@ -2512,28 +2512,28 @@ int main(int argc, char **argv)
> >                         dump_read_iter = &(*dump_read_iter)->next;
> >                         break;
> >                 case 'm':
> > -                       modversions = 1;
> > +                       modversions = true;
> >                         break;
> >                 case 'n':
> > -                       ignore_missing_files = 1;
> > +                       ignore_missing_files = true;
> >                         break;
> >                 case 'o':
> >                         dump_write = optarg;
> >                         break;
> >                 case 'a':
> > -                       all_versions = 1;
> > +                       all_versions = true;
> >                         break;
> >                 case 'T':
> >                         files_source = optarg;
> >                         break;
> >                 case 'w':
> > -                       warn_unresolved = 1;
> > +                       warn_unresolved = true;
> >                         break;
> >                 case 'E':
> >                         sec_mismatch_warn_only = false;
> >                         break;
> >                 case 'N':
> > -                       allow_missing_ns_imports = 1;
> > +                       allow_missing_ns_imports = true;
> >                         break;
> >                 case 'd':
> >                         missing_namespace_deps = optarg;
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index a85dcec3669a..4085bf5b33aa 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -1,4 +1,5 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > +#include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <stdarg.h>
> > @@ -116,11 +117,10 @@ struct module {
> >         struct module *next;
> >         int gpl_compatible;
> >         struct symbol *unres;
> > -       int from_dump;  /* 1 if module was loaded from *.symvers */
> > -       int is_vmlinux;
> > -       int seen;
> > -       int has_init;
> > -       int has_cleanup;
> > +       bool from_dump;         /* true if module was loaded from *.symvers */
> > +       bool is_vmlinux;
> > +       bool seen;
> > +       bool has_init, has_cleanup;
> >         struct buffer dev_table_buf;
> >         char         srcversion[25];
> >         // Missing namespace dependencies
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH 07/27] modpost: import include/linux/list.h

[Masahiro Yamada]

Import include/linux/list.h to use convenient list macros in modpost.

I dropped kernel-space code such as {WRITE,READ}_ONCE etc. and unneeded
macros.

I also imported container_of() from include/linux/container_of.h and
type definitions from include/linux/types.h.

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

 scripts/mod/list.h | 336 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 336 insertions(+)
 create mode 100644 scripts/mod/list.h

diff --git a/scripts/mod/list.h b/scripts/mod/list.h
new file mode 100644
index 000000000000..c87583a71714
--- /dev/null
+++ b/scripts/mod/list.h
@@ -0,0 +1,336 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef LIST_H
+#define LIST_H
+
+#include <stdbool.h>
+#include <stddef.h>
+
+/* Are two types/vars the same type (ignoring qualifiers)? */
+#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr:	the pointer to the member.
+ * @type:	the type of the container struct this is embedded in.
+ * @member:	the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({				\
+	void *__mptr = (void *)(ptr);					\
+	_Static_assert(__same_type(*(ptr), ((type *)0)->member) ||	\
+		      __same_type(*(ptr), void),			\
+		      "pointer type mismatch in container_of()");	\
+	((type *)(__mptr - offsetof(type, member))); })
+
+#define LIST_POISON1  ((void *) 0x100)
+#define LIST_POISON2  ((void *) 0x122)
+
+/*
+ * Circular doubly linked list implementation.
+ *
+ * Some of the internal functions ("__xxx") are useful when
+ * manipulating whole lists rather than single entries, as
+ * sometimes we already know the next/prev entries and we can
+ * generate better code by using them directly rather than
+ * using the generic single-entry routines.
+ */
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+	struct list_head name = LIST_HEAD_INIT(name)
+
+/**
+ * INIT_LIST_HEAD - Initialize a list_head structure
+ * @list: list_head structure to be initialized.
+ *
+ * Initializes the list_head to point to itself.  If it is a list header,
+ * the result is an empty list.
+ */
+static inline void INIT_LIST_HEAD(struct list_head *list)
+{
+	list->next = list;
+	list->prev = list;
+}
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_add(struct list_head *new,
+			      struct list_head *prev,
+			      struct list_head *next)
+{
+	next->prev = new;
+	new->next = next;
+	new->prev = prev;
+	prev->next = new;
+}
+
+/**
+ * list_add - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+static inline void list_add(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head, head->next);
+}
+
+/**
+ * list_add_tail - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it before
+ *
+ * Insert a new entry before the specified head.
+ * This is useful for implementing queues.
+ */
+static inline void list_add_tail(struct list_head *new, struct list_head *head)
+{
+	__list_add(new, head->prev, head);
+}
+
+/*
+ * Delete a list entry by making the prev/next entries
+ * point to each other.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_del(struct list_head *prev, struct list_head *next)
+{
+	next->prev = prev;
+	prev->next = next;
+}
+
+static inline void __list_del_entry(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+}
+
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty() on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+static inline void list_del(struct list_head *entry)
+{
+	__list_del_entry(entry);
+	entry->next = LIST_POISON1;
+	entry->prev = LIST_POISON2;
+}
+
+/**
+ * list_is_head - tests whether @list is the list @head
+ * @list: the entry to test
+ * @head: the head of the list
+ */
+static inline int list_is_head(const struct list_head *list, const struct list_head *head)
+{
+	return list == head;
+}
+
+/**
+ * list_empty - tests whether a list is empty
+ * @head: the list to test.
+ */
+static inline int list_empty(const struct list_head *head)
+{
+	return head->next == head;
+}
+
+/**
+ * list_entry - get the struct for this entry
+ * @ptr:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_entry(ptr, type, member) \
+	container_of(ptr, type, member)
+
+/**
+ * list_first_entry - get the first element from a list
+ * @ptr:	the list head to take the element from.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ *
+ * Note, that list is expected to be not empty.
+ */
+#define list_first_entry(ptr, type, member) \
+	list_entry((ptr)->next, type, member)
+
+/**
+ * list_next_entry - get the next element in list
+ * @pos:	the type * to cursor
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_next_entry(pos, member) \
+	list_entry((pos)->member.next, typeof(*(pos)), member)
+
+/**
+ * list_entry_is_head - test if the entry points to the head of the list
+ * @pos:	the type * to cursor
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_entry_is_head(pos, head, member)				\
+	(&pos->member == (head))
+
+/**
+ * list_for_each_entry	-	iterate over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry(pos, head, member)				\
+	for (pos = list_first_entry(head, typeof(*pos), member);	\
+	     !list_entry_is_head(pos, head, member);			\
+	     pos = list_next_entry(pos, member))
+
+/**
+ * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry_safe(pos, n, head, member)			\
+	for (pos = list_first_entry(head, typeof(*pos), member),	\
+		n = list_next_entry(pos, member);			\
+	     !list_entry_is_head(pos, head, member);			\
+	     pos = n, n = list_next_entry(n, member))
+
+/*
+ * Double linked lists with a single pointer list head.
+ * Mostly useful for hash tables where the two pointer list head is
+ * too wasteful.
+ * You lose the ability to access the tail in O(1).
+ */
+
+struct hlist_head {
+	struct hlist_node *first;
+};
+
+struct hlist_node {
+	struct hlist_node *next, **pprev;
+};
+
+#define HLIST_HEAD_INIT { .first = NULL }
+#define HLIST_HEAD(name) struct hlist_head name = {  .first = NULL }
+#define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)
+static inline void INIT_HLIST_NODE(struct hlist_node *h)
+{
+	h->next = NULL;
+	h->pprev = NULL;
+}
+
+/**
+ * hlist_unhashed - Has node been removed from list and reinitialized?
+ * @h: Node to be checked
+ *
+ * Not that not all removal functions will leave a node in unhashed
+ * state.  For example, hlist_nulls_del_init_rcu() does leave the
+ * node in unhashed state, but hlist_nulls_del() does not.
+ */
+static inline int hlist_unhashed(const struct hlist_node *h)
+{
+	return !h->pprev;
+}
+
+static inline void __hlist_del(struct hlist_node *n)
+{
+	struct hlist_node *next = n->next;
+	struct hlist_node **pprev = n->pprev;
+
+	*pprev = next;
+	if (next)
+		next->pprev = pprev;
+}
+
+/**
+ * hlist_del - Delete the specified hlist_node from its list
+ * @n: Node to delete.
+ *
+ * Note that this function leaves the node in hashed state.  Use
+ * hlist_del_init() or similar instead to unhash @n.
+ */
+static inline void hlist_del(struct hlist_node *n)
+{
+	__hlist_del(n);
+	n->next = LIST_POISON1;
+	n->pprev = LIST_POISON2;
+}
+
+/**
+ * hlist_del_init - Delete the specified hlist_node from its list and initialize
+ * @n: Node to delete.
+ *
+ * Note that this function leaves the node in unhashed state.
+ */
+static inline void hlist_del_init(struct hlist_node *n)
+{
+	if (!hlist_unhashed(n)) {
+		__hlist_del(n);
+		INIT_HLIST_NODE(n);
+	}
+}
+
+/**
+ * hlist_add_head - add a new entry at the beginning of the hlist
+ * @n: new entry to be added
+ * @h: hlist head to add it after
+ *
+ * Insert a new entry after the specified head.
+ * This is good for implementing stacks.
+ */
+static inline void hlist_add_head(struct hlist_node *n, struct hlist_head *h)
+{
+	struct hlist_node *first = h->first;
+	n->next = first;
+	if (first)
+		first->pprev = &n->next;
+	h->first = n;
+	n->pprev = &h->first;
+}
+
+#define hlist_entry(ptr, type, member) container_of(ptr, type, member)
+
+#define hlist_entry_safe(ptr, type, member) \
+	({ typeof(ptr) ____ptr = (ptr); \
+	   ____ptr ? hlist_entry(____ptr, type, member) : NULL; \
+	})
+
+/**
+ * hlist_for_each_entry	- iterate over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry(pos, head, member)				\
+	for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
+	     pos;							\
+	     pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
+
+/**
+ * hlist_for_each_entry_safe - iterate over list of given type safe against removal of list entry
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		a &struct hlist_node to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the hlist_node within the struct.
+ */
+#define hlist_for_each_entry_safe(pos, n, head, member)			\
+	for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
+	     pos && ({ n = pos->member.next; 1; });			\
+	     pos = hlist_entry_safe(n, typeof(*pos), member))
+
+#endif /* LIST_H */
-- 
2.32.0

Re: [PATCH 07/27] modpost: import include/linux/list.h

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Import include/linux/list.h to use convenient list macros in modpost.
>
> I dropped kernel-space code such as {WRITE,READ}_ONCE etc. and unneeded
> macros.
>
> I also imported container_of() from include/linux/container_of.h and
> type definitions from include/linux/types.h.

Is there a better way to just use the kernel headers? I kind of hate
copy+paste since the in tree duplication will diverge over time.

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/list.h | 336 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 336 insertions(+)
>  create mode 100644 scripts/mod/list.h
>
> diff --git a/scripts/mod/list.h b/scripts/mod/list.h
> new file mode 100644
> index 000000000000..c87583a71714
> --- /dev/null
> +++ b/scripts/mod/list.h
> @@ -0,0 +1,336 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LIST_H
> +#define LIST_H
> +
> +#include <stdbool.h>
> +#include <stddef.h>
> +
> +/* Are two types/vars the same type (ignoring qualifiers)? */
> +#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
> +
> +/**
> + * container_of - cast a member of a structure out to the containing structure
> + * @ptr:       the pointer to the member.
> + * @type:      the type of the container struct this is embedded in.
> + * @member:    the name of the member within the struct.
> + *
> + */
> +#define container_of(ptr, type, member) ({                             \
> +       void *__mptr = (void *)(ptr);                                   \
> +       _Static_assert(__same_type(*(ptr), ((type *)0)->member) ||      \
> +                     __same_type(*(ptr), void),                        \
> +                     "pointer type mismatch in container_of()");       \
> +       ((type *)(__mptr - offsetof(type, member))); })
> +
> +#define LIST_POISON1  ((void *) 0x100)
> +#define LIST_POISON2  ((void *) 0x122)
> +
> +/*
> + * Circular doubly linked list implementation.
> + *
> + * Some of the internal functions ("__xxx") are useful when
> + * manipulating whole lists rather than single entries, as
> + * sometimes we already know the next/prev entries and we can
> + * generate better code by using them directly rather than
> + * using the generic single-entry routines.
> + */
> +
> +struct list_head {
> +       struct list_head *next, *prev;
> +};
> +
> +#define LIST_HEAD_INIT(name) { &(name), &(name) }
> +
> +#define LIST_HEAD(name) \
> +       struct list_head name = LIST_HEAD_INIT(name)
> +
> +/**
> + * INIT_LIST_HEAD - Initialize a list_head structure
> + * @list: list_head structure to be initialized.
> + *
> + * Initializes the list_head to point to itself.  If it is a list header,
> + * the result is an empty list.
> + */
> +static inline void INIT_LIST_HEAD(struct list_head *list)
> +{
> +       list->next = list;
> +       list->prev = list;
> +}
> +
> +/*
> + * Insert a new entry between two known consecutive entries.
> + *
> + * This is only for internal list manipulation where we know
> + * the prev/next entries already!
> + */
> +static inline void __list_add(struct list_head *new,
> +                             struct list_head *prev,
> +                             struct list_head *next)
> +{
> +       next->prev = new;
> +       new->next = next;
> +       new->prev = prev;
> +       prev->next = new;
> +}
> +
> +/**
> + * list_add - add a new entry
> + * @new: new entry to be added
> + * @head: list head to add it after
> + *
> + * Insert a new entry after the specified head.
> + * This is good for implementing stacks.
> + */
> +static inline void list_add(struct list_head *new, struct list_head *head)
> +{
> +       __list_add(new, head, head->next);
> +}
> +
> +/**
> + * list_add_tail - add a new entry
> + * @new: new entry to be added
> + * @head: list head to add it before
> + *
> + * Insert a new entry before the specified head.
> + * This is useful for implementing queues.
> + */
> +static inline void list_add_tail(struct list_head *new, struct list_head *head)
> +{
> +       __list_add(new, head->prev, head);
> +}
> +
> +/*
> + * Delete a list entry by making the prev/next entries
> + * point to each other.
> + *
> + * This is only for internal list manipulation where we know
> + * the prev/next entries already!
> + */
> +static inline void __list_del(struct list_head *prev, struct list_head *next)
> +{
> +       next->prev = prev;
> +       prev->next = next;
> +}
> +
> +static inline void __list_del_entry(struct list_head *entry)
> +{
> +       __list_del(entry->prev, entry->next);
> +}
> +
> +/**
> + * list_del - deletes entry from list.
> + * @entry: the element to delete from the list.
> + * Note: list_empty() on entry does not return true after this, the entry is
> + * in an undefined state.
> + */
> +static inline void list_del(struct list_head *entry)
> +{
> +       __list_del_entry(entry);
> +       entry->next = LIST_POISON1;
> +       entry->prev = LIST_POISON2;
> +}
> +
> +/**
> + * list_is_head - tests whether @list is the list @head
> + * @list: the entry to test
> + * @head: the head of the list
> + */
> +static inline int list_is_head(const struct list_head *list, const struct list_head *head)
> +{
> +       return list == head;
> +}
> +
> +/**
> + * list_empty - tests whether a list is empty
> + * @head: the list to test.
> + */
> +static inline int list_empty(const struct list_head *head)
> +{
> +       return head->next == head;
> +}
> +
> +/**
> + * list_entry - get the struct for this entry
> + * @ptr:       the &struct list_head pointer.
> + * @type:      the type of the struct this is embedded in.
> + * @member:    the name of the list_head within the struct.
> + */
> +#define list_entry(ptr, type, member) \
> +       container_of(ptr, type, member)
> +
> +/**
> + * list_first_entry - get the first element from a list
> + * @ptr:       the list head to take the element from.
> + * @type:      the type of the struct this is embedded in.
> + * @member:    the name of the list_head within the struct.
> + *
> + * Note, that list is expected to be not empty.
> + */
> +#define list_first_entry(ptr, type, member) \
> +       list_entry((ptr)->next, type, member)
> +
> +/**
> + * list_next_entry - get the next element in list
> + * @pos:       the type * to cursor
> + * @member:    the name of the list_head within the struct.
> + */
> +#define list_next_entry(pos, member) \
> +       list_entry((pos)->member.next, typeof(*(pos)), member)
> +
> +/**
> + * list_entry_is_head - test if the entry points to the head of the list
> + * @pos:       the type * to cursor
> + * @head:      the head for your list.
> + * @member:    the name of the list_head within the struct.
> + */
> +#define list_entry_is_head(pos, head, member)                          \
> +       (&pos->member == (head))
> +
> +/**
> + * list_for_each_entry -       iterate over list of given type
> + * @pos:       the type * to use as a loop cursor.
> + * @head:      the head for your list.
> + * @member:    the name of the list_head within the struct.
> + */
> +#define list_for_each_entry(pos, head, member)                         \
> +       for (pos = list_first_entry(head, typeof(*pos), member);        \
> +            !list_entry_is_head(pos, head, member);                    \
> +            pos = list_next_entry(pos, member))
> +
> +/**
> + * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
> + * @pos:       the type * to use as a loop cursor.
> + * @n:         another type * to use as temporary storage
> + * @head:      the head for your list.
> + * @member:    the name of the list_head within the struct.
> + */
> +#define list_for_each_entry_safe(pos, n, head, member)                 \
> +       for (pos = list_first_entry(head, typeof(*pos), member),        \
> +               n = list_next_entry(pos, member);                       \
> +            !list_entry_is_head(pos, head, member);                    \
> +            pos = n, n = list_next_entry(n, member))
> +
> +/*
> + * Double linked lists with a single pointer list head.
> + * Mostly useful for hash tables where the two pointer list head is
> + * too wasteful.
> + * You lose the ability to access the tail in O(1).
> + */
> +
> +struct hlist_head {
> +       struct hlist_node *first;
> +};
> +
> +struct hlist_node {
> +       struct hlist_node *next, **pprev;
> +};
> +
> +#define HLIST_HEAD_INIT { .first = NULL }
> +#define HLIST_HEAD(name) struct hlist_head name = {  .first = NULL }
> +#define INIT_HLIST_HEAD(ptr) ((ptr)->first = NULL)
> +static inline void INIT_HLIST_NODE(struct hlist_node *h)
> +{
> +       h->next = NULL;
> +       h->pprev = NULL;
> +}
> +
> +/**
> + * hlist_unhashed - Has node been removed from list and reinitialized?
> + * @h: Node to be checked
> + *
> + * Not that not all removal functions will leave a node in unhashed
> + * state.  For example, hlist_nulls_del_init_rcu() does leave the
> + * node in unhashed state, but hlist_nulls_del() does not.
> + */
> +static inline int hlist_unhashed(const struct hlist_node *h)
> +{
> +       return !h->pprev;
> +}
> +
> +static inline void __hlist_del(struct hlist_node *n)
> +{
> +       struct hlist_node *next = n->next;
> +       struct hlist_node **pprev = n->pprev;
> +
> +       *pprev = next;
> +       if (next)
> +               next->pprev = pprev;
> +}
> +
> +/**
> + * hlist_del - Delete the specified hlist_node from its list
> + * @n: Node to delete.
> + *
> + * Note that this function leaves the node in hashed state.  Use
> + * hlist_del_init() or similar instead to unhash @n.
> + */
> +static inline void hlist_del(struct hlist_node *n)
> +{
> +       __hlist_del(n);
> +       n->next = LIST_POISON1;
> +       n->pprev = LIST_POISON2;
> +}
> +
> +/**
> + * hlist_del_init - Delete the specified hlist_node from its list and initialize
> + * @n: Node to delete.
> + *
> + * Note that this function leaves the node in unhashed state.
> + */
> +static inline void hlist_del_init(struct hlist_node *n)
> +{
> +       if (!hlist_unhashed(n)) {
> +               __hlist_del(n);
> +               INIT_HLIST_NODE(n);
> +       }
> +}
> +
> +/**
> + * hlist_add_head - add a new entry at the beginning of the hlist
> + * @n: new entry to be added
> + * @h: hlist head to add it after
> + *
> + * Insert a new entry after the specified head.
> + * This is good for implementing stacks.
> + */
> +static inline void hlist_add_head(struct hlist_node *n, struct hlist_head *h)
> +{
> +       struct hlist_node *first = h->first;
> +       n->next = first;
> +       if (first)
> +               first->pprev = &n->next;
> +       h->first = n;
> +       n->pprev = &h->first;
> +}
> +
> +#define hlist_entry(ptr, type, member) container_of(ptr, type, member)
> +
> +#define hlist_entry_safe(ptr, type, member) \
> +       ({ typeof(ptr) ____ptr = (ptr); \
> +          ____ptr ? hlist_entry(____ptr, type, member) : NULL; \
> +       })
> +
> +/**
> + * hlist_for_each_entry        - iterate over list of given type
> + * @pos:       the type * to use as a loop cursor.
> + * @head:      the head for your list.
> + * @member:    the name of the hlist_node within the struct.
> + */
> +#define hlist_for_each_entry(pos, head, member)                                \
> +       for (pos = hlist_entry_safe((head)->first, typeof(*(pos)), member);\
> +            pos;                                                       \
> +            pos = hlist_entry_safe((pos)->member.next, typeof(*(pos)), member))
> +
> +/**
> + * hlist_for_each_entry_safe - iterate over list of given type safe against removal of list entry
> + * @pos:       the type * to use as a loop cursor.
> + * @n:         a &struct hlist_node to use as temporary storage
> + * @head:      the head for your list.
> + * @member:    the name of the hlist_node within the struct.
> + */
> +#define hlist_for_each_entry_safe(pos, n, head, member)                        \
> +       for (pos = hlist_entry_safe((head)->first, typeof(*pos), member);\
> +            pos && ({ n = pos->member.next; 1; });                     \
> +            pos = hlist_entry_safe(n, typeof(*pos), member))
> +
> +#endif /* LIST_H */
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 07/27] modpost: import include/linux/list.h

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:42 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Import include/linux/list.h to use convenient list macros in modpost.
> >
> > I dropped kernel-space code such as {WRITE,READ}_ONCE etc. and unneeded
> > macros.
> >
> > I also imported container_of() from include/linux/container_of.h and
> > type definitions from include/linux/types.h.
>
> Is there a better way to just use the kernel headers?

Presumably no.
include/linux/list.h includes more.

Adding so many #ifdef __KERNEL__ stubs
would be really ugly.


> I kind of hate
> copy+paste since the in tree duplication will diverge over time.

I only cherry-picked some macros from include/linux/kernel.h

No need to resync.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH 07/27] modpost: import include/linux/list.h

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Import include/linux/list.h to use convenient list macros in modpost.
>
> I dropped kernel-space code such as {WRITE,READ}_ONCE etc. and unneeded
> macros.
>
> I also imported container_of() from include/linux/container_of.h and
> type definitions from include/linux/types.h.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Ok then, just two small nits about two comments, which may have been
just copied over from the sources.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>
>  scripts/mod/list.h | 336 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 336 insertions(+)
>  create mode 100644 scripts/mod/list.h
>
> diff --git a/scripts/mod/list.h b/scripts/mod/list.h
> new file mode 100644
> index 000000000000..c87583a71714
> --- /dev/null
> +++ b/scripts/mod/list.h
> @@ -0,0 +1,336 @@

<snip>

> +/**
> + * list_for_each_entry -       iterate over list of given type

^ Excessive whitespace after the `-`

> + * @pos:       the type * to use as a loop cursor.
> + * @head:      the head for your list.
> + * @member:    the name of the list_head within the struct.
> + */
> +#define list_for_each_entry(pos, head, member)                         \
> +       for (pos = list_first_entry(head, typeof(*pos), member);        \
> +            !list_entry_is_head(pos, head, member);                    \
> +            pos = list_next_entry(pos, member))
> +
> +/**
> + * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry

^ This sounds like two sentences and looks like it's missing
punctuation separating them?

"iterate over list of given type. Safe against removal of list entry"
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 07/27] modpost: import include/linux/list.h

[Masahiro Yamada]

On Wed, Apr 27, 2022 at 1:30 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Import include/linux/list.h to use convenient list macros in modpost.
> >
> > I dropped kernel-space code such as {WRITE,READ}_ONCE etc. and unneeded
> > macros.
> >
> > I also imported container_of() from include/linux/container_of.h and
> > type definitions from include/linux/types.h.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Ok then, just two small nits about two comments, which may have been
> just copied over from the sources.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> > ---
> >
> >  scripts/mod/list.h | 336 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 336 insertions(+)
> >  create mode 100644 scripts/mod/list.h
> >
> > diff --git a/scripts/mod/list.h b/scripts/mod/list.h
> > new file mode 100644
> > index 000000000000..c87583a71714
> > --- /dev/null
> > +++ b/scripts/mod/list.h
> > @@ -0,0 +1,336 @@
>
> <snip>
>
> > +/**
> > + * list_for_each_entry -       iterate over list of given type
>
> ^ Excessive whitespace after the `-`
>
> > + * @pos:       the type * to use as a loop cursor.
> > + * @head:      the head for your list.
> > + * @member:    the name of the list_head within the struct.
> > + */
> > +#define list_for_each_entry(pos, head, member)                         \
> > +       for (pos = list_first_entry(head, typeof(*pos), member);        \
> > +            !list_entry_is_head(pos, head, member);                    \
> > +            pos = list_next_entry(pos, member))
> > +
> > +/**
> > + * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
>
> ^ This sounds like two sentences and looks like it's missing
> punctuation separating them?
>
> "iterate over list of given type. Safe against removal of list entry"


OK, I will fold your suggestions.

If you have a chance to fix up include/linux/list.h in the same way,
that would be appreciated.

./scripts/checkpatch.pl  -f  include/linux/list.h

might be useful to find out style issues.



-- 
Best Regards
Masahiro Yamada

[PATCH 08/27] modpost: traverse modules in order

[Masahiro Yamada]

Currently, modpost manages modules in a singly liked list; it adds a new
node to the head, and traverses the list from new to old.

It works, but the error messages are shown in the reverse order.

If you have a Makefile like this:

  obj-m += foo.o bar.o

then, modpost shows error messages in bar.o, foo.o, in this order.

Use a doubly linked list to keep the order in modules.order; use
list_add_tail() for the node addition and list_for_each_entry() for
the list traverse.

Now that the kernel's list macros have been imported to modpost, I will
use them actively going forward.

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

 scripts/mod/modpost.c | 18 +++++++++---------
 scripts/mod/modpost.h |  3 ++-
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 52dd07a36379..86416e4af626 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -165,16 +165,17 @@ char *get_line(char **stringp)
 }
 
 /* A list of all modules we processed */
-static struct module *modules;
+LIST_HEAD(modules);
 
 static struct module *find_module(const char *modname)
 {
 	struct module *mod;
 
-	for (mod = modules; mod; mod = mod->next)
+	list_for_each_entry(mod, &modules, list) {
 		if (strcmp(mod->name, modname) == 0)
-			break;
-	return mod;
+			return mod;
+	}
+	return NULL;
 }
 
 static struct module *new_module(const char *modname)
@@ -184,12 +185,11 @@ static struct module *new_module(const char *modname)
 	mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
 	memset(mod, 0, sizeof(*mod));
 
-	/* add to list */
 	strcpy(mod->name, modname);
 	mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
 	mod->gpl_compatible = true;
-	mod->next = modules;
-	modules = mod;
+
+	list_add_tail(&mod->list, &modules);
 
 	return mod;
 }
@@ -2467,7 +2467,7 @@ static void write_namespace_deps_files(const char *fname)
 	struct namespace_list *ns;
 	struct buffer ns_deps_buf = {};
 
-	for (mod = modules; mod; mod = mod->next) {
+	list_for_each_entry(mod, &modules, list) {
 
 		if (mod->from_dump || !mod->missing_namespaces)
 			continue;
@@ -2558,7 +2558,7 @@ int main(int argc, char **argv)
 	if (files_source)
 		read_symbols_from_files(files_source);
 
-	for (mod = modules; mod; mod = mod->next) {
+	list_for_each_entry(mod, &modules, list) {
 		char fname[PATH_MAX];
 		int ret;
 
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 4085bf5b33aa..c3b5d2f0e2bb 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -11,6 +11,7 @@
 #include <unistd.h>
 #include <elf.h>
 
+#include "list.h"
 #include "elfconfig.h"
 
 /* On BSD-alike OSes elf.h defines these according to host's word size */
@@ -114,7 +115,7 @@ struct namespace_list {
 };
 
 struct module {
-	struct module *next;
+	struct list_head list;
 	int gpl_compatible;
 	struct symbol *unres;
 	bool from_dump;		/* true if module was loaded from *.symvers */
-- 
2.32.0

Re: [PATCH 08/27] modpost: traverse modules in order

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Currently, modpost manages modules in a singly liked list; it adds a new

s/liked/linked/

> node to the head, and traverses the list from new to old.
>
> It works, but the error messages are shown in the reverse order.
>
> If you have a Makefile like this:
>
>   obj-m += foo.o bar.o
>
> then, modpost shows error messages in bar.o, foo.o, in this order.
>
> Use a doubly linked list to keep the order in modules.order; use
> list_add_tail() for the node addition and list_for_each_entry() for
> the list traverse.
>
> Now that the kernel's list macros have been imported to modpost, I will
> use them actively going forward.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

[PATCH 09/27] modpost: add sym_add_unresolved() helper

[Masahiro Yamada]

Add a small helper, sym_add_unresolved() to ease the further
refactoring.

Remove the 'weak' argument from alloc_symbol() because it is sensible
only for unresolved symbols.

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

 scripts/mod/modpost.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 86416e4af626..1c7d2831e89d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -230,14 +230,12 @@ static inline unsigned int tdb_hash(const char *name)
  * Allocate a new symbols for use in the hash of exported symbols or
  * the list of unresolved symbols per module
  **/
-static struct symbol *alloc_symbol(const char *name, bool weak,
-				   struct symbol *next)
+static struct symbol *alloc_symbol(const char *name, struct symbol *next)
 {
 	struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
 
 	memset(s, 0, sizeof(*s));
 	strcpy(s->name, name);
-	s->weak = weak;
 	s->next = next;
 	s->is_static = true;
 	return s;
@@ -250,11 +248,17 @@ static struct symbol *new_symbol(const char *name, struct module *module,
 	unsigned int hash;
 
 	hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
-	symbolhash[hash] = alloc_symbol(name, false, symbolhash[hash]);
+	symbolhash[hash] = alloc_symbol(name, symbolhash[hash]);
 
 	return symbolhash[hash];
 }
 
+static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
+{
+	mod->unres = alloc_symbol(name, mod->unres);
+	mod->unres->weak = weak;
+}
+
 static struct symbol *find_symbol(const char *name)
 {
 	struct symbol *s;
@@ -701,9 +705,8 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
 			}
 		}
 
-		mod->unres = alloc_symbol(symname,
-					  ELF_ST_BIND(sym->st_info) == STB_WEAK,
-					  mod->unres);
+		sym_add_unresolved(symname, mod,
+				   ELF_ST_BIND(sym->st_info) == STB_WEAK);
 		break;
 	default:
 		/* All exported symbols */
@@ -2073,7 +2076,7 @@ static void read_symbols(const char *modname)
 	 * the automatic versioning doesn't pick it up, but it's really
 	 * important anyhow */
 	if (modversions)
-		mod->unres = alloc_symbol("module_layout", false, mod->unres);
+		sym_add_unresolved("module_layout", mod, false);
 }
 
 static void read_symbols_from_files(const char *filename)
-- 
2.32.0

Re: [PATCH 09/27] modpost: add sym_add_unresolved() helper

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Add a small helper, sym_add_unresolved() to ease the further
> refactoring.
>
> Remove the 'weak' argument from alloc_symbol() because it is sensible
> only for unresolved symbols.

I did not yet read the rest of the series to see how else your newly
added helper `sym_add_unresolved` is used.
Perhaps the callers of `alloc_symbol` should just set the symbol's
weak member to true if needed, and alloc_symbol can default to setting
it false (as the memset currently does)?

Then, you don't need the helper, and just `handle_symbol` needs the
assignment when `ELF_ST_BIND(sym->st_info) == STB_WEAK`?

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 86416e4af626..1c7d2831e89d 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -230,14 +230,12 @@ static inline unsigned int tdb_hash(const char *name)
>   * Allocate a new symbols for use in the hash of exported symbols or
>   * the list of unresolved symbols per module
>   **/
> -static struct symbol *alloc_symbol(const char *name, bool weak,
> -                                  struct symbol *next)
> +static struct symbol *alloc_symbol(const char *name, struct symbol *next)
>  {
>         struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
>
>         memset(s, 0, sizeof(*s));
>         strcpy(s->name, name);
> -       s->weak = weak;
>         s->next = next;
>         s->is_static = true;
>         return s;
> @@ -250,11 +248,17 @@ static struct symbol *new_symbol(const char *name, struct module *module,
>         unsigned int hash;
>
>         hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
> -       symbolhash[hash] = alloc_symbol(name, false, symbolhash[hash]);
> +       symbolhash[hash] = alloc_symbol(name, symbolhash[hash]);
>
>         return symbolhash[hash];
>  }
>
> +static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
> +{
> +       mod->unres = alloc_symbol(name, mod->unres);
> +       mod->unres->weak = weak;
> +}
> +
>  static struct symbol *find_symbol(const char *name)
>  {
>         struct symbol *s;
> @@ -701,9 +705,8 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
>                         }
>                 }
>
> -               mod->unres = alloc_symbol(symname,
> -                                         ELF_ST_BIND(sym->st_info) == STB_WEAK,
> -                                         mod->unres);
> +               sym_add_unresolved(symname, mod,
> +                                  ELF_ST_BIND(sym->st_info) == STB_WEAK);
>                 break;
>         default:
>                 /* All exported symbols */
> @@ -2073,7 +2076,7 @@ static void read_symbols(const char *modname)
>          * the automatic versioning doesn't pick it up, but it's really
>          * important anyhow */
>         if (modversions)
> -               mod->unres = alloc_symbol("module_layout", false, mod->unres);
> +               sym_add_unresolved("module_layout", mod, false);
>  }
>
>  static void read_symbols_from_files(const char *filename)
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 09/27] modpost: add sym_add_unresolved() helper

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:41 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Add a small helper, sym_add_unresolved() to ease the further
> > refactoring.
> >
> > Remove the 'weak' argument from alloc_symbol() because it is sensible
> > only for unresolved symbols.
>
> I did not yet read the rest of the series to see how else your newly
> added helper `sym_add_unresolved` is used.
> Perhaps the callers of `alloc_symbol` should just set the symbol's
> weak member to true if needed, and alloc_symbol can default to setting
> it false (as the memset currently does)?
>
> Then, you don't need the helper, and just `handle_symbol` needs the
> assignment when `ELF_ST_BIND(sym->st_info) == STB_WEAK`?


I will change this in the later commit:
https://patchwork.kernel.org/project/linux-kbuild/patch/20220424190811.1678416-11-masahiroy@kernel.org/

I think this is a good case for a new helper.

If you look at the entire series,
"allocate a new symbol and connect it to the proper linked list or hash_table"
is consistently done in a helper function.


Also, I chose the function name as they look symmetrical.

 sym_add_unresolved()
 sym_add_exported()
 sym_add_crc()




> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 86416e4af626..1c7d2831e89d 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -230,14 +230,12 @@ static inline unsigned int tdb_hash(const char *name)
> >   * Allocate a new symbols for use in the hash of exported symbols or
> >   * the list of unresolved symbols per module
> >   **/
> > -static struct symbol *alloc_symbol(const char *name, bool weak,
> > -                                  struct symbol *next)
> > +static struct symbol *alloc_symbol(const char *name, struct symbol *next)
> >  {
> >         struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
> >
> >         memset(s, 0, sizeof(*s));
> >         strcpy(s->name, name);
> > -       s->weak = weak;
> >         s->next = next;
> >         s->is_static = true;
> >         return s;
> > @@ -250,11 +248,17 @@ static struct symbol *new_symbol(const char *name, struct module *module,
> >         unsigned int hash;
> >
> >         hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
> > -       symbolhash[hash] = alloc_symbol(name, false, symbolhash[hash]);
> > +       symbolhash[hash] = alloc_symbol(name, symbolhash[hash]);
> >
> >         return symbolhash[hash];
> >  }
> >
> > +static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
> > +{
> > +       mod->unres = alloc_symbol(name, mod->unres);
> > +       mod->unres->weak = weak;
> > +}
> > +
> >  static struct symbol *find_symbol(const char *name)
> >  {
> >         struct symbol *s;
> > @@ -701,9 +705,8 @@ static void handle_symbol(struct module *mod, struct elf_info *info,
> >                         }
> >                 }
> >
> > -               mod->unres = alloc_symbol(symname,
> > -                                         ELF_ST_BIND(sym->st_info) == STB_WEAK,
> > -                                         mod->unres);
> > +               sym_add_unresolved(symname, mod,
> > +                                  ELF_ST_BIND(sym->st_info) == STB_WEAK);
> >                 break;
> >         default:
> >                 /* All exported symbols */
> > @@ -2073,7 +2076,7 @@ static void read_symbols(const char *modname)
> >          * the automatic versioning doesn't pick it up, but it's really
> >          * important anyhow */
> >         if (modversions)
> > -               mod->unres = alloc_symbol("module_layout", false, mod->unres);
> > +               sym_add_unresolved("module_layout", mod, false);
> >  }
> >
> >  static void read_symbols_from_files(const char *filename)
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 09/27] modpost: add sym_add_unresolved() helper

[Nick Desaulniers]

On Mon, Apr 25, 2022 at 9:00 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Apr 26, 2022 at 3:41 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > Add a small helper, sym_add_unresolved() to ease the further
> > > refactoring.
> > >
> > > Remove the 'weak' argument from alloc_symbol() because it is sensible
> > > only for unresolved symbols.
> >
> > I did not yet read the rest of the series to see how else your newly
> > added helper `sym_add_unresolved` is used.
> > Perhaps the callers of `alloc_symbol` should just set the symbol's
> > weak member to true if needed, and alloc_symbol can default to setting
> > it false (as the memset currently does)?
> >
> > Then, you don't need the helper, and just `handle_symbol` needs the
> > assignment when `ELF_ST_BIND(sym->st_info) == STB_WEAK`?
>
>
> I will change this in the later commit:
> https://patchwork.kernel.org/project/linux-kbuild/patch/20220424190811.1678416-11-masahiroy@kernel.org/
>
> I think this is a good case for a new helper.
>
> If you look at the entire series,
> "allocate a new symbol and connect it to the proper linked list or hash_table"
> is consistently done in a helper function.
>
>
> Also, I chose the function name as they look symmetrical.
>
>  sym_add_unresolved()
>  sym_add_exported()
>  sym_add_crc()

Ok.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

[PATCH 10/27] modpost: traverse unresolved symbols in order

[Masahiro Yamada]

Currently, modpost manages unresolved in a singly liked list; it adds
a new node to the head, and traverses the list from new to old.

Use a doubly linked list to keep the order in the symbol table in the
ELF file.

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

 scripts/mod/modpost.c | 20 ++++++++++++++------
 scripts/mod/modpost.h |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 1c7d2831e89d..e1eb188d6282 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -185,6 +185,8 @@ static struct module *new_module(const char *modname)
 	mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
 	memset(mod, 0, sizeof(*mod));
 
+	INIT_LIST_HEAD(&mod->unresolved_symbols);
+
 	strcpy(mod->name, modname);
 	mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
 	mod->gpl_compatible = true;
@@ -201,6 +203,7 @@ static struct module *new_module(const char *modname)
 
 struct symbol {
 	struct symbol *next;
+	struct list_head list;
 	struct module *module;
 	unsigned int crc;
 	bool crc_valid;
@@ -255,8 +258,12 @@ static struct symbol *new_symbol(const char *name, struct module *module,
 
 static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
 {
-	mod->unres = alloc_symbol(name, mod->unres);
-	mod->unres->weak = weak;
+	struct symbol *sym;
+
+	sym = alloc_symbol(name, NULL);
+	sym->weak = weak;
+
+	list_add_tail(&sym->list, &mod->unresolved_symbols);
 }
 
 static struct symbol *find_symbol(const char *name)
@@ -2147,7 +2154,7 @@ static void check_exports(struct module *mod)
 {
 	struct symbol *s, *exp;
 
-	for (s = mod->unres; s; s = s->next) {
+	list_for_each_entry(s, &mod->unresolved_symbols, list) {
 		const char *basename;
 		exp = find_symbol(s->name);
 		if (!exp) {
@@ -2268,7 +2275,7 @@ static void add_versions(struct buffer *b, struct module *mod)
 	buf_printf(b, "static const struct modversion_info ____versions[]\n");
 	buf_printf(b, "__used __section(\"__versions\") = {\n");
 
-	for (s = mod->unres; s; s = s->next) {
+	list_for_each_entry(s, &mod->unresolved_symbols, list) {
 		if (!s->module)
 			continue;
 		if (!s->crc_valid) {
@@ -2294,13 +2301,14 @@ static void add_depends(struct buffer *b, struct module *mod)
 	int first = 1;
 
 	/* Clear ->seen flag of modules that own symbols needed by this. */
-	for (s = mod->unres; s; s = s->next)
+	list_for_each_entry(s, &mod->unresolved_symbols, list) {
 		if (s->module)
 			s->module->seen = s->module->is_vmlinux;
+	}
 
 	buf_printf(b, "\n");
 	buf_printf(b, "MODULE_INFO(depends, \"");
-	for (s = mod->unres; s; s = s->next) {
+	list_for_each_entry(s, &mod->unresolved_symbols, list) {
 		const char *p;
 		if (!s->module)
 			continue;
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index c3b5d2f0e2bb..6a90bfc08458 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -117,7 +117,7 @@ struct namespace_list {
 struct module {
 	struct list_head list;
 	int gpl_compatible;
-	struct symbol *unres;
+	struct list_head unresolved_symbols;
 	bool from_dump;		/* true if module was loaded from *.symvers */
 	bool is_vmlinux;
 	bool seen;
-- 
2.32.0

Re: [PATCH 10/27] modpost: traverse unresolved symbols in order

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Currently, modpost manages unresolved in a singly liked list; it adds

s/liked/linked/

> a new node to the head, and traverses the list from new to old.
>
> Use a doubly linked list to keep the order in the symbol table in the
> ELF file.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 20 ++++++++++++++------
>  scripts/mod/modpost.h |  2 +-
>  2 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 1c7d2831e89d..e1eb188d6282 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -185,6 +185,8 @@ static struct module *new_module(const char *modname)
>         mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
>         memset(mod, 0, sizeof(*mod));
>
> +       INIT_LIST_HEAD(&mod->unresolved_symbols);
> +
>         strcpy(mod->name, modname);
>         mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
>         mod->gpl_compatible = true;
> @@ -201,6 +203,7 @@ static struct module *new_module(const char *modname)
>
>  struct symbol {
>         struct symbol *next;
> +       struct list_head list;

Isn't `list` meant to replace `next`?

>         struct module *module;
>         unsigned int crc;
>         bool crc_valid;
> @@ -255,8 +258,12 @@ static struct symbol *new_symbol(const char *name, struct module *module,
>
>  static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
>  {
> -       mod->unres = alloc_symbol(name, mod->unres);
> -       mod->unres->weak = weak;
> +       struct symbol *sym;
> +
> +       sym = alloc_symbol(name, NULL);
> +       sym->weak = weak;
> +
> +       list_add_tail(&sym->list, &mod->unresolved_symbols);

Because I was curious here why NULL was passed, rather than remove the
assignment to struct symbol's next member in alloc_symbol.

I get why you replace the `unres` member of struct module. I guess I'm
curious then why yet another list is added to struct symbol, rather
than replace the next member.

Also, does adding a struct list_head member really not specify the
_type_ of the next element?  I guess when I look at the definition of
struct module, at the member unresolved symbols, I don't know whether
it's a list of struct module* or a list of struct symbol*.

<snip>

> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index c3b5d2f0e2bb..6a90bfc08458 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -117,7 +117,7 @@ struct namespace_list {
>  struct module {
>         struct list_head list;
>         int gpl_compatible;
> -       struct symbol *unres;
> +       struct list_head unresolved_symbols;
>         bool from_dump;         /* true if module was loaded from *.symvers */
>         bool is_vmlinux;
>         bool seen;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 10/27] modpost: traverse unresolved symbols in order

[Masahiro Yamada]

On Wed, Apr 27, 2022 at 2:08 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Currently, modpost manages unresolved in a singly liked list; it adds
>
> s/liked/linked/
>
> > a new node to the head, and traverses the list from new to old.
> >
> > Use a doubly linked list to keep the order in the symbol table in the
> > ELF file.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 20 ++++++++++++++------
> >  scripts/mod/modpost.h |  2 +-
> >  2 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 1c7d2831e89d..e1eb188d6282 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -185,6 +185,8 @@ static struct module *new_module(const char *modname)
> >         mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
> >         memset(mod, 0, sizeof(*mod));
> >
> > +       INIT_LIST_HEAD(&mod->unresolved_symbols);
> > +
> >         strcpy(mod->name, modname);
> >         mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
> >         mod->gpl_compatible = true;
> > @@ -201,6 +203,7 @@ static struct module *new_module(const char *modname)
> >
> >  struct symbol {
> >         struct symbol *next;
> > +       struct list_head list;
>
> Isn't `list` meant to replace `next`?


No.  'next' is still used for the hash table.

modpost used it in various ways.
You need to read the code closely if you are interested in
how modpost works.

>
> >         struct module *module;
> >         unsigned int crc;
> >         bool crc_valid;
> > @@ -255,8 +258,12 @@ static struct symbol *new_symbol(const char *name, struct module *module,
> >
> >  static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
> >  {
> > -       mod->unres = alloc_symbol(name, mod->unres);
> > -       mod->unres->weak = weak;
> > +       struct symbol *sym;
> > +
> > +       sym = alloc_symbol(name, NULL);
> > +       sym->weak = weak;
> > +
> > +       list_add_tail(&sym->list, &mod->unresolved_symbols);
>
> Because I was curious here why NULL was passed, rather than remove the
> assignment to struct symbol's next member in alloc_symbol.
> I get why you replace the `unres` member of struct module. I guess I'm
> curious then why yet another list is added to struct symbol, rather
> than replace the next member.


'next' is used for ad-hoc hash table implementation.
I will be cleaned up by a later commit.

See this patch:
https://patchwork.kernel.org/project/linux-kbuild/patch/20220424190811.1678416-20-masahiroy@kernel.org/



>
> Also, does adding a struct list_head member really not specify the
> _type_ of the next element?  I guess when I look at the definition of
> struct module, at the member unresolved symbols, I don't know whether
> it's a list of struct module* or a list of struct symbol*.


This is how include/linux/list.h works.
struct list_head is really generic, does not specify a specific type definition.



>
> <snip>
>
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index c3b5d2f0e2bb..6a90bfc08458 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -117,7 +117,7 @@ struct namespace_list {
> >  struct module {
> >         struct list_head list;
> >         int gpl_compatible;
> > -       struct symbol *unres;
> > +       struct list_head unresolved_symbols;
> >         bool from_dump;         /* true if module was loaded from *.symvers */
> >         bool is_vmlinux;
> >         bool seen;
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH 11/27] modpost: use doubly linked list for dump_lists

[Masahiro Yamada]

This looks easier to understand (just because this is a pattern in
the kernel code). No functional change is intended.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index e1eb188d6282..4c074d6c1721 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2496,7 +2496,7 @@ static void write_namespace_deps_files(const char *fname)
 }
 
 struct dump_list {
-	struct dump_list *next;
+	struct list_head list;
 	const char *file;
 };
 
@@ -2508,8 +2508,8 @@ int main(int argc, char **argv)
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int n;
-	struct dump_list *dump_read_start = NULL;
-	struct dump_list **dump_read_iter = &dump_read_start;
+	LIST_HEAD(dump_lists);
+	struct dump_list *dl, *dl2;
 
 	while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
 		switch (opt) {
@@ -2517,10 +2517,9 @@ int main(int argc, char **argv)
 			external_module = true;
 			break;
 		case 'i':
-			*dump_read_iter =
-				NOFAIL(calloc(1, sizeof(**dump_read_iter)));
-			(*dump_read_iter)->file = optarg;
-			dump_read_iter = &(*dump_read_iter)->next;
+			dl = NOFAIL(malloc(sizeof(*dl)));
+			dl->file = optarg;
+			list_add_tail(&dl->list, &dump_lists);
 			break;
 		case 'm':
 			modversions = true;
@@ -2554,13 +2553,10 @@ int main(int argc, char **argv)
 		}
 	}
 
-	while (dump_read_start) {
-		struct dump_list *tmp;
-
-		read_dump(dump_read_start->file);
-		tmp = dump_read_start->next;
-		free(dump_read_start);
-		dump_read_start = tmp;
+	list_for_each_entry_safe(dl, dl2, &dump_lists, list) {
+		read_dump(dl->file);
+		list_del(&dl->list);
+		free(dl);
 	}
 
 	while (optind < argc)
-- 
2.32.0

Re: [PATCH 11/27] modpost: use doubly linked list for dump_lists

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This looks easier to understand (just because this is a pattern in
> the kernel code). No functional change is intended.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index e1eb188d6282..4c074d6c1721 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2496,7 +2496,7 @@ static void write_namespace_deps_files(const char *fname)
>  }
>
>  struct dump_list {
> -       struct dump_list *next;
> +       struct list_head list;
>         const char *file;
>  };
>
> @@ -2508,8 +2508,8 @@ int main(int argc, char **argv)
>         char *dump_write = NULL, *files_source = NULL;
>         int opt;
>         int n;
> -       struct dump_list *dump_read_start = NULL;
> -       struct dump_list **dump_read_iter = &dump_read_start;
> +       LIST_HEAD(dump_lists);
> +       struct dump_list *dl, *dl2;
>
>         while ((opt = getopt(argc, argv, "ei:mnT:o:awENd:")) != -1) {
>                 switch (opt) {
> @@ -2517,10 +2517,9 @@ int main(int argc, char **argv)
>                         external_module = true;
>                         break;
>                 case 'i':
> -                       *dump_read_iter =
> -                               NOFAIL(calloc(1, sizeof(**dump_read_iter)));
> -                       (*dump_read_iter)->file = optarg;
> -                       dump_read_iter = &(*dump_read_iter)->next;
> +                       dl = NOFAIL(malloc(sizeof(*dl)));
> +                       dl->file = optarg;
> +                       list_add_tail(&dl->list, &dump_lists);

Consider NULL'ing out dl since you reuse its storage below.  Either way
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>                         break;
>                 case 'm':
>                         modversions = true;
> @@ -2554,13 +2553,10 @@ int main(int argc, char **argv)
>                 }
>         }
>
> -       while (dump_read_start) {
> -               struct dump_list *tmp;
> -
> -               read_dump(dump_read_start->file);
> -               tmp = dump_read_start->next;
> -               free(dump_read_start);
> -               dump_read_start = tmp;
> +       list_for_each_entry_safe(dl, dl2, &dump_lists, list) {
> +               read_dump(dl->file);
> +               list_del(&dl->list);
> +               free(dl);
>         }
>
>         while (optind < argc)
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 12/27] modpost: move struct namespace_list to modpost.c

[Masahiro Yamada]

There is no good reason to define struct namespace_list in modpost.h

struct module has pointers to struct namespace_list, but that does
not require the definition of struct namespace_list.

Move it to modpost.c.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 4c074d6c1721..6f2748340746 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -281,6 +281,11 @@ static struct symbol *find_symbol(const char *name)
 	return NULL;
 }
 
+struct namespace_list {
+	struct namespace_list *next;
+	char namespace[];
+};
+
 static bool contains_namespace(struct namespace_list *list,
 			       const char *namespace)
 {
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 6a90bfc08458..2dbafbda9b0f 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -109,11 +109,6 @@ buf_printf(struct buffer *buf, const char *fmt, ...);
 void
 buf_write(struct buffer *buf, const char *s, int len);
 
-struct namespace_list {
-	struct namespace_list *next;
-	char namespace[];
-};
-
 struct module {
 	struct list_head list;
 	int gpl_compatible;
-- 
2.32.0

Re: [PATCH 12/27] modpost: move struct namespace_list to modpost.c

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> There is no good reason to define struct namespace_list in modpost.h
>
> struct module has pointers to struct namespace_list, but that does
> not require the definition of struct namespace_list.
>
> Move it to modpost.c.

Looks like modpost.h is included in:
- scripts/mod/sumversion.c
- scripts/mod/file2alias.c
- scripts/mod/modpost.c

But indeed, only modpost.c uses struct namespace_list.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 5 +++++
>  scripts/mod/modpost.h | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 4c074d6c1721..6f2748340746 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -281,6 +281,11 @@ static struct symbol *find_symbol(const char *name)
>         return NULL;
>  }
>
> +struct namespace_list {
> +       struct namespace_list *next;
> +       char namespace[];
> +};
> +
>  static bool contains_namespace(struct namespace_list *list,
>                                const char *namespace)
>  {
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 6a90bfc08458..2dbafbda9b0f 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -109,11 +109,6 @@ buf_printf(struct buffer *buf, const char *fmt, ...);
>  void
>  buf_write(struct buffer *buf, const char *s, int len);
>
> -struct namespace_list {
> -       struct namespace_list *next;
> -       char namespace[];
> -};
> -
>  struct module {
>         struct list_head list;
>         int gpl_compatible;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 13/27] modpost: traverse the namespace_list in order

[Masahiro Yamada]

Use the doubly linked list to traverse the list in the added order.
This makes the code more consistent.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 6f2748340746..e23e416213bf 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -186,6 +186,8 @@ static struct module *new_module(const char *modname)
 	memset(mod, 0, sizeof(*mod));
 
 	INIT_LIST_HEAD(&mod->unresolved_symbols);
+	INIT_LIST_HEAD(&mod->missing_namespaces);
+	INIT_LIST_HEAD(&mod->imported_namespaces);
 
 	strcpy(mod->name, modname);
 	mod->is_vmlinux = (strcmp(modname, "vmlinux") == 0);
@@ -282,39 +284,34 @@ static struct symbol *find_symbol(const char *name)
 }
 
 struct namespace_list {
-	struct namespace_list *next;
+	struct list_head list;
 	char namespace[];
 };
 
-static bool contains_namespace(struct namespace_list *list,
-			       const char *namespace)
+static bool contains_namespace(struct list_head *head, const char *namespace)
 {
-	for (; list; list = list->next)
+	struct namespace_list *list;
+
+	list_for_each_entry(list, head, list) {
 		if (!strcmp(list->namespace, namespace))
 			return true;
+	}
 
 	return false;
 }
 
-static void add_namespace(struct namespace_list **list, const char *namespace)
+static void add_namespace(struct list_head *head, const char *namespace)
 {
 	struct namespace_list *ns_entry;
 
-	if (!contains_namespace(*list, namespace)) {
-		ns_entry = NOFAIL(malloc(sizeof(struct namespace_list) +
+	if (!contains_namespace(head, namespace)) {
+		ns_entry = NOFAIL(malloc(sizeof(*ns_entry) +
 					 strlen(namespace) + 1));
 		strcpy(ns_entry->namespace, namespace);
-		ns_entry->next = *list;
-		*list = ns_entry;
+		list_add_tail(&ns_entry->list, head);
 	}
 }
 
-static bool module_imports_namespace(struct module *module,
-				     const char *namespace)
-{
-	return contains_namespace(module->imported_namespaces, namespace);
-}
-
 static const struct {
 	const char *str;
 	enum export export;
@@ -2186,7 +2183,7 @@ static void check_exports(struct module *mod)
 			basename = mod->name;
 
 		if (exp->namespace &&
-		    !module_imports_namespace(mod, exp->namespace)) {
+		    !contains_namespace(&mod->imported_namespaces, exp->namespace)) {
 			modpost_log(allow_missing_ns_imports ? LOG_WARN : LOG_ERROR,
 				    "module %s uses symbol %s from namespace %s, but does not import it.\n",
 				    basename, exp->name, exp->namespace);
@@ -2485,12 +2482,12 @@ static void write_namespace_deps_files(const char *fname)
 
 	list_for_each_entry(mod, &modules, list) {
 
-		if (mod->from_dump || !mod->missing_namespaces)
+		if (mod->from_dump || list_empty(&mod->missing_namespaces))
 			continue;
 
 		buf_printf(&ns_deps_buf, "%s.ko:", mod->name);
 
-		for (ns = mod->missing_namespaces; ns; ns = ns->next)
+		list_for_each_entry(ns, &mod->missing_namespaces, list)
 			buf_printf(&ns_deps_buf, " %s", ns->namespace);
 
 		buf_printf(&ns_deps_buf, "\n");
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 2dbafbda9b0f..4cb955dda83f 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -120,9 +120,9 @@ struct module {
 	struct buffer dev_table_buf;
 	char	     srcversion[25];
 	// Missing namespace dependencies
-	struct namespace_list *missing_namespaces;
+	struct list_head missing_namespaces;
 	// Actual imported namespaces
-	struct namespace_list *imported_namespaces;
+	struct list_head imported_namespaces;
 	char name[];
 };
 
-- 
2.32.0

Re: [PATCH 13/27] modpost: traverse the namespace_list in order

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Use the doubly linked list to traverse the list in the added order.
> This makes the code more consistent.

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

-- 
Thanks,
~Nick Desaulniers

[PATCH 14/27] modpost: dump Module.symvers in the same order of modules.order

[Masahiro Yamada]

modpost dumps the exported symbols into Module.symvers, but currently
in random order because it iterates in the hash table.

Add a linked list of exported symbols in struct module, so we can
iterate on symbols per module.

This commit makes Module.symvers much more readable; the outer loop in
write_dump() iterates over the modules in the order of modules.order,
and the inner loop dumps symbols in each module.

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

 scripts/mod/modpost.c | 27 ++++++++++++---------------
 scripts/mod/modpost.h |  1 +
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index e23e416213bf..1793396e1323 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -185,6 +185,7 @@ static struct module *new_module(const char *modname)
 	mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
 	memset(mod, 0, sizeof(*mod));
 
+	INIT_LIST_HEAD(&mod->exported_symbols);
 	INIT_LIST_HEAD(&mod->unresolved_symbols);
 	INIT_LIST_HEAD(&mod->missing_namespaces);
 	INIT_LIST_HEAD(&mod->imported_namespaces);
@@ -407,6 +408,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 
 	if (!s) {
 		s = new_symbol(name, mod, export);
+		list_add_tail(&s->list, &mod->exported_symbols);
 	} else if (!external_module || s->module->is_vmlinux ||
 		   s->module == mod) {
 		warn("%s: '%s' exported twice. Previous export was in %s%s\n",
@@ -2452,22 +2454,17 @@ static void read_dump(const char *fname)
 static void write_dump(const char *fname)
 {
 	struct buffer buf = { };
-	struct symbol *symbol;
-	const char *namespace;
-	int n;
+	struct module *mod;
+	struct symbol *sym;
 
-	for (n = 0; n < SYMBOL_HASH_SIZE ; n++) {
-		symbol = symbolhash[n];
-		while (symbol) {
-			if (!symbol->module->from_dump) {
-				namespace = symbol->namespace;
-				buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
-					   symbol->crc, symbol->name,
-					   symbol->module->name,
-					   export_str(symbol->export),
-					   namespace ? namespace : "");
-			}
-			symbol = symbol->next;
+	list_for_each_entry(mod, &modules, list) {
+		if (mod->from_dump)
+			continue;
+		list_for_each_entry(sym, &mod->exported_symbols, list) {
+			buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
+				   sym->crc, sym->name, mod->name,
+				   export_str(sym->export),
+				   sym->namespace ?: "");
 		}
 	}
 	write_buf(&buf, fname);
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 4cb955dda83f..5922b0c39bb7 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -112,6 +112,7 @@ buf_write(struct buffer *buf, const char *s, int len);
 struct module {
 	struct list_head list;
 	int gpl_compatible;
+	struct list_head exported_symbols;
 	struct list_head unresolved_symbols;
 	bool from_dump;		/* true if module was loaded from *.symvers */
 	bool is_vmlinux;
-- 
2.32.0

Re: [PATCH 14/27] modpost: dump Module.symvers in the same order of modules.order

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> modpost dumps the exported symbols into Module.symvers, but currently
> in random order because it iterates in the hash table.
>
> Add a linked list of exported symbols in struct module, so we can
> iterate on symbols per module.
>
> This commit makes Module.symvers much more readable; the outer loop in
> write_dump() iterates over the modules in the order of modules.order,
> and the inner loop dumps symbols in each module.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
>
>  scripts/mod/modpost.c | 27 ++++++++++++---------------
>  scripts/mod/modpost.h |  1 +
>  2 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index e23e416213bf..1793396e1323 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -185,6 +185,7 @@ static struct module *new_module(const char *modname)
>         mod = NOFAIL(malloc(sizeof(*mod) + strlen(modname) + 1));
>         memset(mod, 0, sizeof(*mod));
>
> +       INIT_LIST_HEAD(&mod->exported_symbols);
>         INIT_LIST_HEAD(&mod->unresolved_symbols);
>         INIT_LIST_HEAD(&mod->missing_namespaces);
>         INIT_LIST_HEAD(&mod->imported_namespaces);
> @@ -407,6 +408,7 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
>
>         if (!s) {
>                 s = new_symbol(name, mod, export);
> +               list_add_tail(&s->list, &mod->exported_symbols);
>         } else if (!external_module || s->module->is_vmlinux ||
>                    s->module == mod) {
>                 warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> @@ -2452,22 +2454,17 @@ static void read_dump(const char *fname)
>  static void write_dump(const char *fname)
>  {
>         struct buffer buf = { };
> -       struct symbol *symbol;
> -       const char *namespace;
> -       int n;
> +       struct module *mod;
> +       struct symbol *sym;
>
> -       for (n = 0; n < SYMBOL_HASH_SIZE ; n++) {
> -               symbol = symbolhash[n];
> -               while (symbol) {
> -                       if (!symbol->module->from_dump) {
> -                               namespace = symbol->namespace;
> -                               buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
> -                                          symbol->crc, symbol->name,
> -                                          symbol->module->name,
> -                                          export_str(symbol->export),
> -                                          namespace ? namespace : "");
> -                       }
> -                       symbol = symbol->next;
> +       list_for_each_entry(mod, &modules, list) {
> +               if (mod->from_dump)
> +                       continue;
> +               list_for_each_entry(sym, &mod->exported_symbols, list) {
> +                       buf_printf(&buf, "0x%08x\t%s\t%s\t%s\t%s\n",
> +                                  sym->crc, sym->name, mod->name,
> +                                  export_str(sym->export),
> +                                  sym->namespace ?: "");
>                 }
>         }
>         write_buf(&buf, fname);
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 4cb955dda83f..5922b0c39bb7 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -112,6 +112,7 @@ buf_write(struct buffer *buf, const char *s, int len);
>  struct module {
>         struct list_head list;
>         int gpl_compatible;
> +       struct list_head exported_symbols;
>         struct list_head unresolved_symbols;
>         bool from_dump;         /* true if module was loaded from *.symvers */
>         bool is_vmlinux;
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

[PATCH 15/27] modpost: move static EXPORT_SYMBOL check to check_exports()

[Masahiro Yamada]

Now that struct module has a list of symbols it exports, this check
can go into check_exports(). The code becomes shorter.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 1793396e1323..14044cd94aaa 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2195,6 +2195,12 @@ static void check_exports(struct module *mod)
 		if (!mod->gpl_compatible)
 			check_for_gpl_usage(exp->export, basename, exp->name);
 	}
+
+	list_for_each_entry(s, &mod->exported_symbols, list) {
+		if (s->is_static)
+			error("\"%s\" [%s] is a static %s\n",
+			      s->name, mod->name, export_str(s->export));
+	}
 }
 
 static void check_modname_len(struct module *mod)
@@ -2506,7 +2512,6 @@ int main(int argc, char **argv)
 	char *missing_namespace_deps = NULL;
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
-	int n;
 	LIST_HEAD(dump_lists);
 	struct dump_list *dl, *dl2;
 
@@ -2602,16 +2607,6 @@ int main(int argc, char **argv)
 	if (sec_mismatch_count && !sec_mismatch_warn_only)
 		error("Section mismatches detected.\n"
 		      "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
-	for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
-		struct symbol *s;
-
-		for (s = symbolhash[n]; s; s = s->next) {
-			if (s->is_static)
-				error("\"%s\" [%s] is a static %s\n",
-				      s->name, s->module->name,
-				      export_str(s->export));
-		}
-	}
 
 	if (nr_unresolved > MAX_UNRESOLVED_REPORTS)
 		warn("suppressed %u unresolved symbol warnings because there were too many)\n",
-- 
2.32.0

[PATCH 16/27] modpost: make multiple export error

[Masahiro Yamada]

This is currently a warning, but I think modpost should stop building
in this case.

If the same symbol is exported multiple times and we let it keep going,
the sanity check becomes difficult.

Only the legitimate case is that an external module overrides the
corresponding in-tree module to provide a different implementation
with the same interface.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 14044cd94aaa..73f0b98e3b5a 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -411,9 +411,9 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 		list_add_tail(&s->list, &mod->exported_symbols);
 	} else if (!external_module || s->module->is_vmlinux ||
 		   s->module == mod) {
-		warn("%s: '%s' exported twice. Previous export was in %s%s\n",
-		     mod->name, name, s->module->name,
-		     s->module->is_vmlinux ? "" : ".ko");
+		error("%s: '%s' exported twice. Previous export was in %s%s\n",
+		      mod->name, name, s->module->name,
+		      s->module->is_vmlinux ? "" : ".ko");
 		return s;
 	}
 
-- 
2.32.0

Re: [PATCH 16/27] modpost: make multiple export error

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> This is currently a warning, but I think modpost should stop building
> in this case.
>
> If the same symbol is exported multiple times and we let it keep going,
> the sanity check becomes difficult.
>
> Only the legitimate case is that an external module overrides the
> corresponding in-tree module to provide a different implementation
> with the same interface.

Could the same module export a weak version of a symbol, and a strong one?

Can kernel modules override in-kernel strong symbols?

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 14044cd94aaa..73f0b98e3b5a 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -411,9 +411,9 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
>                 list_add_tail(&s->list, &mod->exported_symbols);
>         } else if (!external_module || s->module->is_vmlinux ||
>                    s->module == mod) {
> -               warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> -                    mod->name, name, s->module->name,
> -                    s->module->is_vmlinux ? "" : ".ko");
> +               error("%s: '%s' exported twice. Previous export was in %s%s\n",
> +                     mod->name, name, s->module->name,
> +                     s->module->is_vmlinux ? "" : ".ko");
>                 return s;
>         }
>
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 16/27] modpost: make multiple export error

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:48 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > This is currently a warning, but I think modpost should stop building
> > in this case.
> >
> > If the same symbol is exported multiple times and we let it keep going,
> > the sanity check becomes difficult.
> >
> > Only the legitimate case is that an external module overrides the
> > corresponding in-tree module to provide a different implementation
> > with the same interface.
>
> Could the same module export a weak version of a symbol, and a strong one?

No.  There is no concept like   EXPORT_SYMBOL_WEAK.

I am talking about kmod things.
You can modprobe an external module instead of the in-kernel one.

>
> Can kernel modules override in-kernel strong symbols?

Yes, I think so.


>
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 14044cd94aaa..73f0b98e3b5a 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -411,9 +411,9 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
> >                 list_add_tail(&s->list, &mod->exported_symbols);
> >         } else if (!external_module || s->module->is_vmlinux ||
> >                    s->module == mod) {
> > -               warn("%s: '%s' exported twice. Previous export was in %s%s\n",
> > -                    mod->name, name, s->module->name,
> > -                    s->module->is_vmlinux ? "" : ".ko");
> > +               error("%s: '%s' exported twice. Previous export was in %s%s\n",
> > +                     mod->name, name, s->module->name,
> > +                     s->module->is_vmlinux ? "" : ".ko");
> >                 return s;
> >         }
> >
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 16/27] modpost: make multiple export error

[Nick Desaulniers]

On Mon, Apr 25, 2022 at 9:10 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Apr 26, 2022 at 3:48 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > This is currently a warning, but I think modpost should stop building
> > > in this case.
> > >
> > > If the same symbol is exported multiple times and we let it keep going,
> > > the sanity check becomes difficult.
> > >
> > > Only the legitimate case is that an external module overrides the
> > > corresponding in-tree module to provide a different implementation
> > > with the same interface.
> >
> > Could the same module export a weak version of a symbol, and a strong one?
>
> No.  There is no concept like   EXPORT_SYMBOL_WEAK.
>
> I am talking about kmod things.
> You can modprobe an external module instead of the in-kernel one.

Ok, this patch seems fine to me.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 16/27] modpost: make multiple export error

[Masahiro Yamada]

On Wed, Apr 27, 2022 at 1:40 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Apr 25, 2022 at 9:10 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Tue, Apr 26, 2022 at 3:48 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > This is currently a warning, but I think modpost should stop building
> > > > in this case.
> > > >
> > > > If the same symbol is exported multiple times and we let it keep going,
> > > > the sanity check becomes difficult.
> > > >
> > > > Only the legitimate case is that an external module overrides the
> > > > corresponding in-tree module to provide a different implementation
> > > > with the same interface.
> > >
> > > Could the same module export a weak version of a symbol, and a strong one?
> >
> > No.  There is no concept like   EXPORT_SYMBOL_WEAK.
> >
> > I am talking about kmod things.
> > You can modprobe an external module instead of the in-kernel one.
>
> Ok, this patch seems fine to me.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> --
> Thanks,
> ~Nick Desaulniers


Nick,


If useful, I can add more commits to the commit description.


I know one example in the tree that exploits this feature.

$ make allmodconfig all
   You will get drivers/nvdimm/libnvdimm.ko, then

$ make M=tools/testing/nvdimm
   You will get tools/testing/nvdimm/libnvdimm.ko

The latter is a mocked one that exported the same symbols
as drivers/nvdimm/libnvdimm.ko









-- 
Best Regards
Masahiro Yamada

[PATCH 17/27] modpost: make sym_add_exported() always allocate a new symbol

[Masahiro Yamada]

Currently, sym_add_exported() does not allocate a symbol if the same
name symbol already exists in the hash table.

This does not reflect the real usecases. You can have an external
module override the in-tree one. In this case, the external module
will export the same name symbols as the in-tree one. However,
modpost simply ignores those symbols, then Module.symvers for the
external module miss to list its symbols.

sym_add_exported() should allocate a new symbol.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 73f0b98e3b5a..4f0b2b23516a 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -406,19 +406,16 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 {
 	struct symbol *s = find_symbol(name);
 
-	if (!s) {
-		s = new_symbol(name, mod, export);
-		list_add_tail(&s->list, &mod->exported_symbols);
-	} else if (!external_module || s->module->is_vmlinux ||
-		   s->module == mod) {
+	if (s && (!external_module || s->module->is_vmlinux || s->module == mod)) {
 		error("%s: '%s' exported twice. Previous export was in %s%s\n",
 		      mod->name, name, s->module->name,
 		      s->module->is_vmlinux ? "" : ".ko");
-		return s;
 	}
 
+	s = new_symbol(name, mod, export);
 	s->module = mod;
 	s->export    = export;
+	list_add_tail(&s->list, &mod->exported_symbols);
 	return s;
 }
 
-- 
2.32.0

[PATCH 18/27] modpost: make sym_add_exported() a void function

[Masahiro Yamada]

Currently, sym_add_exported() returns the pointer to the added symbol
just for doing:

    s->is_static = false;

in the read_dump() function.

I noticed it was unneeded because sym_add_exported() can know the proper
default for the 'is_static' member by checking mod->from_dump.

This makes the code slightly simpler.

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 4f0b2b23516a..fd710aa59bda 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -243,7 +243,7 @@ static struct symbol *alloc_symbol(const char *name, struct symbol *next)
 	memset(s, 0, sizeof(*s));
 	strcpy(s->name, name);
 	s->next = next;
-	s->is_static = true;
+
 	return s;
 }
 
@@ -401,8 +401,8 @@ static void sym_update_namespace(const char *symname, const char *namespace)
 	s->namespace = namespace[0] ? NOFAIL(strdup(namespace)) : NULL;
 }
 
-static struct symbol *sym_add_exported(const char *name, struct module *mod,
-				       enum export export)
+static void sym_add_exported(const char *name, struct module *mod,
+			     enum export export)
 {
 	struct symbol *s = find_symbol(name);
 
@@ -414,9 +414,9 @@ static struct symbol *sym_add_exported(const char *name, struct module *mod,
 
 	s = new_symbol(name, mod, export);
 	s->module = mod;
+	s->is_static = !mod->from_dump;
 	s->export    = export;
 	list_add_tail(&s->list, &mod->exported_symbols);
-	return s;
 }
 
 static void sym_set_crc(const char *name, unsigned int crc)
@@ -2419,7 +2419,6 @@ static void read_dump(const char *fname)
 		char *symname, *namespace, *modname, *d, *export;
 		unsigned int crc;
 		struct module *mod;
-		struct symbol *s;
 
 		if (!(symname = strchr(line, '\t')))
 			goto fail;
@@ -2442,8 +2441,7 @@ static void read_dump(const char *fname)
 			mod = new_module(modname);
 			mod->from_dump = true;
 		}
-		s = sym_add_exported(symname, mod, export_no(export));
-		s->is_static = false;
+		sym_add_exported(symname, mod, export_no(export));
 		sym_set_crc(symname, crc);
 		sym_update_namespace(symname, namespace);
 	}
-- 
2.32.0

[PATCH 19/27] modpost: use hlist for hash table implementation

[Masahiro Yamada]

modpost has a simple hash table, which only supports the entry addition
(new_symbol), and search (find_symbol).

I want to have a macro to delete an entry from the hash table, and also
support multiple hash tables.

Instead of extending the own implementation, let's reuse hlist in list.h.
The code originates in included/linux/list.h, and looks familiar for
kernel developers.

I added 4 macros on top of the hlist utility macros:

 - hash_add_symbol

     add a symbol to the given hash table

 - hash_del_symbol

     delete a symbol from the given hash table

 - hash_for_matched_symbol

     traverse the hash table, stopping by the symbol whose name matches

 - hash_for_matched_symbol_safe

     like hash_for_each_symbol, but safe against node removal

While I was here, I increased the hash table size from 1024 to 8192
to decrease the hash collision. We have more and more exported symbols
these days.

I moved ARRAY_SIZE() from file2alias.c to modpost.h because it is needed
in modpost.c as well.

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

 scripts/mod/file2alias.c |  2 --
 scripts/mod/modpost.c    | 54 +++++++++++++++++++++-------------------
 scripts/mod/modpost.h    |  2 ++
 3 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 5258247d78ac..e8a9c6816fec 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -734,8 +734,6 @@ static int do_vio_entry(const char *filename, void *symval,
 	return 1;
 }
 
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
-
 static void do_input(char *alias,
 		     kernel_ulong_t *arr, unsigned int min, unsigned int max)
 {
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index fd710aa59bda..908390b4fa80 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -199,13 +199,8 @@ static struct module *new_module(const char *modname)
 	return mod;
 }
 
-/* A hash of all exported symbols,
- * struct symbol is also used for lists of unresolved symbols */
-
-#define SYMBOL_HASH_SIZE 1024
-
 struct symbol {
-	struct symbol *next;
+	struct hlist_node hash_node;
 	struct list_head list;
 	struct module *module;
 	unsigned int crc;
@@ -217,8 +212,6 @@ struct symbol {
 	char name[];
 };
 
-static struct symbol *symbolhash[SYMBOL_HASH_SIZE];
-
 /* This is based on the hash algorithm from gdbm, via tdb */
 static inline unsigned int tdb_hash(const char *name)
 {
@@ -232,38 +225,47 @@ static inline unsigned int tdb_hash(const char *name)
 	return (1103515243 * value + 12345);
 }
 
+/* useful hash macros */
+#define hash_head(table, key)		(&(table)[tdb_hash(key) % ARRAY_SIZE(table)])
+
+#define hash_add_symbol(sym, table)	hlist_add_head(&(sym)->hash_node, \
+						       hash_head(table, (sym)->name))
+
+#define hash_del_symbol(sym)		hlist_del_init(&(sym)->hash_node)
+
+#define hash_for_matched_symbol(sym, table, key) \
+	hlist_for_each_entry(sym, hash_head(table, key), hash_node) \
+		if (!strcmp(sym->name, key))
+
+#define hash_for_matched_symbol_safe(sym, n, table, key) \
+	hlist_for_each_entry_safe(sym, n, hash_head(table, key), hash_node) \
+		if (!strcmp(sym->name, key))
+
+#define HASHTABLE_DECLARE(name, size)	struct hlist_head name[size];
+
+/* hash table of all exported symbols */
+HASHTABLE_DECLARE(exported_symbols, 8192);
+
 /**
  * Allocate a new symbols for use in the hash of exported symbols or
  * the list of unresolved symbols per module
  **/
-static struct symbol *alloc_symbol(const char *name, struct symbol *next)
+static struct symbol *alloc_symbol(const char *name)
 {
 	struct symbol *s = NOFAIL(malloc(sizeof(*s) + strlen(name) + 1));
 
 	memset(s, 0, sizeof(*s));
 	strcpy(s->name, name);
-	s->next = next;
 
 	return s;
 }
 
-/* For the hash of exported symbols */
-static struct symbol *new_symbol(const char *name, struct module *module,
-				 enum export export)
-{
-	unsigned int hash;
-
-	hash = tdb_hash(name) % SYMBOL_HASH_SIZE;
-	symbolhash[hash] = alloc_symbol(name, symbolhash[hash]);
-
-	return symbolhash[hash];
-}
 
 static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
 {
 	struct symbol *sym;
 
-	sym = alloc_symbol(name, NULL);
+	sym = alloc_symbol(name);
 	sym->weak = weak;
 
 	list_add_tail(&sym->list, &mod->unresolved_symbols);
@@ -277,9 +279,8 @@ static struct symbol *find_symbol(const char *name)
 	if (name[0] == '.')
 		name++;
 
-	for (s = symbolhash[tdb_hash(name) % SYMBOL_HASH_SIZE]; s; s = s->next) {
-		if (strcmp(s->name, name) == 0)
-			return s;
+	hash_for_matched_symbol(s, exported_symbols, name) {
+		return s;
 	}
 	return NULL;
 }
@@ -412,11 +413,12 @@ static void sym_add_exported(const char *name, struct module *mod,
 		      s->module->is_vmlinux ? "" : ".ko");
 	}
 
-	s = new_symbol(name, mod, export);
+	s = alloc_symbol(name);
 	s->module = mod;
 	s->is_static = !mod->from_dump;
 	s->export    = export;
 	list_add_tail(&s->list, &mod->exported_symbols);
+	hash_add_symbol(s, exported_symbols);
 }
 
 static void sym_set_crc(const char *name, unsigned int crc)
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 5922b0c39bb7..7c6ece1957fc 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -14,6 +14,8 @@
 #include "list.h"
 #include "elfconfig.h"
 
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
 /* On BSD-alike OSes elf.h defines these according to host's word size */
 #undef ELF_ST_BIND
 #undef ELF_ST_TYPE
-- 
2.32.0

[PATCH 20/27] modpost: mitigate false-negatives for static EXPORT_SYMBOL checks

[Masahiro Yamada]

The 'static' specifier and EXPORT_SYMBOL() are an odd combination.

Since commit 15bfc2348d54 ("modpost: check for static EXPORT_SYMBOL*
functions"), modpost tries to detect it, but there are false negatives.

Here is the sample code.

[Sample 1]

  Makefile:

    obj-m += mymod1.o mymod2.o

  mymod1.c:

    #include <linux/export.h>
    #include <linux/module.h>
    static void foo(void) {}
    EXPORT_SYMBOL(foo);
    MODULE_LICENSE("GPL");

  mymod2.c:

    #include <linux/module.h>
    void foo(void) {}
    MODULE_LICENSE("GPL");

mymod1 exports the static symbol 'foo', but modpost cannot catch it
because it is confused by the same name symbol in another module, mymod2.
(Without mymod2, modpost can detect the error in mymod1)

find_symbol() returns the first symbol found in the hash table with the
given name. This hash table is global, so it may return a symbol from
an unrelated module. So, a global symbol in a module may unset the
'is_static' flag of a different module.

To mitigate this issue, add find_symbol_in_module(), which receives the
module pointer as the second argument. If non-NULL pointer is passed, it
returns the symbol in the specified module. If NULL is passed, it is
equivalent to find_module().

Please note there are still false positives in the composite module,
like below (or when both are built-in). I have no idea how to do this
correctly.

[Sample 2]  (not fixed by this commit)

  Makefile:
    obj-m += mymod.o
    mymod-objs := mymod1.o mymod2.o

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

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

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 908390b4fa80..fb50927cd241 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -271,7 +271,8 @@ static void sym_add_unresolved(const char *name, struct module *mod, bool weak)
 	list_add_tail(&sym->list, &mod->unresolved_symbols);
 }
 
-static struct symbol *find_symbol(const char *name)
+static struct symbol *find_symbol_in_module(const char *name,
+					    struct module *mod)
 {
 	struct symbol *s;
 
@@ -280,11 +281,17 @@ static struct symbol *find_symbol(const char *name)
 		name++;
 
 	hash_for_matched_symbol(s, exported_symbols, name) {
-		return s;
+		if (!mod || s->module == mod)
+			return s;
 	}
 	return NULL;
 }
 
+static struct symbol *find_symbol(const char *name)
+{
+	return find_symbol_in_module(name, NULL);
+}
+
 struct namespace_list {
 	struct list_head list;
 	char namespace[];
@@ -2062,8 +2069,9 @@ static void read_symbols(const char *modname)
 
 		if (bind == STB_GLOBAL || bind == STB_WEAK) {
 			struct symbol *s =
-				find_symbol(remove_dot(info.strtab +
-						       sym->st_name));
+				find_symbol_in_module(remove_dot(info.strtab +
+								 sym->st_name),
+						      mod);
 
 			if (s)
 				s->is_static = false;
-- 
2.32.0

[PATCH 21/27] kbuild: record symbol versions in *.cmd files

[Masahiro Yamada]

When CONFIG_MODVERSIONS=y, the output from genksyms is saved in
separate *.symversions files, and will be used much later when
CONFIG_LTO_CLANG=y because it is impossible to update LLVM bit code
here.

This approach is not robust because:

 - *.symversions may or may not exist. If *.symversions does not
   exist, we never know if it is missing for legitimate reason
   (i.e. no EXPORT_SYMBOL) or something bad has happened (for
   example, the user accidentally deleted it). Once it occurs,
   it is not self-healing because *.symversions is generated
   as a side effect of the build rule of the object.

 - stale (i.e. invalid) *.symversions might be picked up if an
   object is generated in a non-ordinary way, and corresponding
   *.symversions (, which was generated by old builds) just happen
   to exist.

A more robust approach is to save symbol versions in *.cmd files
because:

 - *.cmd always exists (if the object is generated by if_changed
   rule or friends). Even if the user accidentally deletes it,
   it will be regenerated in the next build.

 - *.cmd is always re-generated when the object is updated. This
   avoid stale version information being picked up.

I will remove *.symversions later.

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

 scripts/Makefile.build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index f6a506318795..e03e85c90b26 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -175,6 +175,8 @@ gen_symversions =								\
 	if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then			\
 		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
 		    > $@.symversions;						\
+		echo >> $(dot-target).cmd ;					\
+		sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' $@.symversions >> $(dot-target).cmd ; \
 	else									\
 		rm -f $@.symversions;						\
 	fi
-- 
2.32.0

Re: [PATCH 21/27] kbuild: record symbol versions in *.cmd files

[Nicolas Schier]

On ma. 25. april 2022 kl. 04.08 +0000 Masahiro Yamada wrote:
> When CONFIG_MODVERSIONS=y, the output from genksyms is saved in
> separate *.symversions files, and will be used much later when
> CONFIG_LTO_CLANG=y because it is impossible to update LLVM bit code
> here.
> 
> This approach is not robust because:
> 
>  - *.symversions may or may not exist. If *.symversions does not
>    exist, we never know if it is missing for legitimate reason
>    (i.e. no EXPORT_SYMBOL) or something bad has happened (for
>    example, the user accidentally deleted it). Once it occurs,
>    it is not self-healing because *.symversions is generated
>    as a side effect of the build rule of the object.
> 
>  - stale (i.e. invalid) *.symversions might be picked up if an
>    object is generated in a non-ordinary way, and corresponding
>    *.symversions (, which was generated by old builds) just happen
>    to exist.
> 
> A more robust approach is to save symbol versions in *.cmd files
> because:
> 
>  - *.cmd always exists (if the object is generated by if_changed
>    rule or friends). Even if the user accidentally deletes it,
>    it will be regenerated in the next build.
> 
>  - *.cmd is always re-generated when the object is updated. This
>    avoid stale version information being picked up.
> 
> I will remove *.symversions later.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/Makefile.build | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index f6a506318795..e03e85c90b26 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -175,6 +175,8 @@ gen_symversions =								\
>  	if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then			\
>  		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
>  		    > $@.symversions;						\
> +		echo >> $(dot-target).cmd ;					\
> +		sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' $@.symversions >> $(dot-target).cmd ; \
>  	else									\
>  		rm -f $@.symversions;						\
>  	fi

While reviewing this patch, I was missing the update of the comment a 
few lines above.  But I understand that it makes more sense to update 
it in the follow-up patch.

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

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

[PATCH 22/27] kbuild: generate a list of objects in vmlinux

[Masahiro Yamada]

A *.mod file lists the member objects of a module, but vmlinux does
not have such a file to list out all the member objects.

Generate this list to allow modpost to know all the member objects.

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

 scripts/link-vmlinux.sh | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 20f44504a644..d2c193f82004 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -310,7 +310,7 @@ cleanup()
 	rm -f vmlinux
 	rm -f vmlinux.map
 	rm -f vmlinux.o
-	rm -f .vmlinux.d
+	rm -f .vmlinux*
 }
 
 # Use "make V=1" to debug this script
@@ -342,6 +342,19 @@ ${MAKE} -f "${srctree}/scripts/Makefile.build" obj=init need-builtin=1
 modpost_link vmlinux.o
 objtool_link vmlinux.o
 
+# Generate the list of objects in vmlinux
+rm -f .vmlinux.objs
+for f in ${KBUILD_VMLINUX_OBJS} ${KBUILD_VMLINUX_LIBS}; do
+	case ${f} in
+	*.a)
+		${AR} t ${f} >> .vmlinux.objs
+		;;
+	*)
+		echo ${f} >> .vmlinux.objs
+		;;
+	esac
+done
+
 # modpost vmlinux.o to check for section mismatches
 ${MAKE} -f "${srctree}/scripts/Makefile.modpost" MODPOST_VMLINUX=1
 
-- 
2.32.0

Re: [PATCH 22/27] kbuild: generate a list of objects in vmlinux

[Nicolas Schier]

On Mon 25 Apr 2022 04:08:06 GMT Masahiro Yamada wrote:
> A *.mod file lists the member objects of a module, but vmlinux does
> not have such a file to list out all the member objects.
> 
> Generate this list to allow modpost to know all the member objects.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/link-vmlinux.sh | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index 20f44504a644..d2c193f82004 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -310,7 +310,7 @@ cleanup()
>  	rm -f vmlinux
>  	rm -f vmlinux.map
>  	rm -f vmlinux.o
> -	rm -f .vmlinux.d
> +	rm -f .vmlinux*

Wouldn't it be safer to keep the removal of .vmlinux.d for some time 
and just add .vmlinux.objs?  Somehow I do not like the wildcard here.

>  }
>  
>  # Use "make V=1" to debug this script
> @@ -342,6 +342,19 @@ ${MAKE} -f "${srctree}/scripts/Makefile.build" obj=init need-builtin=1
>  modpost_link vmlinux.o
>  objtool_link vmlinux.o
>  
> +# Generate the list of objects in vmlinux
> +rm -f .vmlinux.objs
> +for f in ${KBUILD_VMLINUX_OBJS} ${KBUILD_VMLINUX_LIBS}; do
> +	case ${f} in
> +	*.a)
> +		${AR} t ${f} >> .vmlinux.objs
> +		;;
> +	*)
> +		echo ${f} >> .vmlinux.objs
> +		;;
> +	esac
> +done

just some bike shedding comment:  I would have left out the 'rm -f 
.vmlinux.objs' and moved the redirection to the end of the for loop:

for f in ...
    ...
    ${AR}
    echo
done > .vmlinux.objs

but it probably not worth.

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

> +
>  # modpost vmlinux.o to check for section mismatches
>  ${MAKE} -f "${srctree}/scripts/Makefile.modpost" MODPOST_VMLINUX=1
>  
> -- 
> 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 --

Re: [PATCH 22/27] kbuild: generate a list of objects in vmlinux

[Masahiro Yamada]

On Thu, Apr 28, 2022 at 11:56 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Mon 25 Apr 2022 04:08:06 GMT Masahiro Yamada wrote:
> > A *.mod file lists the member objects of a module, but vmlinux does
> > not have such a file to list out all the member objects.
> >
> > Generate this list to allow modpost to know all the member objects.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/link-vmlinux.sh | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > index 20f44504a644..d2c193f82004 100755
> > --- a/scripts/link-vmlinux.sh
> > +++ b/scripts/link-vmlinux.sh
> > @@ -310,7 +310,7 @@ cleanup()
> >       rm -f vmlinux
> >       rm -f vmlinux.map
> >       rm -f vmlinux.o
> > -     rm -f .vmlinux.d
> > +     rm -f .vmlinux*
>
> Wouldn't it be safer to keep the removal of .vmlinux.d for some time
> and just add .vmlinux.objs?  Somehow I do not like the wildcard here.

OK, will change in v2.


> >  }
> >
> >  # Use "make V=1" to debug this script
> > @@ -342,6 +342,19 @@ ${MAKE} -f "${srctree}/scripts/Makefile.build" obj=init need-builtin=1
> >  modpost_link vmlinux.o
> >  objtool_link vmlinux.o
> >
> > +# Generate the list of objects in vmlinux
> > +rm -f .vmlinux.objs
> > +for f in ${KBUILD_VMLINUX_OBJS} ${KBUILD_VMLINUX_LIBS}; do
> > +     case ${f} in
> > +     *.a)
> > +             ${AR} t ${f} >> .vmlinux.objs
> > +             ;;
> > +     *)
> > +             echo ${f} >> .vmlinux.objs
> > +             ;;
> > +     esac
> > +done
>
> just some bike shedding comment:  I would have left out the 'rm -f
> .vmlinux.objs' and moved the redirection to the end of the for loop:
>
> for f in ...
>     ...
>     ${AR}
>     echo
> done > .vmlinux.objs

OK, that would shorten the code a little.
Will do in v2.



>
> but it probably not worth.
>
> Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>
>
> > +
> >  # modpost vmlinux.o to check for section mismatches
> >  ${MAKE} -f "${srctree}/scripts/Makefile.modpost" MODPOST_VMLINUX=1
> >
> > --
> > 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

[PATCH 23/27] modpost: retrieve symbol versions by parsing *.cmd files

[Masahiro Yamada]

Currently, CONFIG_MODVERSIONS needs extra link to embed the symbol
versions into objects. Then, modpost parses the ELF objects to retrieve
the version CRCs.

See the current build flow below.

Current implementation
======================

                  embed CRC        |---------|              |------------|
       $(CC)        $(LD)          |         |              | final link |
  *.c ------> *.o --------> *.o -->| modpost |-- *.o ------>| for        |
                      /            |         |              | vmlinux    |
     genksyms        /             |         |-- *.mod.c -->| or module  |
  *.c ------> *.symversions        |---------|              |------------|

Genksyms outputs the calculated CRCs in the form of linker script
(*.symversions), which is used by $(LD) to update the object.

If CONFIG_LTO_CLANG=y, the build process becomes much more complex.
Embedding the CRCs is postponed until the LLVM bitcode is converted
into ELF, creating another intermediate *.prelink.o.

However, this complexity is unneeded in the first place. There is no
reason why we must embed version CRCs in objects so early.

There is final link stage for vmlinux (scripts/link-vmlinux.sh) and
modules (scripts/Makefile.modfinal). We can embed CRCs at the very
last moment.

See the following figure, which explains what I want to do.

New implementation
==================

       $(CC)           |---------|                    |------------|
  *.c ------> *.o  --->|         |-- *.o ------------>| final link |
                       | modpost |-- *.mod.c -------->| for        |
      genksyms         |         |-- *.symver.lds --->| vmlinux    |
  *.c ------> *.cmd -->|         |                    | or module  |
                       |---------|                    |------------|

We can pass the symbol versions to modpost as separate text data.
(The previous commit output the versions in *.cmd files.)

This commit changes modpost to retrieve CRCs from *.cmd files instead
of from ELF objects.

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

 scripts/mod/modpost.c | 177 ++++++++++++++++++++++++++++++++----------
 1 file changed, 136 insertions(+), 41 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index fb50927cd241..43ab4f000ae3 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -246,6 +246,9 @@ static inline unsigned int tdb_hash(const char *name)
 /* hash table of all exported symbols */
 HASHTABLE_DECLARE(exported_symbols, 8192);
 
+/* hash table of CRCs */
+HASHTABLE_DECLARE(crc_symbols, 1024);
+
 /**
  * Allocate a new symbols for use in the hash of exported symbols or
  * the list of unresolved symbols per module
@@ -420,7 +423,27 @@ static void sym_add_exported(const char *name, struct module *mod,
 		      s->module->is_vmlinux ? "" : ".ko");
 	}
 
-	s = alloc_symbol(name);
+	s = NULL;
+
+	if (modversions) {
+		struct hlist_node *n;
+
+		hash_for_matched_symbol_safe(s, n, crc_symbols, name) {
+			if (s->module == mod)
+				hash_del_symbol(s);
+			break;
+		}
+
+		if (!s)
+			warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n"
+			     "Is \"%s\" prototyped in <asm/asm-prototypes.h>?\n",
+			     name, mod->name, mod->is_vmlinux ? "" : ".ko",
+			     name);
+	}
+
+	if (!s)
+		s = alloc_symbol(name);
+
 	s->module = mod;
 	s->is_static = !mod->from_dump;
 	s->export    = export;
@@ -428,19 +451,116 @@ static void sym_add_exported(const char *name, struct module *mod,
 	hash_add_symbol(s, exported_symbols);
 }
 
-static void sym_set_crc(const char *name, unsigned int crc)
+static void sym_add_crc(const char *name, unsigned int crc, struct module *mod)
 {
-	struct symbol *s = find_symbol(name);
+	struct symbol *sym;
 
-	/*
-	 * Ignore stand-alone __crc_*, which might be auto-generated symbols
-	 * such as __*_veneer in ARM ELF.
-	 */
-	if (!s)
+	sym = alloc_symbol(name);
+
+	sym->crc = crc;
+	sym->crc_valid = true;
+	sym->module = mod;
+
+	hash_add_symbol(sym, crc_symbols);
+}
+
+/*
+ * The CRCs are recorded in .*.cmd files in the form of:
+ * #__crc_<name>=<crc>
+ */
+static void retrieve_crcs_from_cmdfile(const char *object, struct module *mod)
+{
+	char cmd_file[PATH_MAX];
+	char *buf, *p;
+	const char *base;
+	int dirlen, ret;
+
+	base = strrchr(object, '/');
+	if (base) {
+		base++;
+		dirlen = base - object;
+	} else {
+		dirlen = 0;
+		base = object;
+	}
+
+	ret = snprintf(cmd_file, sizeof(cmd_file), "%.*s.%s.cmd",
+		       dirlen, object, base);
+	if (ret >= sizeof(cmd_file)) {
+		error("%s: too long path was truncated\n", cmd_file);
 		return;
+	}
+
+	buf = read_text_file(cmd_file);
+	p = buf;
+
+	while ((p = strstr(p, "\n#__crc_"))) {
+		char *name;
+		size_t namelen;
+		unsigned int crc;
+		char *end;
+
+		name = p + strlen("\n#__crc_");
+
+		p = strchr(name, '=');
+		if (!p) {
+			error("invalid\n");
+			goto out;
+		}
+
+		namelen = p - name;
+
+		p++;
 
-	s->crc = crc;
-	s->crc_valid = true;
+		if (!isdigit(*p)) {
+			error("invalid\n");
+			goto out;
+		}
+
+		crc = strtol(p, &end, 0);
+		p = end;
+
+		if (*p != '\n') {
+			error("invalid\n");
+			goto out;
+		}
+
+		name[namelen] = '\0';
+		sym_add_crc(name, crc, mod);
+	}
+
+out:
+	free(buf);
+}
+
+/*
+ * The symbol versions (CRC) are recorded in the .*.cmd files.
+ * Parse them to retrieve CRCs for the current module.
+ */
+static void retrieve_crcs(struct module *mod)
+{
+	char objlist[PATH_MAX];
+	char *buf, *p, *obj;
+	int ret;
+
+	if (mod->is_vmlinux) {
+		strcpy(objlist, ".vmlinux.objs");
+	} else {
+		/* objects for a module are listed in the *.mod file. */
+		ret = snprintf(objlist, sizeof(objlist), "%s.mod", mod->name);
+		if (ret >= sizeof(objlist)) {
+			error("%s: too long path was truncated\n", objlist);
+			return;
+		}
+	}
+
+	buf = read_text_file(objlist);
+	p = buf;
+
+	while ((obj = strsep(&p, "\n")) && obj[0])
+		retrieve_crcs_from_cmdfile(obj, mod);
+
+	free(buf);
 }
 
 static void *grab_file(const char *filename, size_t *size)
@@ -663,33 +783,6 @@ static int ignore_undef_symbol(struct elf_info *info, const char *symname)
 	return 0;
 }
 
-static void handle_modversion(const struct module *mod,
-			      const struct elf_info *info,
-			      const Elf_Sym *sym, const char *symname)
-{
-	unsigned int crc;
-
-	if (sym->st_shndx == SHN_UNDEF) {
-		warn("EXPORT symbol \"%s\" [%s%s] version generation failed, symbol will not be versioned.\n"
-		     "Is \"%s\" prototyped in <asm/asm-prototypes.h>?\n",
-		     symname, mod->name, mod->is_vmlinux ? "" : ".ko",
-		     symname);
-
-		return;
-	}
-
-	if (sym->st_shndx == SHN_ABS) {
-		crc = sym->st_value;
-	} else {
-		unsigned int *crcp;
-
-		/* symbol points to the CRC in the ELF object */
-		crcp = sym_get_data(info, sym);
-		crc = TO_NATIVE(*crcp);
-	}
-	sym_set_crc(symname, crc);
-}
-
 static void handle_symbol(struct module *mod, struct elf_info *info,
 			  const Elf_Sym *sym, const char *symname)
 {
@@ -2019,6 +2112,10 @@ static void read_symbols(const char *modname)
 		if (strends(tmp, ".prelink"))
 			tmp[strlen(tmp) - 8] = '\0';
 		mod = new_module(tmp);
+
+		if (modversions)
+			retrieve_crcs(mod);
+
 		free(tmp);
 	}
 
@@ -2058,9 +2155,6 @@ static void read_symbols(const char *modname)
 		if (strstarts(symname, "__kstrtabns_"))
 			sym_update_namespace(symname + strlen("__kstrtabns_"),
 					     sym_get_data(&info, sym));
-		if (strstarts(symname, "__crc_"))
-			handle_modversion(mod, &info, sym,
-					  symname + strlen("__crc_"));
 	}
 
 	// check for static EXPORT_SYMBOL_* functions && global vars
@@ -2451,8 +2545,9 @@ static void read_dump(const char *fname)
 			mod = new_module(modname);
 			mod->from_dump = true;
 		}
+
+		sym_add_crc(symname, crc, mod);
 		sym_add_exported(symname, mod, export_no(export));
-		sym_set_crc(symname, crc);
 		sym_update_namespace(symname, namespace);
 	}
 	free(buf);
-- 
2.32.0

[PATCH 24/27] modpost: generate linker script to collect symbol versions

[Masahiro Yamada]

Merge version CRCs per vmlinux or per module.

These linker scripts will be fed to the final link stage.

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

 .gitignore            |  1 +
 Makefile              |  1 +
 scripts/mod/modpost.c | 27 +++++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/.gitignore b/.gitignore
index 265959544978..f9dad6b917e6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -42,6 +42,7 @@
 *.so.dbg
 *.su
 *.symtypes
+*.symver.lds
 *.symversions
 *.tab.[ch]
 *.tar
diff --git a/Makefile b/Makefile
index 235d68fa1470..0779db3d1c0c 100644
--- a/Makefile
+++ b/Makefile
@@ -1859,6 +1859,7 @@ clean: $(clean-dirs)
 		-o -name '*.c.[012]*.*' \
 		-o -name '*.ll' \
 		-o -name '*.gcno' \
+		-o -name '*.symver.lds' \
 		-o -name '*.*.symversions' \) -type f -print | xargs rm -f
 
 # Generate tags for editors
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 43ab4f000ae3..ef779ada04c6 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2577,6 +2577,30 @@ static void write_dump(const char *fname)
 	free(buf.p);
 }
 
+static void write_symversions_lds(struct module *mod)
+{
+	struct buffer buf = { };
+	struct symbol *sym;
+	char lds_file[PATH_MAX];
+	int ret;
+
+	ret = snprintf(lds_file, sizeof(lds_file), "%s.symver.lds", mod->name);
+	if (ret >= sizeof(lds_file)) {
+		error("%s: too long path was truncated\n", lds_file);
+		return;
+	}
+
+	list_for_each_entry(sym, &mod->exported_symbols, list) {
+		if (!sym->crc_valid)
+			continue;
+
+		buf_printf(&buf, "__crc_%s = 0x%08x;\n", sym->name, sym->crc);
+	}
+
+	write_if_changed(&buf, lds_file);
+	free(buf.p);
+}
+
 static void write_namespace_deps_files(const char *fname)
 {
 	struct module *mod;
@@ -2673,6 +2697,9 @@ int main(int argc, char **argv)
 		char fname[PATH_MAX];
 		int ret;
 
+		if (modversions && !mod->from_dump)
+			write_symversions_lds(mod);
+
 		if (mod->is_vmlinux || mod->from_dump)
 			continue;
 
-- 
2.32.0

Re: [PATCH 24/27] modpost: generate linker script to collect symbol versions

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:10 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Merge version CRCs per vmlinux or per module.
>
> These linker scripts will be fed to the final link stage.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  .gitignore            |  1 +
>  Makefile              |  1 +
>  scripts/mod/modpost.c | 27 +++++++++++++++++++++++++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/.gitignore b/.gitignore
> index 265959544978..f9dad6b917e6 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -42,6 +42,7 @@
>  *.so.dbg
>  *.su
>  *.symtypes
> +*.symver.lds
>  *.symversions
>  *.tab.[ch]
>  *.tar
> diff --git a/Makefile b/Makefile
> index 235d68fa1470..0779db3d1c0c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1859,6 +1859,7 @@ clean: $(clean-dirs)
>                 -o -name '*.c.[012]*.*' \
>                 -o -name '*.ll' \
>                 -o -name '*.gcno' \
> +               -o -name '*.symver.lds' \
>                 -o -name '*.*.symversions' \) -type f -print | xargs rm -f
>
>  # Generate tags for editors
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 43ab4f000ae3..ef779ada04c6 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2577,6 +2577,30 @@ static void write_dump(const char *fname)
>         free(buf.p);
>  }
>
> +static void write_symversions_lds(struct module *mod)
> +{
> +       struct buffer buf = { };
> +       struct symbol *sym;
> +       char lds_file[PATH_MAX];
> +       int ret;
> +
> +       ret = snprintf(lds_file, sizeof(lds_file), "%s.symver.lds", mod->name);
> +       if (ret >= sizeof(lds_file)) {
> +               error("%s: too long path was truncated\n", lds_file);
> +               return;

If this exceptional case occurs, we should probably halt or return an
error code to main?

> +       }
> +
> +       list_for_each_entry(sym, &mod->exported_symbols, list) {
> +               if (!sym->crc_valid)
> +                       continue;
> +
> +               buf_printf(&buf, "__crc_%s = 0x%08x;\n", sym->name, sym->crc);
> +       }
> +
> +       write_if_changed(&buf, lds_file);
> +       free(buf.p);
> +}
> +
>  static void write_namespace_deps_files(const char *fname)
>  {
>         struct module *mod;
> @@ -2673,6 +2697,9 @@ int main(int argc, char **argv)
>                 char fname[PATH_MAX];
>                 int ret;
>
> +               if (modversions && !mod->from_dump)
> +                       write_symversions_lds(mod);
> +
>                 if (mod->is_vmlinux || mod->from_dump)
>                         continue;
>
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 24/27] modpost: generate linker script to collect symbol versions

[Masahiro Yamada]

On Fri, Apr 29, 2022 at 6:49 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sun, Apr 24, 2022 at 12:10 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Merge version CRCs per vmlinux or per module.
> >
> > These linker scripts will be fed to the final link stage.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  .gitignore            |  1 +
> >  Makefile              |  1 +
> >  scripts/mod/modpost.c | 27 +++++++++++++++++++++++++++
> >  3 files changed, 29 insertions(+)
> >
> > diff --git a/.gitignore b/.gitignore
> > index 265959544978..f9dad6b917e6 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -42,6 +42,7 @@
> >  *.so.dbg
> >  *.su
> >  *.symtypes
> > +*.symver.lds
> >  *.symversions
> >  *.tab.[ch]
> >  *.tar
> > diff --git a/Makefile b/Makefile
> > index 235d68fa1470..0779db3d1c0c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1859,6 +1859,7 @@ clean: $(clean-dirs)
> >                 -o -name '*.c.[012]*.*' \
> >                 -o -name '*.ll' \
> >                 -o -name '*.gcno' \
> > +               -o -name '*.symver.lds' \
> >                 -o -name '*.*.symversions' \) -type f -print | xargs rm -f
> >
> >  # Generate tags for editors
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 43ab4f000ae3..ef779ada04c6 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -2577,6 +2577,30 @@ static void write_dump(const char *fname)
> >         free(buf.p);
> >  }
> >
> > +static void write_symversions_lds(struct module *mod)
> > +{
> > +       struct buffer buf = { };
> > +       struct symbol *sym;
> > +       char lds_file[PATH_MAX];
> > +       int ret;
> > +
> > +       ret = snprintf(lds_file, sizeof(lds_file), "%s.symver.lds", mod->name);
> > +       if (ret >= sizeof(lds_file)) {
> > +               error("%s: too long path was truncated\n", lds_file);
> > +               return;
>
> If this exceptional case occurs, we should probably halt or return an
> error code to main?



Could you see the comments at the bottom of
scripts/mod/modpost.h  ?


error() functions sets 'error_occurred' variable,
so main() function will exist with 1 at the end.


If we bail out at the very first error, users would
compile the kernel again and again
to fix errors one by one.

modpost continues running as far as possible
to display as many errors as possible.

Compilers usually do similar things.








> > +       }
> > +
> > +       list_for_each_entry(sym, &mod->exported_symbols, list) {
> > +               if (!sym->crc_valid)
> > +                       continue;
> > +
> > +               buf_printf(&buf, "__crc_%s = 0x%08x;\n", sym->name, sym->crc);
> > +       }
> > +
> > +       write_if_changed(&buf, lds_file);
> > +       free(buf.p);
> > +}
> > +
> >  static void write_namespace_deps_files(const char *fname)
> >  {
> >         struct module *mod;
> > @@ -2673,6 +2697,9 @@ int main(int argc, char **argv)
> >                 char fname[PATH_MAX];
> >                 int ret;
> >
> > +               if (modversions && !mod->from_dump)
> > +                       write_symversions_lds(mod);
> > +
> >                 if (mod->is_vmlinux || mod->from_dump)
> >                         continue;
> >
> > --
> > 2.32.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Best Regards
Masahiro Yamada

[PATCH 25/27] kbuild: embed symbol versions at final link of vmlinux or modules

[Masahiro Yamada]

Do not update objects with version CRCs while the directory descending.

Do it at the final link stage.

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

 scripts/Makefile.build    | 19 +++----------------
 scripts/Makefile.modfinal |  3 ++-
 scripts/link-vmlinux.sh   |  4 +++-
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index e03e85c90b26..aadc16e04632 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -162,12 +162,9 @@ ifdef CONFIG_MODVERSIONS
 # 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 .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
-# o remove .tmp_<file>.o to <file>.o
+#   genksyms on the preprocessed source and dump them into .cmd file.
+#   modpost will parse .cmd files to retrieve versions to create linker
+#   scripts that are fed to the final linking of vmlinux or modules.
 
 # Generate .o.symversions files for each .o with exported symbols, and link these
 # to the kernel and/or modules at the end.
@@ -183,12 +180,6 @@ gen_symversions =								\
 
 cmd_gen_symversions_c =	$(call gen_symversions,c)
 
-cmd_modversions =								\
-	if [ -r $@.symversions ]; then						\
-		$(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@ 		\
-			-T $@.symversions;					\
-		mv -f $(@D)/.tmp_$(@F) $@;					\
-	fi
 endif
 
 ifdef CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT
@@ -268,7 +259,6 @@ define rule_cc_o_c
 	$(call cmd,checkdoc)
 	$(call cmd,gen_objtooldep)
 	$(call cmd,gen_symversions_c)
-	$(if $(CONFIG_LTO_CLANG),,$(call cmd,modversions))
 	$(call cmd,record_mcount)
 endef
 
@@ -277,7 +267,6 @@ define rule_as_o_S
 	$(call cmd,gen_ksymdeps)
 	$(call cmd,gen_objtooldep)
 	$(call cmd,gen_symversions_S)
-	$(call cmd,modversions)
 endef
 
 # Built-in and composite module parts
@@ -291,8 +280,6 @@ ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 quiet_cmd_cc_prelink_modules = LD [M]  $@
       cmd_cc_prelink_modules =						\
 	$(LD) $(ld_flags) -r -o $@					\
-               $(shell [ -s $(@:.prelink.o=.o.symversions) ] &&		\
-                       echo -T $(@:.prelink.o=.o.symversions))		\
 		--whole-archive $(filter-out FORCE,$^)			\
 		$(cmd_objtool)
 
diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
index 7f39599e9fae..d429e3f9ae1d 100644
--- a/scripts/Makefile.modfinal
+++ b/scripts/Makefile.modfinal
@@ -34,6 +34,7 @@ quiet_cmd_ld_ko_o = LD [M]  $@
       cmd_ld_ko_o +=							\
 	$(LD) -r $(KBUILD_LDFLAGS)					\
 		$(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)		\
+		$(addprefix -T, $(filter %.symver.lds, $(real-prereqs)))\
 		-T scripts/module.lds -o $@ $(filter %.o, $^);		\
 	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
 
@@ -56,7 +57,7 @@ if_changed_except = $(if $(call newer_prereqs_except,$(2))$(cmd-check),      \
 
 
 # Re-generate module BTFs if either module's .ko or vmlinux changed
-$(modules): %.ko: %$(mod-prelink-ext).o %.mod.o scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
+$(modules): %.ko: %$(mod-prelink-ext).o %.mod.o $(if $(CONFIG_MODVERSIONS), %.symver.lds) scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
 	+$(call if_changed_except,ld_ko_o,vmlinux)
 ifdef CONFIG_DEBUG_INFO_BTF_MODULES
 	+$(if $(newer-prereqs),$(call cmd,btf_ko))
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index d2c193f82004..66a115f204eb 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -90,7 +90,6 @@ modpost_link()
 
 		if is_enabled CONFIG_MODVERSIONS; then
 			gen_symversions
-			lds="${lds} -T .tmp_symversions.lds"
 		fi
 
 		# This might take a while, so indicate that we're doing
@@ -196,6 +195,9 @@ vmlinux_link()
 	fi
 
 	ldflags="${ldflags} ${wl}--script=${objtree}/${KBUILD_LDS}"
+	if is_enabled CONFIG_MODVERSIONS; then
+		ldflags="${ldflags} ${wl}--script=vmlinux.symver.lds"
+	fi
 
 	# The kallsyms linking does not need debug symbols included.
 	if [ "$output" != "${output#.tmp_vmlinux.kallsyms}" ] ; then
-- 
2.32.0

Re: [PATCH 25/27] kbuild: embed symbol versions at final link of vmlinux or modules

[Nicolas Schier]

On Mon 25 Apr 2022 04:08:09 GMT Masahiro Yamada wrote:
> Do not update objects with version CRCs while the directory 
> descending.
> 
> Do it at the final link stage.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

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

> ---
> 
>  scripts/Makefile.build    | 19 +++----------------
>  scripts/Makefile.modfinal |  3 ++-
>  scripts/link-vmlinux.sh   |  4 +++-
>  3 files changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index e03e85c90b26..aadc16e04632 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -162,12 +162,9 @@ ifdef CONFIG_MODVERSIONS
>  # 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 .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
> -# o remove .tmp_<file>.o to <file>.o
> +#   genksyms on the preprocessed source and dump them into .cmd file.
> +#   modpost will parse .cmd files to retrieve versions to create linker
> +#   scripts that are fed to the final linking of vmlinux or modules.
>  
>  # Generate .o.symversions files for each .o with exported symbols, and link these
>  # to the kernel and/or modules at the end.
> @@ -183,12 +180,6 @@ gen_symversions =								\
>  
>  cmd_gen_symversions_c =	$(call gen_symversions,c)
>  
> -cmd_modversions =								\
> -	if [ -r $@.symversions ]; then						\
> -		$(LD) $(KBUILD_LDFLAGS) -r -o $(@D)/.tmp_$(@F) $@ 		\
> -			-T $@.symversions;					\
> -		mv -f $(@D)/.tmp_$(@F) $@;					\
> -	fi
>  endif
>  
>  ifdef CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT
> @@ -268,7 +259,6 @@ define rule_cc_o_c
>  	$(call cmd,checkdoc)
>  	$(call cmd,gen_objtooldep)
>  	$(call cmd,gen_symversions_c)
> -	$(if $(CONFIG_LTO_CLANG),,$(call cmd,modversions))
>  	$(call cmd,record_mcount)
>  endef
>  
> @@ -277,7 +267,6 @@ define rule_as_o_S
>  	$(call cmd,gen_ksymdeps)
>  	$(call cmd,gen_objtooldep)
>  	$(call cmd,gen_symversions_S)
> -	$(call cmd,modversions)
>  endef
>  
>  # Built-in and composite module parts
> @@ -291,8 +280,6 @@ ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
>  quiet_cmd_cc_prelink_modules = LD [M]  $@
>        cmd_cc_prelink_modules =						\
>  	$(LD) $(ld_flags) -r -o $@					\
> -               $(shell [ -s $(@:.prelink.o=.o.symversions) ] &&		\
> -                       echo -T $(@:.prelink.o=.o.symversions))		\
>  		--whole-archive $(filter-out FORCE,$^)			\
>  		$(cmd_objtool)
>  
> diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> index 7f39599e9fae..d429e3f9ae1d 100644
> --- a/scripts/Makefile.modfinal
> +++ b/scripts/Makefile.modfinal
> @@ -34,6 +34,7 @@ quiet_cmd_ld_ko_o = LD [M]  $@
>        cmd_ld_ko_o +=							\
>  	$(LD) -r $(KBUILD_LDFLAGS)					\
>  		$(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)		\
> +		$(addprefix -T, $(filter %.symver.lds, $(real-prereqs)))\
>  		-T scripts/module.lds -o $@ $(filter %.o, $^);		\
>  	$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>  
> @@ -56,7 +57,7 @@ if_changed_except = $(if $(call newer_prereqs_except,$(2))$(cmd-check),      \
>  
>  
>  # Re-generate module BTFs if either module's .ko or vmlinux changed
> -$(modules): %.ko: %$(mod-prelink-ext).o %.mod.o scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
> +$(modules): %.ko: %$(mod-prelink-ext).o %.mod.o $(if $(CONFIG_MODVERSIONS), %.symver.lds) scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
>  	+$(call if_changed_except,ld_ko_o,vmlinux)
>  ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>  	+$(if $(newer-prereqs),$(call cmd,btf_ko))
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index d2c193f82004..66a115f204eb 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -90,7 +90,6 @@ modpost_link()
>  
>  		if is_enabled CONFIG_MODVERSIONS; then
>  			gen_symversions
> -			lds="${lds} -T .tmp_symversions.lds"
>  		fi
>  
>  		# This might take a while, so indicate that we're doing
> @@ -196,6 +195,9 @@ vmlinux_link()
>  	fi
>  
>  	ldflags="${ldflags} ${wl}--script=${objtree}/${KBUILD_LDS}"
> +	if is_enabled CONFIG_MODVERSIONS; then
> +		ldflags="${ldflags} ${wl}--script=vmlinux.symver.lds"
> +	fi
>  
>  	# The kallsyms linking does not need debug symbols included.
>  	if [ "$output" != "${output#.tmp_vmlinux.kallsyms}" ] ; then
> -- 
> 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 --

[PATCH 26/27] kbuild: stop generating *.symversions

[Masahiro Yamada]

Now *.symversions is unneeded. Clean up the Makefile and script.

I will keep *.symversions in .gitignore and 'make clean' for a while
to avoid flooding the output from 'git status'.

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

 scripts/Makefile.build  | 28 ++++------------------------
 scripts/link-vmlinux.sh | 19 -------------------
 2 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index aadc16e04632..7f199b0a5170 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -170,12 +170,9 @@ ifdef CONFIG_MODVERSIONS
 # to the kernel and/or modules at the end.
 gen_symversions =								\
 	if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then			\
-		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
-		    > $@.symversions;						\
 		echo >> $(dot-target).cmd ;					\
-		sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' $@.symversions >> $(dot-target).cmd ; \
-	else									\
-		rm -f $@.symversions;						\
+		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) | \
+		sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' >> $(dot-target).cmd ; \
 	fi
 
 cmd_gen_symversions_c =	$(call gen_symversions,c)
@@ -391,17 +388,6 @@ $(obj)/%.asn1.c $(obj)/%.asn1.h: $(src)/%.asn1 $(objtree)/scripts/asn1_compiler
 $(subdir-builtin): $(obj)/%/built-in.a: $(obj)/% ;
 $(subdir-modorder): $(obj)/%/modules.order: $(obj)/% ;
 
-# combine symversions for later processing
-ifeq ($(CONFIG_LTO_CLANG) $(CONFIG_MODVERSIONS),y y)
-      cmd_update_lto_symversions =					\
-	rm -f $@.symversions						\
-	$(foreach n, $(filter-out FORCE,$^),				\
-		$(if $(shell test -s $(n).symversions && echo y),	\
-			; cat $(n).symversions >> $@.symversions))
-else
-      cmd_update_lto_symversions = echo >/dev/null
-endif
-
 #
 # Rule to compile a set of .o files into one .a file (without symbol table)
 #
@@ -409,11 +395,8 @@ endif
 quiet_cmd_ar_builtin = AR      $@
       cmd_ar_builtin = rm -f $@; $(AR) cDPrST $@ $(real-prereqs)
 
-quiet_cmd_ar_and_symver = AR      $@
-      cmd_ar_and_symver = $(cmd_update_lto_symversions); $(cmd_ar_builtin)
-
 $(obj)/built-in.a: $(real-obj-y) FORCE
-	$(call if_changed,ar_and_symver)
+	$(call if_changed,ar_builtin)
 
 #
 # Rule to create modules.order file
@@ -433,16 +416,13 @@ $(obj)/modules.order: $(obj-m) FORCE
 #
 # Rule to compile a set of .o files into one .a file (with symbol table)
 #
-quiet_cmd_ar_lib = AR      $@
-      cmd_ar_lib = $(cmd_update_lto_symversions); $(cmd_ar)
 
 $(obj)/lib.a: $(lib-y) FORCE
-	$(call if_changed,ar_lib)
+	$(call if_changed,ar)
 
 ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 quiet_cmd_link_multi-m = AR [M]  $@
 cmd_link_multi-m =						\
-	$(cmd_update_lto_symversions);				\
 	rm -f $@; 						\
 	$(AR) cDPrsT $@ @$(patsubst %.o,%.mod,$@)
 else
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 66a115f204eb..e2900d3bd215 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -56,20 +56,6 @@ gen_initcalls()
 		> .tmp_initcalls.lds
 }
 
-# If CONFIG_LTO_CLANG is selected, collect generated symbol versions into
-# .tmp_symversions.lds
-gen_symversions()
-{
-	info GEN .tmp_symversions.lds
-	rm -f .tmp_symversions.lds
-
-	for o in ${KBUILD_VMLINUX_OBJS} ${KBUILD_VMLINUX_LIBS}; do
-		if [ -f ${o}.symversions ]; then
-			cat ${o}.symversions >> .tmp_symversions.lds
-		fi
-	done
-}
-
 # Link of vmlinux.o used for section mismatch analysis
 # ${1} output file
 modpost_link()
@@ -88,10 +74,6 @@ modpost_link()
 		gen_initcalls
 		lds="-T .tmp_initcalls.lds"
 
-		if is_enabled CONFIG_MODVERSIONS; then
-			gen_symversions
-		fi
-
 		# This might take a while, so indicate that we're doing
 		# an LTO link
 		info LTO ${1}
@@ -306,7 +288,6 @@ cleanup()
 	rm -f .btf.*
 	rm -f .tmp_System.map
 	rm -f .tmp_initcalls.lds
-	rm -f .tmp_symversions.lds
 	rm -f .tmp_vmlinux*
 	rm -f System.map
 	rm -f vmlinux
-- 
2.32.0

Re: [PATCH 26/27] kbuild: stop generating *.symversions

[Nicolas Schier]

On Mon 25 Apr 2022 04:08:10 GMT Masahiro Yamada wrote:
> Now *.symversions is unneeded. Clean up the Makefile and script.
> 
> I will keep *.symversions in .gitignore and 'make clean' for a while
> to avoid flooding the output from 'git status'.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/Makefile.build  | 28 ++++------------------------
>  scripts/link-vmlinux.sh | 19 -------------------
>  2 files changed, 4 insertions(+), 43 deletions(-)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index aadc16e04632..7f199b0a5170 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -170,12 +170,9 @@ ifdef CONFIG_MODVERSIONS
>  # to the kernel and/or modules at the end.
>  gen_symversions =								\
>  	if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then			\
> -		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
> -		    > $@.symversions;						\
>  		echo >> $(dot-target).cmd ;					\
> -		sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' $@.symversions >> $(dot-target).cmd ; \
> -	else									\
> -		rm -f $@.symversions;						\
> +		$(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) | \
> +		sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' >> $(dot-target).cmd ; \

I was wondering, whether we should integrate the 'sed' part into 
genksyms and make it output the lines as we need them.
If genksyms fails, we now don't see any error code and make will 
silently continue as the pipe hides genksyms' exit code, right?

>  	fi
>  
>  cmd_gen_symversions_c =	$(call gen_symversions,c)
> @@ -391,17 +388,6 @@ $(obj)/%.asn1.c $(obj)/%.asn1.h: $(src)/%.asn1 $(objtree)/scripts/asn1_compiler
>  $(subdir-builtin): $(obj)/%/built-in.a: $(obj)/% ;
>  $(subdir-modorder): $(obj)/%/modules.order: $(obj)/% ;
>  
> -# combine symversions for later processing
> -ifeq ($(CONFIG_LTO_CLANG) $(CONFIG_MODVERSIONS),y y)
> -      cmd_update_lto_symversions =					\
> -	rm -f $@.symversions						\
> -	$(foreach n, $(filter-out FORCE,$^),				\
> -		$(if $(shell test -s $(n).symversions && echo y),	\
> -			; cat $(n).symversions >> $@.symversions))
> -else
> -      cmd_update_lto_symversions = echo >/dev/null
> -endif
> -
>  #
>  # Rule to compile a set of .o files into one .a file (without symbol table)
>  #
> @@ -409,11 +395,8 @@ endif
>  quiet_cmd_ar_builtin = AR      $@
>        cmd_ar_builtin = rm -f $@; $(AR) cDPrST $@ $(real-prereqs)
>  
> -quiet_cmd_ar_and_symver = AR      $@
> -      cmd_ar_and_symver = $(cmd_update_lto_symversions); $(cmd_ar_builtin)
> -
>  $(obj)/built-in.a: $(real-obj-y) FORCE
> -	$(call if_changed,ar_and_symver)
> +	$(call if_changed,ar_builtin)
>  
>  #
>  # Rule to create modules.order file
> @@ -433,16 +416,13 @@ $(obj)/modules.order: $(obj-m) FORCE
>  #
>  # Rule to compile a set of .o files into one .a file (with symbol table)
>  #
> -quiet_cmd_ar_lib = AR      $@
> -      cmd_ar_lib = $(cmd_update_lto_symversions); $(cmd_ar)
>  
>  $(obj)/lib.a: $(lib-y) FORCE
> -	$(call if_changed,ar_lib)
> +	$(call if_changed,ar)
>  
>  ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
>  quiet_cmd_link_multi-m = AR [M]  $@
>  cmd_link_multi-m =						\
> -	$(cmd_update_lto_symversions);				\
>  	rm -f $@; 						\
>  	$(AR) cDPrsT $@ @$(patsubst %.o,%.mod,$@)
>  else
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index 66a115f204eb..e2900d3bd215 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -56,20 +56,6 @@ gen_initcalls()
>  		> .tmp_initcalls.lds
>  }
>  
> -# If CONFIG_LTO_CLANG is selected, collect generated symbol versions into
> -# .tmp_symversions.lds
> -gen_symversions()
> -{
> -	info GEN .tmp_symversions.lds
> -	rm -f .tmp_symversions.lds
> -
> -	for o in ${KBUILD_VMLINUX_OBJS} ${KBUILD_VMLINUX_LIBS}; do
> -		if [ -f ${o}.symversions ]; then
> -			cat ${o}.symversions >> .tmp_symversions.lds
> -		fi
> -	done
> -}
> -
>  # Link of vmlinux.o used for section mismatch analysis
>  # ${1} output file
>  modpost_link()
> @@ -88,10 +74,6 @@ modpost_link()
>  		gen_initcalls
>  		lds="-T .tmp_initcalls.lds"
>  
> -		if is_enabled CONFIG_MODVERSIONS; then
> -			gen_symversions
> -		fi
> -
>  		# This might take a while, so indicate that we're doing
>  		# an LTO link
>  		info LTO ${1}
> @@ -306,7 +288,6 @@ cleanup()
>  	rm -f .btf.*
>  	rm -f .tmp_System.map
>  	rm -f .tmp_initcalls.lds
> -	rm -f .tmp_symversions.lds
>  	rm -f .tmp_vmlinux*
>  	rm -f System.map
>  	rm -f vmlinux
> -- 
> 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 --

Re: [PATCH 26/27] kbuild: stop generating *.symversions

[Masahiro Yamada]

On Thu, Apr 28, 2022 at 12:15 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Mon 25 Apr 2022 04:08:10 GMT Masahiro Yamada wrote:
> > Now *.symversions is unneeded. Clean up the Makefile and script.
> >
> > I will keep *.symversions in .gitignore and 'make clean' for a while
> > to avoid flooding the output from 'git status'.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/Makefile.build  | 28 ++++------------------------
> >  scripts/link-vmlinux.sh | 19 -------------------
> >  2 files changed, 4 insertions(+), 43 deletions(-)
> >
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index aadc16e04632..7f199b0a5170 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -170,12 +170,9 @@ ifdef CONFIG_MODVERSIONS
> >  # to the kernel and/or modules at the end.
> >  gen_symversions =                                                            \
> >       if $(NM) $@ 2>/dev/null | grep -q __ksymtab; then                       \
> > -             $(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) \
> > -                 > $@.symversions;                                           \
> >               echo >> $(dot-target).cmd ;                                     \
> > -             sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' $@.symversions >> $(dot-target).cmd ; \
> > -     else                                                                    \
> > -             rm -f $@.symversions;                                           \
> > +             $(call cmd_gensymtypes_$(1),$(KBUILD_SYMTYPES),$(@:.o=.symtypes)) | \
> > +             sed 's/\(.*\) = \(.*\);/$(pound)\1=\2/' >> $(dot-target).cmd ; \
>
> I was wondering, whether we should integrate the 'sed' part into
> genksyms and make it output the lines as we need them.
> If genksyms fails, we now don't see any error code and make will
> silently continue as the pipe hides genksyms' exit code, right?


You are right.
I was also thinking of doing this directly in genksyms.
I will do it in v2.

bash supports 'set -o pipefail', but I do not know how
to do it in other shells.

I am also thinking of switching over to SHELL=/bin/bash,
but we cannot rely on bash'ism here yet.

-- 
Best Regards
Masahiro Yamada

[PATCH 27/27] kbuild: do not create *.prelink.o for Clang LTO or IBT

[Masahiro Yamada]

When CONFIG_LTO_CLANG=y, additional intermediate *.prelink.o is created
for each module. Also, objtool is postponed until LLVM bitcode is
converted to ELF.

CONFIG_X86_KERNEL_IBT works in a similar way to postpone objtool until
objects are merged together.

This commit stops generating *.prelink.o, so the build flow will look
the same with/without LTO.

The following figures show how the LTO build currently works, and
how this commit is changing it.

Current build flow
==================

 [1] single-object module

                                    [+objtool]
           $(CC)                      $(LD)                $(LD)
    foo.c --------------------> foo.o -----> foo.prelink.o -----> foo.ko
                           (LLVM bitcode)        (ELF)       |
                                                             |
                                                 foo.mod.o --/

 [2] multi-object module
                                    [+objtool]
           $(CC)         $(AR)        $(LD)                $(LD)
    foo1.c -----> foo1.o -----> foo.o -----> foo.prelink.o -----> foo.ko
                           |  (archive)          (ELF)       |
    foo2.c -----> foo2.o --/                                 |
                (LLVM bitcode)                   foo.mod.o --/

  One confusion is foo.o in multi-object module is an archive despite of
  its suffix.

New build flow
==============

 [1] single-object module

  Since there is only one object, we do not need to have the LLVM
  bitcode stage. Use $(CC)+$(LD) to generate an ELF object in one
  build rule. Of course, only $(CC) is used when LTO is disabled.

           $(CC)+$(LD)[+objtool]           $(LD)
    foo.c ------------------------> foo.o -------> foo.ko
                                    (ELF)    |
                                             |
                                 foo.mod.o --/

 [2] multi-object module

  Previously, $(AR) was used to combine LLVM bitcode into an archive,
  but there was not a technical reason to do so.
  This commit just uses $(LD) to combine and convert them into a single
  ELF object.

                          [+objtool]
            $(CC)           $(LD)          $(LD)
    foo1.c -------> foo1.o -------> foo.o -------> foo.ko
                              |     (ELF)    |
    foo2.c -------> foo2.o ---/              |
                (LLVM bitcode)   foo.mod.o --/

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

 scripts/Kbuild.include    |  4 +++
 scripts/Makefile.build    | 58 ++++++++++++---------------------------
 scripts/Makefile.lib      |  7 -----
 scripts/Makefile.modfinal |  5 ++--
 scripts/Makefile.modpost  |  9 ++----
 scripts/mod/modpost.c     |  7 -----
 6 files changed, 25 insertions(+), 65 deletions(-)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 3514c2149e9d..455a0a6ce12d 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -15,6 +15,10 @@ pound := \#
 # Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
 dot-target = $(dir $@).$(notdir $@)
 
+###
+# Name of target with a '.tmp_' as filename prefix. foo/bar.o => foo/.tmp_bar.o
+tmp-target = $(dir $@).tmp_$(notdir $@)
+
 ###
 # The temporary file to save gcc -MMD generated dependencies must not
 # contain a comma
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 7f199b0a5170..fe4d3a908dd0 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -88,10 +88,6 @@ endif
 targets-for-modules := $(foreach x, o mod $(if $(CONFIG_TRIM_UNUSED_KSYMS), usyms), \
 				$(patsubst %.o, %.$x, $(filter %.o, $(obj-m))))
 
-ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
-targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
-endif
-
 ifdef need-modorder
 targets-for-modules += $(obj)/modules.order
 endif
@@ -153,8 +149,16 @@ $(obj)/%.ll: $(src)/%.c FORCE
 # The C file is compiled and updated dependency information is generated.
 # (See cmd_cc_o_c + relevant part of rule_cc_o_c)
 
+is-single-obj-m = $(if $(part-of-module),$(if $(filter $@, $(obj-m)),y))
+
+ifdef CONFIG_LTO_CLANG
+cmd_ld_single = $(if $(is-single-obj-m), ; $(LD) $(ld_flags) -r -o $(tmp-target) $@; mv $(tmp-target) $@)
+endif
+
 quiet_cmd_cc_o_c = CC $(quiet_modtag)  $@
-      cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< $(cmd_objtool)
+      cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< \
+		$(cmd_ld_single) \
+		$(cmd_objtool)
 
 ifdef CONFIG_MODVERSIONS
 # When module versioning is enabled the following steps are executed:
@@ -228,21 +232,16 @@ cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(o
 
 endif # CONFIG_STACK_VALIDATION
 
-ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
-
-# Skip objtool for LLVM bitcode
-$(obj)/%.o: objtool-enabled :=
-
-else
 
 # '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
 
-$(obj)/%.o: objtool-enabled = $(if $(filter-out y%, \
-	$(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n),y)
+is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n),y)
 
-endif
+delay-objtool := $(or $(CONFIG_LTO_CLANG),$(CONFIG_X86_KERNEL_IBT))
+
+$(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
 
 ifdef CONFIG_TRIM_UNUSED_KSYMS
 cmd_gen_ksymdeps = \
@@ -271,24 +270,6 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
 	$(call if_changed_rule,cc_o_c)
 	$(call cmd,force_checksrc)
 
-ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
-# Module .o files may contain LLVM bitcode, compile them into native code
-# before ELF processing
-quiet_cmd_cc_prelink_modules = LD [M]  $@
-      cmd_cc_prelink_modules =						\
-	$(LD) $(ld_flags) -r -o $@					\
-		--whole-archive $(filter-out FORCE,$^)			\
-		$(cmd_objtool)
-
-# objtool was skipped for LLVM bitcode, run it now that we have compiled
-# modules into native code
-$(obj)/%.prelink.o: objtool-enabled = y
-$(obj)/%.prelink.o: part-of-module := y
-
-$(obj)/%.prelink.o: $(obj)/%.o FORCE
-	$(call if_changed,cc_prelink_modules)
-endif
-
 cmd_mod = echo $(addprefix $(obj)/, $(call real-search, $*.o, .o, -objs -y -m)) | \
 	$(AWK) -v RS='( |\n)' '!x[$$0]++' > $@
 
@@ -298,7 +279,7 @@ $(obj)/%.mod: FORCE
 # List module undefined symbols
 cmd_undefined_syms = $(NM) $< | sed -n 's/^  *U //p' > $@
 
-$(obj)/%.usyms: $(obj)/%$(mod-prelink-ext).o FORCE
+$(obj)/%.usyms: $(obj)/%.o FORCE
 	$(call if_changed,undefined_syms)
 
 quiet_cmd_cc_lst_c = MKLST   $@
@@ -420,16 +401,11 @@ $(obj)/modules.order: $(obj-m) FORCE
 $(obj)/lib.a: $(lib-y) FORCE
 	$(call if_changed,ar)
 
-ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
-quiet_cmd_link_multi-m = AR [M]  $@
-cmd_link_multi-m =						\
-	rm -f $@; 						\
-	$(AR) cDPrsT $@ @$(patsubst %.o,%.mod,$@)
-else
 quiet_cmd_link_multi-m = LD [M]  $@
-      cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ @$(patsubst %.o,%.mod,$@)
-endif
+      cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ @$(patsubst %.o,%.mod,$@) $(cmd_objtool)
 
+$(multi-obj-m): objtool-enabled := $(delay-objtool)
+$(multi-obj-m): part-of-module := y
 $(multi-obj-m): %.o: %.mod FORCE
 	$(call if_changed,link_multi-m)
 $(call multi_depend, $(multi-obj-m), .o, -objs -y -m)
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 0453a1904646..f75138385449 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -225,13 +225,6 @@ dtc_cpp_flags  = -Wp,-MMD,$(depfile).pre.tmp -nostdinc                    \
 		 $(addprefix -I,$(DTC_INCLUDE))                          \
 		 -undef -D__DTS__
 
-ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
-# With CONFIG_LTO_CLANG, .o files in modules might be LLVM bitcode, so we
-# need to run LTO to compile them into native code (.lto.o) before further
-# processing.
-mod-prelink-ext := .prelink
-endif
-
 # Useful for describing the dependency of composite objects
 # Usage:
 #   $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
index d429e3f9ae1d..51d384a0e4f9 100644
--- a/scripts/Makefile.modfinal
+++ b/scripts/Makefile.modfinal
@@ -9,7 +9,7 @@ __modfinal:
 include include/config/auto.conf
 include $(srctree)/scripts/Kbuild.include
 
-# for c_flags and mod-prelink-ext
+# for c_flags
 include $(srctree)/scripts/Makefile.lib
 
 # find all modules listed in modules.order
@@ -55,9 +55,8 @@ if_changed_except = $(if $(call newer_prereqs_except,$(2))$(cmd-check),      \
 	$(cmd);                                                              \
 	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
 
-
 # Re-generate module BTFs if either module's .ko or vmlinux changed
-$(modules): %.ko: %$(mod-prelink-ext).o %.mod.o $(if $(CONFIG_MODVERSIONS), %.symver.lds) scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
+$(modules): %.ko: %.o %.mod.o $(if $(CONFIG_MODVERSIONS), %.symver.lds) scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
 	+$(call if_changed_except,ld_ko_o,vmlinux)
 ifdef CONFIG_DEBUG_INFO_BTF_MODULES
 	+$(if $(newer-prereqs),$(call cmd,btf_ko))
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 48585c4d04ad..f2ce411acd59 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -41,9 +41,6 @@ __modpost:
 include include/config/auto.conf
 include $(srctree)/scripts/Kbuild.include
 
-# for mod-prelink-ext
-include $(srctree)/scripts/Makefile.lib
-
 MODPOST = scripts/mod/modpost								\
 	$(if $(CONFIG_MODVERSIONS),-m)							\
 	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)					\
@@ -118,8 +115,6 @@ $(input-symdump):
 	@echo >&2 '         Modules may not have dependencies or modversions.'
 	@echo >&2 '         You may get many unresolved symbol warnings.'
 
-modules := $(sort $(shell cat $(MODORDER)))
-
 # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined symbols
 ifneq ($(KBUILD_MODPOST_WARN)$(filter-out $(existing-input-symdump), $(input-symdump)),)
 MODPOST += -w
@@ -128,9 +123,9 @@ endif
 # Read out modules.order to pass in modpost.
 # Otherwise, allmodconfig would fail with "Argument list too long".
 quiet_cmd_modpost = MODPOST $@
-      cmd_modpost = sed 's/\.ko$$/$(mod-prelink-ext)\.o/' $< | $(MODPOST) -T -
+      cmd_modpost = sed 's/ko$$/o/' $< | $(MODPOST) -T -
 
-$(output-symdump): $(MODORDER) $(input-symdump) $(modules:.ko=$(mod-prelink-ext).o) FORCE
+$(output-symdump): $(MODORDER) $(input-symdump) FORCE
 	$(call if_changed,modpost)
 
 targets += $(output-symdump)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index ef779ada04c6..d8de62506939 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -2081,10 +2081,6 @@ static char *remove_dot(char *s)
 		size_t m = strspn(s + n + 1, "0123456789");
 		if (m && (s[n + m] == '.' || s[n + m] == 0))
 			s[n] = 0;
-
-		/* strip trailing .prelink */
-		if (strends(s, ".prelink"))
-			s[strlen(s) - 8] = '\0';
 	}
 	return s;
 }
@@ -2108,9 +2104,6 @@ static void read_symbols(const char *modname)
 		/* strip trailing .o */
 		tmp = NOFAIL(strdup(modname));
 		tmp[strlen(tmp) - 2] = '\0';
-		/* strip trailing .prelink */
-		if (strends(tmp, ".prelink"))
-			tmp[strlen(tmp) - 8] = '\0';
 		mod = new_module(tmp);
 
 		if (modversions)
-- 
2.32.0

Re: [PATCH 27/27] kbuild: do not create *.prelink.o for Clang LTO or IBT

[Nicolas Schier]

On Mon 25 Apr 2022 04:08:11 GMT Masahiro Yamada wrote:
> When CONFIG_LTO_CLANG=y, additional intermediate *.prelink.o is 
> created
> for each module. Also, objtool is postponed until LLVM bitcode is
> converted to ELF.
> 
> CONFIG_X86_KERNEL_IBT works in a similar way to postpone objtool until
> objects are merged together.
> 
> This commit stops generating *.prelink.o, so the build flow will look
> the same with/without LTO.
> 
> The following figures show how the LTO build currently works, and
> how this commit is changing it.
> 
> Current build flow
> ==================
> 
>  [1] single-object module
> 
>                                     [+objtool]
>            $(CC)                      $(LD)                $(LD)
>     foo.c --------------------> foo.o -----> foo.prelink.o -----> foo.ko
>                            (LLVM bitcode)        (ELF)       |
>                                                              |
>                                                  foo.mod.o --/
> 
>  [2] multi-object module
>                                     [+objtool]
>            $(CC)         $(AR)        $(LD)                $(LD)
>     foo1.c -----> foo1.o -----> foo.o -----> foo.prelink.o -----> foo.ko
>                            |  (archive)          (ELF)       |
>     foo2.c -----> foo2.o --/                                 |
>                 (LLVM bitcode)                   foo.mod.o --/
> 
>   One confusion is foo.o in multi-object module is an archive despite of
>   its suffix.
> 
> New build flow
> ==============
> 
>  [1] single-object module
> 
>   Since there is only one object, we do not need to have the LLVM
>   bitcode stage. Use $(CC)+$(LD) to generate an ELF object in one
>   build rule. Of course, only $(CC) is used when LTO is disabled.
> 
>            $(CC)+$(LD)[+objtool]           $(LD)
>     foo.c ------------------------> foo.o -------> foo.ko
>                                     (ELF)    |
>                                              |
>                                  foo.mod.o --/
> 
>  [2] multi-object module
> 
>   Previously, $(AR) was used to combine LLVM bitcode into an archive,
>   but there was not a technical reason to do so.
>   This commit just uses $(LD) to combine and convert them into a single
>   ELF object.
> 
>                           [+objtool]
>             $(CC)           $(LD)          $(LD)
>     foo1.c -------> foo1.o -------> foo.o -------> foo.ko
>                               |     (ELF)    |
>     foo2.c -------> foo2.o ---/              |
>                 (LLVM bitcode)   foo.mod.o --/
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/Kbuild.include    |  4 +++
>  scripts/Makefile.build    | 58 ++++++++++++---------------------------
>  scripts/Makefile.lib      |  7 -----
>  scripts/Makefile.modfinal |  5 ++--
>  scripts/Makefile.modpost  |  9 ++----
>  scripts/mod/modpost.c     |  7 -----
>  6 files changed, 25 insertions(+), 65 deletions(-)
> 
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index 3514c2149e9d..455a0a6ce12d 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -15,6 +15,10 @@ pound := \#
>  # Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
>  dot-target = $(dir $@).$(notdir $@)
>  
> +###
> +# Name of target with a '.tmp_' as filename prefix. foo/bar.o => foo/.tmp_bar.o
> +tmp-target = $(dir $@).tmp_$(notdir $@)

This matches the dot-target definition above, otherwise $(@D).tmp_$(@F) 
would be an alternative.

> +
>  ###
>  # The temporary file to save gcc -MMD generated dependencies must not
>  # contain a comma
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 7f199b0a5170..fe4d3a908dd0 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -88,10 +88,6 @@ endif
>  targets-for-modules := $(foreach x, o mod $(if $(CONFIG_TRIM_UNUSED_KSYMS), usyms), \
>  				$(patsubst %.o, %.$x, $(filter %.o, $(obj-m))))
>  
> -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> -targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
> -endif
> -
>  ifdef need-modorder
>  targets-for-modules += $(obj)/modules.order
>  endif
> @@ -153,8 +149,16 @@ $(obj)/%.ll: $(src)/%.c FORCE
>  # The C file is compiled and updated dependency information is generated.
>  # (See cmd_cc_o_c + relevant part of rule_cc_o_c)
>  
> +is-single-obj-m = $(if $(part-of-module),$(if $(filter $@, $(obj-m)),y))

Perhaps using $(and ..,..,y) instead if $(if ..,$(if ..,y))?

is-single-obj-m = $(and $(part-of-module),$(filter $@, $(obj-m)),y)

> +
> +ifdef CONFIG_LTO_CLANG
> +cmd_ld_single = $(if $(is-single-obj-m), ; $(LD) $(ld_flags) -r -o $(tmp-target) $@; mv $(tmp-target) $@)
> +endif
> +
>  quiet_cmd_cc_o_c = CC $(quiet_modtag)  $@
> -      cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< $(cmd_objtool)
> +      cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< \
> +		$(cmd_ld_single) \
> +		$(cmd_objtool)
>  
>  ifdef CONFIG_MODVERSIONS
>  # When module versioning is enabled the following steps are executed:
> @@ -228,21 +232,16 @@ cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(o
>  
>  endif # CONFIG_STACK_VALIDATION
>  
> -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> -
> -# Skip objtool for LLVM bitcode
> -$(obj)/%.o: objtool-enabled :=
> -
> -else
>  
>  # '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
>  
> -$(obj)/%.o: objtool-enabled = $(if $(filter-out y%, \
> -	$(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n),y)
> +is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n),y)
>  
> -endif
> +delay-objtool := $(or $(CONFIG_LTO_CLANG),$(CONFIG_X86_KERNEL_IBT))
> +
> +$(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))

same here?  $(and) versus $(if ..,$(if ..,y))

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

>  
>  ifdef CONFIG_TRIM_UNUSED_KSYMS
>  cmd_gen_ksymdeps = \
> @@ -271,24 +270,6 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
>  	$(call if_changed_rule,cc_o_c)
>  	$(call cmd,force_checksrc)
>  
> -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> -# Module .o files may contain LLVM bitcode, compile them into native code
> -# before ELF processing
> -quiet_cmd_cc_prelink_modules = LD [M]  $@
> -      cmd_cc_prelink_modules =						\
> -	$(LD) $(ld_flags) -r -o $@					\
> -		--whole-archive $(filter-out FORCE,$^)			\
> -		$(cmd_objtool)
> -
> -# objtool was skipped for LLVM bitcode, run it now that we have compiled
> -# modules into native code
> -$(obj)/%.prelink.o: objtool-enabled = y
> -$(obj)/%.prelink.o: part-of-module := y
> -
> -$(obj)/%.prelink.o: $(obj)/%.o FORCE
> -	$(call if_changed,cc_prelink_modules)
> -endif
> -
>  cmd_mod = echo $(addprefix $(obj)/, $(call real-search, $*.o, .o, -objs -y -m)) | \
>  	$(AWK) -v RS='( |\n)' '!x[$$0]++' > $@
>  
> @@ -298,7 +279,7 @@ $(obj)/%.mod: FORCE
>  # List module undefined symbols
>  cmd_undefined_syms = $(NM) $< | sed -n 's/^  *U //p' > $@
>  
> -$(obj)/%.usyms: $(obj)/%$(mod-prelink-ext).o FORCE
> +$(obj)/%.usyms: $(obj)/%.o FORCE
>  	$(call if_changed,undefined_syms)
>  
>  quiet_cmd_cc_lst_c = MKLST   $@
> @@ -420,16 +401,11 @@ $(obj)/modules.order: $(obj-m) FORCE
>  $(obj)/lib.a: $(lib-y) FORCE
>  	$(call if_changed,ar)
>  
> -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> -quiet_cmd_link_multi-m = AR [M]  $@
> -cmd_link_multi-m =						\
> -	rm -f $@; 						\
> -	$(AR) cDPrsT $@ @$(patsubst %.o,%.mod,$@)
> -else
>  quiet_cmd_link_multi-m = LD [M]  $@
> -      cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ @$(patsubst %.o,%.mod,$@)
> -endif
> +      cmd_link_multi-m = $(LD) $(ld_flags) -r -o $@ @$(patsubst %.o,%.mod,$@) $(cmd_objtool)
>  
> +$(multi-obj-m): objtool-enabled := $(delay-objtool)
> +$(multi-obj-m): part-of-module := y
>  $(multi-obj-m): %.o: %.mod FORCE
>  	$(call if_changed,link_multi-m)
>  $(call multi_depend, $(multi-obj-m), .o, -objs -y -m)
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 0453a1904646..f75138385449 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -225,13 +225,6 @@ dtc_cpp_flags  = -Wp,-MMD,$(depfile).pre.tmp -nostdinc                    \
>  		 $(addprefix -I,$(DTC_INCLUDE))                          \
>  		 -undef -D__DTS__
>  
> -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> -# With CONFIG_LTO_CLANG, .o files in modules might be LLVM bitcode, so we
> -# need to run LTO to compile them into native code (.lto.o) before further
> -# processing.
> -mod-prelink-ext := .prelink
> -endif
> -
>  # Useful for describing the dependency of composite objects
>  # Usage:
>  #   $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
> diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> index d429e3f9ae1d..51d384a0e4f9 100644
> --- a/scripts/Makefile.modfinal
> +++ b/scripts/Makefile.modfinal
> @@ -9,7 +9,7 @@ __modfinal:
>  include include/config/auto.conf
>  include $(srctree)/scripts/Kbuild.include
>  
> -# for c_flags and mod-prelink-ext
> +# for c_flags
>  include $(srctree)/scripts/Makefile.lib
>  
>  # find all modules listed in modules.order
> @@ -55,9 +55,8 @@ if_changed_except = $(if $(call newer_prereqs_except,$(2))$(cmd-check),      \
>  	$(cmd);                                                              \
>  	printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
>  
> -
>  # Re-generate module BTFs if either module's .ko or vmlinux changed
> -$(modules): %.ko: %$(mod-prelink-ext).o %.mod.o $(if $(CONFIG_MODVERSIONS), %.symver.lds) scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
> +$(modules): %.ko: %.o %.mod.o $(if $(CONFIG_MODVERSIONS), %.symver.lds) scripts/module.lds $(if $(KBUILD_BUILTIN),vmlinux) FORCE
>  	+$(call if_changed_except,ld_ko_o,vmlinux)
>  ifdef CONFIG_DEBUG_INFO_BTF_MODULES
>  	+$(if $(newer-prereqs),$(call cmd,btf_ko))
> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
> index 48585c4d04ad..f2ce411acd59 100644
> --- a/scripts/Makefile.modpost
> +++ b/scripts/Makefile.modpost
> @@ -41,9 +41,6 @@ __modpost:
>  include include/config/auto.conf
>  include $(srctree)/scripts/Kbuild.include
>  
> -# for mod-prelink-ext
> -include $(srctree)/scripts/Makefile.lib
> -
>  MODPOST = scripts/mod/modpost								\
>  	$(if $(CONFIG_MODVERSIONS),-m)							\
>  	$(if $(CONFIG_MODULE_SRCVERSION_ALL),-a)					\
> @@ -118,8 +115,6 @@ $(input-symdump):
>  	@echo >&2 '         Modules may not have dependencies or modversions.'
>  	@echo >&2 '         You may get many unresolved symbol warnings.'
>  
> -modules := $(sort $(shell cat $(MODORDER)))
> -
>  # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined symbols
>  ifneq ($(KBUILD_MODPOST_WARN)$(filter-out $(existing-input-symdump), $(input-symdump)),)
>  MODPOST += -w
> @@ -128,9 +123,9 @@ endif
>  # Read out modules.order to pass in modpost.
>  # Otherwise, allmodconfig would fail with "Argument list too long".
>  quiet_cmd_modpost = MODPOST $@
> -      cmd_modpost = sed 's/\.ko$$/$(mod-prelink-ext)\.o/' $< | $(MODPOST) -T -
> +      cmd_modpost = sed 's/ko$$/o/' $< | $(MODPOST) -T -
>  
> -$(output-symdump): $(MODORDER) $(input-symdump) $(modules:.ko=$(mod-prelink-ext).o) FORCE
> +$(output-symdump): $(MODORDER) $(input-symdump) FORCE
>  	$(call if_changed,modpost)
>  
>  targets += $(output-symdump)
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index ef779ada04c6..d8de62506939 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2081,10 +2081,6 @@ static char *remove_dot(char *s)
>  		size_t m = strspn(s + n + 1, "0123456789");
>  		if (m && (s[n + m] == '.' || s[n + m] == 0))
>  			s[n] = 0;
> -
> -		/* strip trailing .prelink */
> -		if (strends(s, ".prelink"))
> -			s[strlen(s) - 8] = '\0';
>  	}
>  	return s;
>  }
> @@ -2108,9 +2104,6 @@ static void read_symbols(const char *modname)
>  		/* strip trailing .o */
>  		tmp = NOFAIL(strdup(modname));
>  		tmp[strlen(tmp) - 2] = '\0';
> -		/* strip trailing .prelink */
> -		if (strends(tmp, ".prelink"))
> -			tmp[strlen(tmp) - 8] = '\0';
>  		mod = new_module(tmp);
>  
>  		if (modversions)
> -- 
> 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 --

Re: [PATCH 27/27] kbuild: do not create *.prelink.o for Clang LTO or IBT

[Masahiro Yamada]

On Thu, Apr 28, 2022 at 12:31 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Mon 25 Apr 2022 04:08:11 GMT Masahiro Yamada wrote:
> > When CONFIG_LTO_CLANG=y, additional intermediate *.prelink.o is
> > created
> > for each module. Also, objtool is postponed until LLVM bitcode is
> > converted to ELF.
> >
> > CONFIG_X86_KERNEL_IBT works in a similar way to postpone objtool until
> > objects are merged together.
> >
> > This commit stops generating *.prelink.o, so the build flow will look
> > the same with/without LTO.
> >
> > The following figures show how the LTO build currently works, and
> > how this commit is changing it.
> >
> > Current build flow
> > ==================
> >
> >  [1] single-object module
> >
> >                                     [+objtool]
> >            $(CC)                      $(LD)                $(LD)
> >     foo.c --------------------> foo.o -----> foo.prelink.o -----> foo.ko
> >                            (LLVM bitcode)        (ELF)       |
> >                                                              |
> >                                                  foo.mod.o --/
> >
> >  [2] multi-object module
> >                                     [+objtool]
> >            $(CC)         $(AR)        $(LD)                $(LD)
> >     foo1.c -----> foo1.o -----> foo.o -----> foo.prelink.o -----> foo.ko
> >                            |  (archive)          (ELF)       |
> >     foo2.c -----> foo2.o --/                                 |
> >                 (LLVM bitcode)                   foo.mod.o --/
> >
> >   One confusion is foo.o in multi-object module is an archive despite of
> >   its suffix.
> >
> > New build flow
> > ==============
> >
> >  [1] single-object module
> >
> >   Since there is only one object, we do not need to have the LLVM
> >   bitcode stage. Use $(CC)+$(LD) to generate an ELF object in one
> >   build rule. Of course, only $(CC) is used when LTO is disabled.
> >
> >            $(CC)+$(LD)[+objtool]           $(LD)
> >     foo.c ------------------------> foo.o -------> foo.ko
> >                                     (ELF)    |
> >                                              |
> >                                  foo.mod.o --/
> >
> >  [2] multi-object module
> >
> >   Previously, $(AR) was used to combine LLVM bitcode into an archive,
> >   but there was not a technical reason to do so.
> >   This commit just uses $(LD) to combine and convert them into a single
> >   ELF object.
> >
> >                           [+objtool]
> >             $(CC)           $(LD)          $(LD)
> >     foo1.c -------> foo1.o -------> foo.o -------> foo.ko
> >                               |     (ELF)    |
> >     foo2.c -------> foo2.o ---/              |
> >                 (LLVM bitcode)   foo.mod.o --/
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/Kbuild.include    |  4 +++
> >  scripts/Makefile.build    | 58 ++++++++++++---------------------------
> >  scripts/Makefile.lib      |  7 -----
> >  scripts/Makefile.modfinal |  5 ++--
> >  scripts/Makefile.modpost  |  9 ++----
> >  scripts/mod/modpost.c     |  7 -----
> >  6 files changed, 25 insertions(+), 65 deletions(-)
> >
> > diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> > index 3514c2149e9d..455a0a6ce12d 100644
> > --- a/scripts/Kbuild.include
> > +++ b/scripts/Kbuild.include
> > @@ -15,6 +15,10 @@ pound := \#
> >  # Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
> >  dot-target = $(dir $@).$(notdir $@)
> >
> > +###
> > +# Name of target with a '.tmp_' as filename prefix. foo/bar.o => foo/.tmp_bar.o
> > +tmp-target = $(dir $@).tmp_$(notdir $@)
>
> This matches the dot-target definition above, otherwise $(@D).tmp_$(@F)
> would be an alternative.

Yes, I intentionally made dot-target and tmp-target look similar.

The difference is $(dir ...) leaves the trailing slash, but $(@D) does not.

The alternative would be
$(@D)/.tmp_$(@F)





>
> > +
> >  ###
> >  # The temporary file to save gcc -MMD generated dependencies must not
> >  # contain a comma
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index 7f199b0a5170..fe4d3a908dd0 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -88,10 +88,6 @@ endif
> >  targets-for-modules := $(foreach x, o mod $(if $(CONFIG_TRIM_UNUSED_KSYMS), usyms), \
> >                               $(patsubst %.o, %.$x, $(filter %.o, $(obj-m))))
> >
> > -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> > -targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
> > -endif
> > -
> >  ifdef need-modorder
> >  targets-for-modules += $(obj)/modules.order
> >  endif
> > @@ -153,8 +149,16 @@ $(obj)/%.ll: $(src)/%.c FORCE
> >  # The C file is compiled and updated dependency information is generated.
> >  # (See cmd_cc_o_c + relevant part of rule_cc_o_c)
> >
> > +is-single-obj-m = $(if $(part-of-module),$(if $(filter $@, $(obj-m)),y))
>
> Perhaps using $(and ..,..,y) instead if $(if ..,$(if ..,y))?


Good idea!
I did not notice this.  I will do it in v2.



> >
> > -endif
> > +delay-objtool := $(or $(CONFIG_LTO_CLANG),$(CONFIG_X86_KERNEL_IBT))
> > +
> > +$(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
>
> same here?  $(and) versus $(if ..,$(if ..,y))

I am not sure about this case.
The second if-func is  $(if  cond, A, B) form.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 27/27] kbuild: do not create *.prelink.o for Clang LTO or IBT

[Nicolas Schier]

On Thu 28 Apr 2022 13:38:39 GMT Masahiro Yamada wrote:
> 
> On Thu, Apr 28, 2022 at 12:31 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Mon 25 Apr 2022 04:08:11 GMT Masahiro Yamada wrote:
> > > When CONFIG_LTO_CLANG=y, additional intermediate *.prelink.o is
> > > created
> > > for each module. Also, objtool is postponed until LLVM bitcode is
> > > converted to ELF.
> > >
> > > CONFIG_X86_KERNEL_IBT works in a similar way to postpone objtool until
> > > objects are merged together.
> > >
> > > This commit stops generating *.prelink.o, so the build flow will look
> > > the same with/without LTO.
> > >
> > > The following figures show how the LTO build currently works, and
> > > how this commit is changing it.
> > >
> > > Current build flow
> > > ==================
> > >
> > >  [1] single-object module
> > >
> > >                                     [+objtool]
> > >            $(CC)                      $(LD)                $(LD)
> > >     foo.c --------------------> foo.o -----> foo.prelink.o -----> foo.ko
> > >                            (LLVM bitcode)        (ELF)       |
> > >                                                              |
> > >                                                  foo.mod.o --/
> > >
> > >  [2] multi-object module
> > >                                     [+objtool]
> > >            $(CC)         $(AR)        $(LD)                $(LD)
> > >     foo1.c -----> foo1.o -----> foo.o -----> foo.prelink.o -----> foo.ko
> > >                            |  (archive)          (ELF)       |
> > >     foo2.c -----> foo2.o --/                                 |
> > >                 (LLVM bitcode)                   foo.mod.o --/
> > >
> > >   One confusion is foo.o in multi-object module is an archive despite of
> > >   its suffix.
> > >
> > > New build flow
> > > ==============
> > >
> > >  [1] single-object module
> > >
> > >   Since there is only one object, we do not need to have the LLVM
> > >   bitcode stage. Use $(CC)+$(LD) to generate an ELF object in one
> > >   build rule. Of course, only $(CC) is used when LTO is disabled.
> > >
> > >            $(CC)+$(LD)[+objtool]           $(LD)
> > >     foo.c ------------------------> foo.o -------> foo.ko
> > >                                     (ELF)    |
> > >                                              |
> > >                                  foo.mod.o --/
> > >
> > >  [2] multi-object module
> > >
> > >   Previously, $(AR) was used to combine LLVM bitcode into an archive,
> > >   but there was not a technical reason to do so.
> > >   This commit just uses $(LD) to combine and convert them into a single
> > >   ELF object.
> > >
> > >                           [+objtool]
> > >             $(CC)           $(LD)          $(LD)
> > >     foo1.c -------> foo1.o -------> foo.o -------> foo.ko
> > >                               |     (ELF)    |
> > >     foo2.c -------> foo2.o ---/              |
> > >                 (LLVM bitcode)   foo.mod.o --/
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > >  scripts/Kbuild.include    |  4 +++
> > >  scripts/Makefile.build    | 58 ++++++++++++---------------------------
> > >  scripts/Makefile.lib      |  7 -----
> > >  scripts/Makefile.modfinal |  5 ++--
> > >  scripts/Makefile.modpost  |  9 ++----
> > >  scripts/mod/modpost.c     |  7 -----
> > >  6 files changed, 25 insertions(+), 65 deletions(-)
> > >
> > > diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> > > index 3514c2149e9d..455a0a6ce12d 100644
> > > --- a/scripts/Kbuild.include
> > > +++ b/scripts/Kbuild.include
> > > @@ -15,6 +15,10 @@ pound := \#
> > >  # Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
> > >  dot-target = $(dir $@).$(notdir $@)
> > >
> > > +###
> > > +# Name of target with a '.tmp_' as filename prefix. foo/bar.o => foo/.tmp_bar.o
> > > +tmp-target = $(dir $@).tmp_$(notdir $@)
> >
> > This matches the dot-target definition above, otherwise $(@D).tmp_$(@F)
> > would be an alternative.
> 
> Yes, I intentionally made dot-target and tmp-target look similar.
> 
> The difference is $(dir ...) leaves the trailing slash, but $(@D) does not.
> 
> The alternative would be
> $(@D)/.tmp_$(@F)

ah, yes, thanks.

> >
> > > +
> > >  ###
> > >  # The temporary file to save gcc -MMD generated dependencies must not
> > >  # contain a comma
> > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > index 7f199b0a5170..fe4d3a908dd0 100644
> > > --- a/scripts/Makefile.build
> > > +++ b/scripts/Makefile.build
> > > @@ -88,10 +88,6 @@ endif
> > >  targets-for-modules := $(foreach x, o mod $(if $(CONFIG_TRIM_UNUSED_KSYMS), usyms), \
> > >                               $(patsubst %.o, %.$x, $(filter %.o, $(obj-m))))
> > >
> > > -ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
> > > -targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
> > > -endif
> > > -
> > >  ifdef need-modorder
> > >  targets-for-modules += $(obj)/modules.order
> > >  endif
> > > @@ -153,8 +149,16 @@ $(obj)/%.ll: $(src)/%.c FORCE
> > >  # The C file is compiled and updated dependency information is generated.
> > >  # (See cmd_cc_o_c + relevant part of rule_cc_o_c)
> > >
> > > +is-single-obj-m = $(if $(part-of-module),$(if $(filter $@, $(obj-m)),y))
> >
> > Perhaps using $(and ..,..,y) instead if $(if ..,$(if ..,y))?
> 
> 
> Good idea!
> I did not notice this.  I will do it in v2.
> 
> 
> 
> > >
> > > -endif
> > > +delay-objtool := $(or $(CONFIG_LTO_CLANG),$(CONFIG_X86_KERNEL_IBT))
> > > +
> > > +$(obj)/%.o: objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y))
> >
> > same here?  $(and) versus $(if ..,$(if ..,y))
> 
> I am not sure about this case.
> The second if-func is  $(if  cond, A, B) form.

Oh, I didn't see it.  Then it might be the best to keep it the way it 
is.

Re: [PATCH 00/27] kbuild: yet another series of cleanups (modpost and LTO)

[Nicolas Schier]

On Mon, Apr 25, 2022 at 04:07:44AM +0900 Masahiro Yamada wrote:
> This is the third batch of cleanups in this development cycle.
> 
> This weekend, I wrote up the code I have been planning.
> 
> After a bunch of modpost refactoring, I got rid of the ugly code
> in Makefiles.
> 
> With this, Kbuild will get back much simpler and cleaner.
> 

Hi Masahiro,

I tried applying the patch set onto your kbuild and
kbuild-fixes-v5.18, but it didn't apply.  Can you give me
a hint on your commit base?

Kind regards,
Nicolas

> 
> Masahiro Yamada (27):
>   modpost: use snprintf() instead of sprintf() for safety
>   modpost: do not write out any file when error occurred
>   modpost: remove stale comment about sym_add_exported()
>   modpost: add a separate error for exported symbols without definition
>   modpost: retrieve the module dependency and CRCs in check_exports()
>   modpost: use bool type where appropriate
>   modpost: import include/linux/list.h
>   modpost: traverse modules in order
>   modpost: add sym_add_unresolved() helper
>   modpost: traverse unresolved symbols in order
>   modpost: use doubly linked list for dump_lists
>   modpost: move struct namespace_list to modpost.c
>   modpost: traverse the namespace_list in order
>   modpost: dump Module.symvers in the same order of modules.order
>   modpost: move static EXPORT_SYMBOL check to check_exports()
>   modpost: make multiple export error
>   modpost: make sym_add_exported() always allocate a new symbol
>   modpost: make sym_add_exported() a void function
>   modpost: use hlist for hash table implementation
>   modpost: mitigate false-negatives for static EXPORT_SYMBOL checks
>   kbuild: record symbol versions in *.cmd files
>   kbuild: generate a list of objects in vmlinux
>   modpost: retrieve symbol versions by parsing *.cmd files
>   modpost: generate linker script to collect symbol versions
>   kbuild: embed symbol versions at final link of vmlinux or modules
>   kbuild: stop generating *.symversions
>   kbuild: do not create *.prelink.o for Clang LTO or IBT
> 
>  .gitignore                |   1 +
>  Makefile                  |   1 +
>  scripts/Kbuild.include    |   4 +
>  scripts/Makefile.build    | 107 ++------
>  scripts/Makefile.lib      |   7 -
>  scripts/Makefile.modfinal |   6 +-
>  scripts/Makefile.modpost  |   9 +-
>  scripts/link-vmlinux.sh   |  38 ++-
>  scripts/mod/file2alias.c  |   2 -
>  scripts/mod/list.h        | 336 ++++++++++++++++++++++++
>  scripts/mod/modpost.c     | 529 +++++++++++++++++++++++---------------
>  scripts/mod/modpost.h     |  27 +-
>  12 files changed, 731 insertions(+), 336 deletions(-)
>  create mode 100644 scripts/mod/list.h
> 
> -- 
> 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 --

Re: [PATCH 00/27] kbuild: yet another series of cleanups (modpost and LTO)

[Masahiro Yamada]

On Wed, Apr 27, 2022 at 5:11 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
>
> On Mon, Apr 25, 2022 at 04:07:44AM +0900 Masahiro Yamada wrote:
> > This is the third batch of cleanups in this development cycle.
> >
> > This weekend, I wrote up the code I have been planning.
> >
> > After a bunch of modpost refactoring, I got rid of the ugly code
> > in Makefiles.
> >
> > With this, Kbuild will get back much simpler and cleaner.
> >
>
> Hi Masahiro,
>
> I tried applying the patch set onto your kbuild and
> kbuild-fixes-v5.18, but it didn't apply.  Can you give me
> a hint on your commit base?


This series is based on  linux-kbuild/kbuild branch
7c39c50dcb74 ("scripts: dummy-tools, add pahole")


Anyway, this series is too big.
For convenience, I pushed this to a topic branch.

Please try:

git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
 lto-cleanup





> Kind regards,
> Nicolas
>
> >
> > Masahiro Yamada (27):
> >   modpost: use snprintf() instead of sprintf() for safety
> >   modpost: do not write out any file when error occurred
> >   modpost: remove stale comment about sym_add_exported()
> >   modpost: add a separate error for exported symbols without definition
> >   modpost: retrieve the module dependency and CRCs in check_exports()
> >   modpost: use bool type where appropriate
> >   modpost: import include/linux/list.h
> >   modpost: traverse modules in order
> >   modpost: add sym_add_unresolved() helper
> >   modpost: traverse unresolved symbols in order
> >   modpost: use doubly linked list for dump_lists
> >   modpost: move struct namespace_list to modpost.c
> >   modpost: traverse the namespace_list in order
> >   modpost: dump Module.symvers in the same order of modules.order
> >   modpost: move static EXPORT_SYMBOL check to check_exports()
> >   modpost: make multiple export error
> >   modpost: make sym_add_exported() always allocate a new symbol
> >   modpost: make sym_add_exported() a void function
> >   modpost: use hlist for hash table implementation
> >   modpost: mitigate false-negatives for static EXPORT_SYMBOL checks
> >   kbuild: record symbol versions in *.cmd files
> >   kbuild: generate a list of objects in vmlinux
> >   modpost: retrieve symbol versions by parsing *.cmd files
> >   modpost: generate linker script to collect symbol versions
> >   kbuild: embed symbol versions at final link of vmlinux or modules
> >   kbuild: stop generating *.symversions
> >   kbuild: do not create *.prelink.o for Clang LTO or IBT
> >
> >  .gitignore                |   1 +
> >  Makefile                  |   1 +
> >  scripts/Kbuild.include    |   4 +
> >  scripts/Makefile.build    | 107 ++------
> >  scripts/Makefile.lib      |   7 -
> >  scripts/Makefile.modfinal |   6 +-
> >  scripts/Makefile.modpost  |   9 +-
> >  scripts/link-vmlinux.sh   |  38 ++-
> >  scripts/mod/file2alias.c  |   2 -
> >  scripts/mod/list.h        | 336 ++++++++++++++++++++++++
> >  scripts/mod/modpost.c     | 529 +++++++++++++++++++++++---------------
> >  scripts/mod/modpost.h     |  27 +-
> >  12 files changed, 731 insertions(+), 336 deletions(-)
> >  create mode 100644 scripts/mod/list.h
> >
> > --
> > 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

Re: [PATCH 00/27] kbuild: yet another series of cleanups (modpost and LTO)

[Masahiro Yamada]

On Wed, Apr 27, 2022 at 12:18 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Apr 27, 2022 at 5:11 AM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> > On Mon, Apr 25, 2022 at 04:07:44AM +0900 Masahiro Yamada wrote:
> > > This is the third batch of cleanups in this development cycle.
> > >
> > > This weekend, I wrote up the code I have been planning.
> > >
> > > After a bunch of modpost refactoring, I got rid of the ugly code
> > > in Makefiles.
> > >
> > > With this, Kbuild will get back much simpler and cleaner.
> > >
> >
> > Hi Masahiro,
> >
> > I tried applying the patch set onto your kbuild and
> > kbuild-fixes-v5.18, but it didn't apply.  Can you give me
> > a hint on your commit base?
>
>
> This series is based on  linux-kbuild/kbuild branch
> 7c39c50dcb74 ("scripts: dummy-tools, add pahole")
>
>
> Anyway, this series is too big.
> For convenience, I pushed this to a topic branch.
>
> Please try:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
>  lto-cleanup
>
>
>
>
>
> > Kind regards,
> > Nicolas
> >
> > >
> > > Masahiro Yamada (27):
> > >   modpost: use snprintf() instead of sprintf() for safety
> > >   modpost: do not write out any file when error occurred
> > >   modpost: remove stale comment about sym_add_exported()
> > >   modpost: add a separate error for exported symbols without definition
> > >   modpost: retrieve the module dependency and CRCs in check_exports()
> > >   modpost: use bool type where appropriate
> > >   modpost: import include/linux/list.h
> > >   modpost: traverse modules in order
> > >   modpost: add sym_add_unresolved() helper
> > >   modpost: traverse unresolved symbols in order
> > >   modpost: use doubly linked list for dump_lists
> > >   modpost: move struct namespace_list to modpost.c
> > >   modpost: traverse the namespace_list in order
> > >   modpost: dump Module.symvers in the same order of modules.order
> > >   modpost: move static EXPORT_SYMBOL check to check_exports()
> > >   modpost: make multiple export error
> > >   modpost: make sym_add_exported() always allocate a new symbol
> > >   modpost: make sym_add_exported() a void function
> > >   modpost: use hlist for hash table implementation
> > >   modpost: mitigate false-negatives for static EXPORT_SYMBOL checks
> > >   kbuild: record symbol versions in *.cmd files
> > >   kbuild: generate a list of objects in vmlinux
> > >   modpost: retrieve symbol versions by parsing *.cmd files
> > >   modpost: generate linker script to collect symbol versions
> > >   kbuild: embed symbol versions at final link of vmlinux or modules
> > >   kbuild: stop generating *.symversions
> > >   kbuild: do not create *.prelink.o for Clang LTO or IBT

01-05 and 12 applied.

I will send v2 for the rest.




-- 
Best Regards
Masahiro Yamada