[v1] modpost: refactor get_secindex()

[PATCH] modpost: refactor get_secindex()

Posted by Masahiro Yamada 3 years, 8 months ago

SPECIAL() is only used in get_secindex(). Squash it.

Make the code more readable with more comments.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 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;
+
+	/*
+	 * Elf{32,64}_Sym::st_shndx is 2 byte. Big section numbers are available
+	 * 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)
+		return index - SHN_HIRESERVE - 1;
+
+	return index;
 }
 
 /* file2alias.c */
-- 
2.34.1

Re: [PATCH] modpost: refactor get_secindex()

Posted by Nick Desaulniers 3 years, 8 months ago

On Tue, Jul 26, 2022 at 11:08 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> SPECIAL() is only used in get_secindex(). Squash it.
>
> Make the code more readable with more comments.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  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

Re: [PATCH] modpost: refactor get_secindex()

Posted by Masahiro Yamada 3 years, 8 months ago

On Wed, Aug 3, 2022 at 2:50 AM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Jul 26, 2022 at 11:08 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > SPECIAL() is only used in get_secindex(). Squash it.
> >
> > Make the code more readable with more comments.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  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