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
