Re: [PATCH] modpost: refactor get_secindex()

[Nick Desaulniers]

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`?

> +
> +       /*
> +        * 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`?  Perhaps just
call is_shndx_special() like the code did before?

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