On Wed, Aug 3, 2022 at 2:50 AM Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
>
> On Tue, Jul 26, 2022 at 11:08 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > SPECIAL() is only used in get_secindex(). Squash it.
> >
> > Make the code more readable with more comments.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > ---
> >
> > scripts/mod/modpost.h | 30 ++++++++++++++++++------------
> > 1 file changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index bd874f906781..33b376d9ba71 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -156,22 +156,28 @@ static inline int is_shndx_special(unsigned int i)
> > return i != SHN_XINDEX && i >= SHN_LORESERVE && i <= SHN_HIRESERVE;
> > }
> >
> > -/*
> > - * Move reserved section indices SHN_LORESERVE..SHN_HIRESERVE out of
> > - * the way to -256..-1, to avoid conflicting with real section
> > - * indices.
> > - */
> > -#define SPECIAL(i) ((i) - (SHN_HIRESERVE + 1))
> > -
> > /* Accessor for sym->st_shndx, hides ugliness of "64k sections" */
> > static inline unsigned int get_secindex(const struct elf_info *info,
> > const Elf_Sym *sym)
> > {
> > - if (is_shndx_special(sym->st_shndx))
> > - return SPECIAL(sym->st_shndx);
> > - if (sym->st_shndx != SHN_XINDEX)
> > - return sym->st_shndx;
> > - return info->symtab_shndx_start[sym - info->symtab_start];
> > + unsigned int index = sym->st_shndx;
>
> I think `Elf_Section` would be preferable to `unsigned int` for the
> type of `index`?
But, rather I believe 'unsigned int' is easier to understand
so that you do not need to check the real type of Elf_Section.
Also, it took me a while to convince myself the following still works
as expected.
return index - SHN_HIRESERVE - 1;
('index' will be extended from 'unsigned short' to 'int' before
the substracton, so it will work)
> > +
> > + /*
> > + * Elf{32,64}_Sym::st_shndx is 2 byte. Big section numbers are available
>
> Then I'd update the comment, too, to mention `Elf_Section` rather than
> `Elf{32,64}_Sym::st_shndx`.
>
> > + * in the .symtab_shndx section.
> > + */
> > + if (index == SHN_XINDEX)
> > + return info->symtab_shndx_start[sym - info->symtab_start];
> > +
> > + /*
> > + * Move reserved section indices SHN_LORESERVE..SHN_HIRESERVE out of
> > + * the way to UINT_MAX-255..UINT_MAX, to avoid conflicting with real
> > + * section indices.
> > + */
> > + if (index >= SHN_LORESERVE)
>
> ^ should this also check that `index <= SHN_HIRESERVE`?
Probably, yes.
I will fix it in v2.
> Perhaps just
> call is_shndx_special() like the code did before?
According to my refactoring plan,
all the call-sites of is_shnx_special() will be gone in the future,
and is_shndx_special() will be removed as well.
So, I am not using it here.
>
> Or SHN_HIRESERVE is #defined in include/uapi/linux/elf.h to 0xffff and
> SHN_XINDEX is ... not defined in kernel sources (what?! perhaps
> <elf.h>?)...but should have the same value of 0xffff according to
> https://docs.oracle.com/cd/E19683-01/817-3677/chapter6-94076/index.html
>
> I guess this is fine then, but I would prefer not open coding types
> when dealing with ELF. (i.e. my first suggestion in this thread).
>
> > + return index - SHN_HIRESERVE - 1;
> > +
> > + return index;
> > }
> >
> > /* file2alias.c */
> > --
> > 2.34.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Best Regards
Masahiro Yamada
