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
