Re: [PATCH] modpost: refactor get_secindex()

[Masahiro Yamada]

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