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

[Nick Desaulniers]

On Sun, Apr 24, 2022 at 12:09 PM Masahiro Yamada <masahiroy@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>
>  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