Re: [PATCH v3 5/7] mkimage: refactor a bunch of section data into a struct.

[Daniel Kiper]

On Wed, Feb 21, 2018 at 03:20:27PM -0500, Peter Jones wrote:
> This basically moves a bunch of the section information we pass around a
> lot into a struct, and passes a pointer to a single one of those
> instead.
>
> Because it's more convenient, it also puts the section_vaddresses
> calculations in locate_section(), which no longer returns the allocation
> for section_addresses directly either.
>
> This shouldn't change the binary file output or the "grub-mkimage -v"
> output in any way.
>
> Signed-off-by: Peter Jones <address@hidden>
> ---
>  util/grub-mkimagexx.c | 273 
> ++++++++++++++++++++++----------------------------
>  1 file changed, 121 insertions(+), 152 deletions(-)
>
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 0b7c00e2297..19aa87f7efe 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -93,6 +93,17 @@ struct fixup_block_list
>
>  #define ALIGN_ADDR(x) (ALIGN_UP((x), image_target->voidp_sizeof))
>
> +struct section_metadata
> +{
> +  Elf_Half num_sections;
> +  Elf_Shdr *sections;
> +  Elf_Addr *addrs;
> +  Elf_Addr *vaddrs;
> +  Elf_Half section_entsize;
> +  Elf_Shdr *symtab;
> +  const char *strtab;
> +};
> +
>  static int
>  is_relocatable (const struct grub_install_image_target_desc *image_target)
>  {
> @@ -508,9 +519,7 @@ SUFFIX (grub_mkimage_generate_elf) (const struct 
> grub_install_image_target_desc
>  /* Relocate symbols; note that this function overwrites the symbol table.
>     Return the address of a start symbol.  */
>  static Elf_Addr
> -SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr *sections,
> -                        Elf_Shdr *symtab_section, Elf_Addr 
> *section_vaddresses,
> -                        Elf_Half section_entsize, Elf_Half num_sections,
> +SUFFIX (relocate_symbols) (Elf_Ehdr *e, struct section_metadata *smd,
>                          void *jumpers, Elf_Addr jumpers_addr,
>                          Elf_Addr bss_start, Elf_Addr end,
>                          const struct grub_install_image_target_desc 
> *image_target)
> @@ -520,19 +529,18 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
> *sections,
>    Elf_Addr start_address = (Elf_Addr) -1;
>    Elf_Sym *sym;
>    Elf_Word i;
> -  Elf_Shdr *symtab_link_section;
> +  Elf_Shdr *symtab_section;
>    const char *symtab;
>    grub_uint64_t *jptr = jumpers;
>
> -  symtab_link_section
> -    = (Elf_Shdr *) ((char *) sections
> -                   + (grub_target_to_host32 (symtab_section->sh_link)
> -                      * section_entsize));
> -  symtab = (char *) e + grub_target_to_host (symtab_link_section->sh_offset);
> +  symtab_section = (Elf_Shdr *) ((char *) smd->sections
> +                              + grub_target_to_host32 (smd->symtab->sh_link)
> +                                * smd->section_entsize);
> +  symtab = (char *) e + grub_target_to_host (symtab_section->sh_offset);
>
> -  symtab_size = grub_target_to_host (symtab_section->sh_size);
> -  sym_size = grub_target_to_host (symtab_section->sh_entsize);
> -  symtab_offset = grub_target_to_host (symtab_section->sh_offset);
> +  symtab_size = grub_target_to_host (smd->symtab->sh_size);
> +  sym_size = grub_target_to_host (smd->symtab->sh_entsize);
> +  symtab_offset = grub_target_to_host (smd->symtab->sh_offset);
>    num_syms = symtab_size / sym_size;
>
>    for (i = 0, sym = (Elf_Sym *) ((char *) e + symtab_offset);
> @@ -560,12 +568,12 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
> *sections,
>         else
>           continue;
>       }
> -      else if (cur_index >= num_sections)
> +      else if (cur_index >= smd->num_sections)
>       grub_util_error ("section %d does not exist", cur_index);
>        else
>       {
>         sym->st_value = (grub_target_to_host (sym->st_value)
> -                        + section_vaddresses[cur_index]);
> +                        + smd->vaddrs[cur_index]);
>       }
>
>        if (image_target->elf_target == EM_IA_64 && ELF_ST_TYPE (sym->st_info)
> @@ -580,7 +588,7 @@ SUFFIX (relocate_symbols) (Elf_Ehdr *e, Elf_Shdr 
> *sections,
>        grub_util_info ("locating %s at 0x%"  GRUB_HOST_PRIxLONG_LONG
>                     " (0x%"  GRUB_HOST_PRIxLONG_LONG ")", name,
>                     (unsigned long long) sym->st_value,
> -                   (unsigned long long) section_vaddresses[cur_index]);
> +                   (unsigned long long) smd->vaddrs[cur_index]);
>
>        if (start_address == (Elf_Addr)-1)
>       if (strcmp (name, "_start") == 0 || strcmp (name, "start") == 0)
> @@ -638,9 +646,9 @@ SUFFIX (count_funcs) (Elf_Ehdr *e, Elf_Shdr 
> *symtab_section,
>  #endif
>
>  #ifdef MKIMAGE_ELF32
> -/* Deal with relocation information. This function relocates addresses
> +/* Deal with relocation information. This function relocates addrs

I think that this and comments below are changed by mistake.
If it is true I can fix it during commit.

Otherwise Reviewed-by: Daniel Kiper <address@hidden>

Daniel