Re: [PATCH 6/8] abg-dwarf-reader: migrate more ELF helpers to elf-helpers

[Giuliano Procida <gprocida@google.com>]

Hi.

On Mon, 20 Apr 2020 at 12:09, Matthias Maennich <maennich@google.com> wrote:
>
> This change migrates all ELF helpers related to section lookup to
> abg-elf-helpers.{cc,h}. It also homogenizes the interface of those to
> always return Elf_Scn* and NULL in case that section can't be found.
> Though this smells like a functional change, this latter change is
> purely cosmetic.
>
>         * src/abg-dwarf-reader.cc (read_context::find_symbol_table_section):
>         adjust to new interface of elf_helpers::find_symbol_table_section.
>         (find_opd_section): use elf_helpers::find_opd_section for lookup.
>         (find_ksymtab_section): use elf_helpers::find_ksymtab_section.
>         (find_ksymtab_gpl_section): use elf_helpers::find_ksymtab_gpl_section.
>         (find_relocation_section): Move out function.
>         (get_binary_load_address): Move out function.
>         (find_ksymtab_reloc_section): use elf_helpers::find_relocation_section
>         (find_ksymtab_gpl_reloc_section): use elf_helpers::find_relocation_section
>         * src/elf-helpers.cc (find_symbol_table_section): change
>         interface to match other find_*_section functions.
>         (find_symbol_table_section_index): Adjust for the new interface
>         of find_symbol_table_section.
>         (find_opd_section): New function.
>         (find_ksymtab_section): New function.
>         (find_ksymtab_gpl_section): New function.
>         (find_relocation_section): New function.
>         (get_binary_load_address): New function.
>         * src/elf-helpers.h (find_symbol_table_section): Change declaration.
>         (find_opd_section): New function declation.
>         (find_ksymtab_section): New function declation.
>         (find_ksymtab_gpl_section): New function declation.
>         (find_relocation_section): New function declation.
>         (get_binary_load_address): New function declation.
>
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  src/abg-dwarf-reader.cc | 112 ++++++----------------------------
>  src/abg-elf-helpers.cc  | 129 +++++++++++++++++++++++++++++++++++++---
>  src/abg-elf-helpers.h   |  22 ++++++-
>  3 files changed, 157 insertions(+), 106 deletions(-)
>
> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> index ec1f9f3fe8f3..56da03a60940 100644
> --- a/src/abg-dwarf-reader.cc
> +++ b/src/abg-dwarf-reader.cc
> @@ -542,53 +542,6 @@ compare_dies(const read_context& ctxt,
>              bool update_canonical_dies_on_the_fly);
>
>
> -/// Get the address at which a given binary is loaded in memory⋅
> -///
> -/// @param elf_handle the elf handle for the binary to consider.
> -///
> -/// @param load_address the address where the binary is loaded.  This
> -/// is set by the function iff it returns true.
> -///
> -/// @return true if the function could get the binary load address
> -/// and assign @p load_address to it.
> -static bool
> -get_binary_load_address(Elf *elf_handle,
> -                       GElf_Addr &load_address)
> -{
> -  GElf_Ehdr eh_mem;
> -  GElf_Ehdr *elf_header = gelf_getehdr(elf_handle, &eh_mem);
> -  size_t num_segments = elf_header->e_phnum;
> -  GElf_Phdr *program_header = 0;
> -  GElf_Addr result;
> -  bool found_loaded_segment = false;
> -  GElf_Phdr ph_mem;
> -
> -  for (unsigned i = 0; i < num_segments; ++i)
> -    {
> -      program_header = gelf_getphdr(elf_handle, i, &ph_mem);
> -      if (program_header && program_header->p_type == PT_LOAD)
> -       {
> -         if (!found_loaded_segment)
> -           {
> -             result = program_header->p_vaddr;
> -             found_loaded_segment = true;
> -           }
> -
> -         if (program_header->p_vaddr < result)
> -           // The resulting load address we want is the lowest
> -           // load address of all the loaded segments.
> -           result = program_header->p_vaddr;
> -       }
> -    }
> -
> -  if (found_loaded_segment)
> -    {
> -      load_address = result;
> -      return true;
> -    }
> -  return false;
> -}
> -
>  /// Find the file name of the alternate debug info file.
>  ///
>  /// @param elf_module the elf module to consider.
> @@ -5282,8 +5235,8 @@ public:
>    find_symbol_table_section() const
>    {
>      if (!symtab_section_)
> -      dwarf_reader::find_symbol_table_section(elf_handle(),
> -                                             const_cast<read_context*>(this)->symtab_section_);
> +      const_cast<read_context*>(this)->symtab_section_ =
> +         elf_helpers::find_symbol_table_section(elf_handle());
>      return symtab_section_;
>    }
>
> @@ -5296,8 +5249,8 @@ public:
>    find_opd_section() const
>    {
>      if (!opd_section_)
> -      const_cast<read_context*>(this)->opd_section_=
> -       find_section(elf_handle(), ".opd", SHT_PROGBITS);
> +      const_cast<read_context*>(this)->opd_section_ =
> +         elf_helpers::find_opd_section(elf_handle());
>      return opd_section_;
>    }
>
> @@ -5310,39 +5263,21 @@ public:
>    {
>      if (!ksymtab_section_)
>        const_cast<read_context*>(this)->ksymtab_section_ =
> -       find_section(elf_handle(), "__ksymtab", SHT_PROGBITS);
> +         elf_helpers::find_ksymtab_section(elf_handle());
>      return ksymtab_section_;
>    }
>
> -  /// Return the .rel{a,} section corresponding to a given section.
> -  ///
> -  /// @param target_section the section to search the relocation section for
> +  /// Return the __ksymtab_gpl section of a linux kernel ELF file
> +  /// (either a vmlinux binary or a kernel module).
>    ///
> -  /// @return the .rel{a,} section if found, null otherwise.
> +  /// @return the __ksymtab_gpl section if found, nil otherwise.
>    Elf_Scn*
> -  find_relocation_section(Elf_Scn* target_section) const
> +  find_ksymtab_gpl_section() const
>    {
> -    if (target_section)
> -      {
> -       // the relo section we are searching for has this index as sh_info
> -       size_t target_index = elf_ndxscn(target_section);
> -
> -       // now iterate over all the sections, look for relocation sections and
> -       // find the one that points to the section we are searching for
> -       Elf_Scn*  section = 0;
> -       GElf_Shdr header_mem, *header;
> -       while ((section = elf_nextscn(elf_handle(), section)) != 0)
> -         {
> -           header = gelf_getshdr(section, &header_mem);
> -           if (header == NULL
> -               || (header->sh_type != SHT_RELA && header->sh_type != SHT_REL))
> -             continue;
> -
> -           if (header->sh_info == target_index)
> -             return section;
> -         }
> -      }
> -    return NULL;
> +    if (!ksymtab_gpl_section_)
> +      const_cast<read_context*>(this)->ksymtab_gpl_section_ =
> +         elf_helpers::find_ksymtab_gpl_section(elf_handle());
> +    return ksymtab_gpl_section_;
>    }
>
>    /// Return the .rel{a,}__ksymtab section of a linux kernel ELF file (either
> @@ -5354,25 +5289,12 @@ public:
>    {
>      if (!ksymtab_reloc_section_)
>        {
> -       const_cast<read_context*>(this)->ksymtab_reloc_section_
> -           = find_relocation_section(find_ksymtab_section());
> +       const_cast<read_context*>(this)->ksymtab_reloc_section_ =
> +           find_relocation_section(elf_handle(), find_ksymtab_section());
>        }
>      return ksymtab_reloc_section_;
>    }
>
> -  /// Return the __ksymtab_gpl section of a linux kernel ELF file
> -  /// (either a vmlinux binary or a kernel module).
> -  ///
> -  /// @return the __ksymtab_gpl section if found, nil otherwise.
> -  Elf_Scn*
> -  find_ksymtab_gpl_section() const
> -  {
> -    if (!ksymtab_gpl_section_)
> -      const_cast<read_context*>(this)->ksymtab_gpl_section_ =
> -       find_section(elf_handle(), "__ksymtab_gpl", SHT_PROGBITS);
> -    return ksymtab_gpl_section_;
> -  }
> -
>    /// Return the .rel{a,}__ksymtab_gpl section of a linux kernel ELF file
>    /// (either a vmlinux binary or a kernel module).
>    ///
> @@ -5382,8 +5304,8 @@ public:
>    {
>      if (!ksymtab_gpl_reloc_section_)
>        {
> -       const_cast<read_context*>(this)->ksymtab_gpl_reloc_section_
> -           = find_relocation_section(find_ksymtab_gpl_section());
> +       const_cast<read_context*>(this)->ksymtab_gpl_reloc_section_ =
> +           find_relocation_section(elf_handle(), find_ksymtab_gpl_section());
>        }
>      return ksymtab_gpl_reloc_section_;
>    }
> diff --git a/src/abg-elf-helpers.cc b/src/abg-elf-helpers.cc
> index ede191014369..b77440206fb0 100644
> --- a/src/abg-elf-helpers.cc
> +++ b/src/abg-elf-helpers.cc
> @@ -357,9 +357,9 @@ find_section(Elf* elf_handle, const std::string& name, Elf64_Word section_type)
>  ///
>  /// @param symtab the symbol table found.
>  ///
> -/// @return true iff the symbol table is found.
> -bool
> -find_symbol_table_section(Elf* elf_handle, Elf_Scn*& symtab)
> +/// @return the symbol table section
> +Elf_Scn*
> +find_symbol_table_section(Elf* elf_handle)
>  {
>    Elf_Scn* section = 0, *dynsym = 0, *sym_tab = 0;
>    while ((section = elf_nextscn(elf_handle, section)) != 0)
> @@ -378,12 +378,11 @@ find_symbol_table_section(Elf* elf_handle, Elf_Scn*& symtab)
>        GElf_Ehdr* elf_header = gelf_getehdr(elf_handle, &eh_mem);
>        if (elf_header->e_type == ET_REL
>           || elf_header->e_type == ET_EXEC)
> -       symtab = sym_tab ? sym_tab : dynsym;
> +       return sym_tab ? sym_tab : dynsym;
>        else
> -       symtab = dynsym ? dynsym : sym_tab;
> -      return true;
> +       return dynsym ? dynsym : sym_tab;
>      }
> -  return false;
> +  return NULL;
>  }
>
>  /// Find the index (in the section headers table) of the symbol table
> @@ -402,8 +401,9 @@ find_symbol_table_section(Elf* elf_handle, Elf_Scn*& symtab)
>  bool
>  find_symbol_table_section_index(Elf* elf_handle, size_t& symtab_index)
>  {
> -  Elf_Scn* section = 0;
> -  if (!find_symbol_table_section(elf_handle, section))
> +  Elf_Scn* section = find_symbol_table_section(elf_handle);
> +
> +  if (!section)
>      return false;
>
>    symtab_index = elf_ndxscn(section);
> @@ -500,6 +500,17 @@ Elf_Scn*
>  find_data1_section(Elf* elf_handle)
>  {return find_section(elf_handle, ".data1", SHT_PROGBITS);}
>
> +/// Return the "Official Procedure descriptors section."  This
> +/// section is named .opd, and is usually present only on PPC64
> +/// ELFv1 binaries.
> +///
> +/// @param elf_handle the elf handle to consider.
> +///
> +/// @return the .opd section, if found.  Return nil otherwise.
> +Elf_Scn*
> +find_opd_section(Elf* elf_handle)
> +{return find_section(elf_handle, ".opd", SHT_PROGBITS);}
> +
>  /// Return the SHT_GNU_versym, SHT_GNU_verdef and SHT_GNU_verneed
>  /// sections that are involved in symbol versionning.
>  ///
> @@ -548,6 +559,26 @@ get_symbol_versionning_sections(Elf*               elf_handle,
>    return false;
>  }
>
> +/// Return the __ksymtab section of a linux kernel ELF file (either
> +/// a vmlinux binary or a kernel module).
> +///
> +/// @param elf_handle the elf handle to consider.
> +///
> +/// @return the __ksymtab section if found, nil otherwise.
> +Elf_Scn*
> +find_ksymtab_section(Elf* elf_handle)
> +{return find_section(elf_handle, "__ksymtab", SHT_PROGBITS);}
> +
> +/// Return the __ksymtab_gpl section of a linux kernel ELF file (either
> +/// a vmlinux binary or a kernel module).
> +///
> +/// @param elf_handle the elf handle to consider.
> +///
> +/// @return the __ksymtab section if found, nil otherwise.
> +Elf_Scn*
> +find_ksymtab_gpl_section(Elf* elf_handle)
> +{return find_section(elf_handle, "__ksymtab_gpl", SHT_PROGBITS);}
> +
>  /// Find the __ksymtab_strings section of a Linux kernel binary.
>  ///
>  /// @param elf_handle the elf handle to use.
> @@ -563,6 +594,39 @@ find_ksymtab_strings_section(Elf *elf_handle)
>    return 0;
>  }
>
> +/// Return the .rel{a,} section corresponding to a given section.
> +///
> +/// @param elf_handle the elf handle to consider.
> +///
> +/// @param target_section the section to search the relocation section for
> +///
> +/// @return the .rel{a,} section if found, null otherwise.
> +Elf_Scn*
> +find_relocation_section(Elf* elf_handle, Elf_Scn* target_section)
> +{
> +  if (target_section)
> +    {
> +      // the relo section we are searching for has this index as sh_info
> +      size_t target_index = elf_ndxscn(target_section);
> +
> +      // now iterate over all the sections, look for relocation sections and
> +      // find the one that points to the section we are searching for
> +      Elf_Scn* section = 0;
> +      GElf_Shdr header_mem, *header;
> +      while ((section = elf_nextscn(elf_handle, section)) != 0)
> +       {
> +         header = gelf_getshdr(section, &header_mem);
> +         if (header == NULL
> +             || (header->sh_type != SHT_RELA && header->sh_type != SHT_REL))
> +           continue;
> +
> +         if (header->sh_info == target_index)
> +           return section;
> +       }
> +    }
> +  return NULL;
> +}
> +
>  /// Get the version definition (from the SHT_GNU_verdef section) of a
>  /// given symbol represented by a pointer to GElf_Versym.
>  ///
> @@ -797,5 +861,52 @@ is_linux_kernel(Elf *elf_handle)
>           || is_linux_kernel_module(elf_handle));
>  }
>
> +/// Get the address at which a given binary is loaded in memory⋅
> +///
> +/// @param elf_handle the elf handle for the binary to consider.
> +///
> +/// @param load_address the address where the binary is loaded.  This
> +/// is set by the function iff it returns true.
> +///
> +/// @return true if the function could get the binary load address
> +/// and assign @p load_address to it.

Should be returning nullable load_address.


> +bool
> +get_binary_load_address(Elf* elf_handle, GElf_Addr& load_address)
> +{
> +  GElf_Ehdr elf_header;
> +  gelf_getehdr(elf_handle, &elf_header);
> +  size_t num_segments = elf_header.e_phnum;
> +  GElf_Phdr *program_header = NULL;
> +  GElf_Addr result;
> +  bool found_loaded_segment = false;
> +  GElf_Phdr ph_mem;
> +
> +  for (unsigned i = 0; i < num_segments; ++i)
> +    {
> +      program_header = gelf_getphdr(elf_handle, i, &ph_mem);
> +      if (program_header && program_header->p_type == PT_LOAD)
> +       {
> +         if (!found_loaded_segment)
> +           {
> +             result = program_header->p_vaddr;
> +             found_loaded_segment = true;
> +           }
> +
> +         if (program_header->p_vaddr < result)
> +           // The resulting load address we want is the lowest
> +           // load address of all the loaded segments.
> +           result = program_header->p_vaddr;
> +       }
> +    }
> +
> +  if (found_loaded_segment)
> +    {
> +      load_address = result;
> +      return true;
> +    }
> +  return false;
> +}
> +
> +
>  } // end namespace elf_helpers
>  } // end namespace abigail
> diff --git a/src/abg-elf-helpers.h b/src/abg-elf-helpers.h
> index 7ddd887de959..8a83bb4f2e95 100644
> --- a/src/abg-elf-helpers.h
> +++ b/src/abg-elf-helpers.h
> @@ -63,8 +63,8 @@ find_section(Elf*             elf_handle,
>              const std::string& name,
>              Elf64_Word         section_type);
>
> -bool
> -find_symbol_table_section(Elf* elf_handle, Elf_Scn*& symtab);
> +Elf_Scn*
> +find_symbol_table_section(Elf* elf_handle);
>
>  bool
>  find_symbol_table_section_index(Elf* elf_handle, size_t& symtab_index);
> @@ -96,15 +96,27 @@ find_data_section(Elf* elf_handle);
>  Elf_Scn*
>  find_data1_section(Elf* elf_handle);
>
> +Elf_Scn*
> +find_opd_section(Elf* elf_handle);
> +
>  bool
>  get_symbol_versionning_sections(Elf*           elf_handle,
>                                 Elf_Scn*&       versym_section,
>                                 Elf_Scn*&       verdef_section,
>                                 Elf_Scn*&       verneed_section);
>
> +Elf_Scn*
> +find_ksymtab_section(Elf* elf_handle);
> +
> +Elf_Scn*
> +find_ksymtab_gpl_section(Elf* elf_handle);
> +
>  Elf_Scn*
>  find_ksymtab_strings_section(Elf *elf_handle);
>
> +Elf_Scn*
> +find_relocation_section(Elf* elf_handle, Elf_Scn* target_section);
> +
>  //
>  // Helpers for symbol versioning
>  //
> @@ -137,6 +149,12 @@ is_linux_kernel_module(Elf *elf_handle);
>  bool
>  is_linux_kernel(Elf *elf_handle);
>
> +//
> +// Misc Helpers
> +//
> +
> +bool
> +get_binary_load_address(Elf* elf_handle, GElf_Addr& load_address);
>
>  } // end namespace elf_helpers
>  } // end namespace abigail
> --
> 2.26.1.301.g55bc3eb7cb9-goog
>