| Message ID | 20220424190811.1678416-10-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: yet another series of cleanups (modpost and LTO) | expand |
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 >
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
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>
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)
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(-)