Hi Matt,
On Thu, Apr 23, 2020 at 02:47:34PM -0700, Matt Helsley wrote:
> > +static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
>
> I noticed this returns an unsigned int ...
>
> > + Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void *)ehdr);
> > + unsigned const old_shnum = get_shnum(ehdr, shdr0);
>
> While this is not explicitly called out as an unsigned int. Perhaps we
> could just make this and new_shnum explicit unsigned ints and then...
> > + if (!ehdr->e_shnum || new_shnum >= SHN_LORESERVE) {
> > + ehdr->e_shnum = 0;
> > + shdr0->sh_size = w(new_shnum);
> > + } else
> > + ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum));
>
> If we make the unsigned int change proposed above can we reuse new_shnum
> here like so:
> ehdr->e_shnum = w2(new_shnum);
>
> So this if/else is doing the inverse of get_shnum(). I think the code
> could be cleaned up a little and prepare for moving to objtool by
> putting it in a helper function.
Sure, sounds good to me.
> > + for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
> > + if (relhdr->sh_type == SHT_SYMTAB)
> > + symtab = (void *)ehdr + relhdr->sh_offset;
> > + else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
> > + symtab_shndx = (void *)ehdr + relhdr->sh_offset;
> > +
> > + if (symtab && symtab_shndx)
> > + break;
> > + }
>
> Could you break this out into a helper function? find_symtab() maybe? Again, I think
> that helper would go away with conversion to objtool.
Agreed, this wouldn't be needed with libelf. I'll send v2 shortly.
Thanks for the review!
Sami
