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

[Masahiro Yamada]

On Tue, Apr 26, 2022 at 3:41 AM Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
>
> 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`?


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

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



-- 
Best Regards
Masahiro Yamada