Re: [GOLD] advice on powerpc64 .opd handling

[Ian Lance Taylor <iant at google dot com>]

On Thu, Aug 16, 2012 at 6:43 AM, Alan Modra <amodra@gmail.com> wrote:
>
>         * powerpc.cc: Formatting and white space.
>         (Powerpc_relobj): Rename got2_section_ to special_.
>         Add opd_ent_count_, opd_ent_shndx_, opd_ent_off_.
>         (Powerpc_relobj::opd_shndx, init_opd, get_opd_ent, set_opd_ent,
>         do_read_relocs, opd_ent_ndx): New functions.
>         (Target_powerpc::is_branch_reloc): New function.
>         (Powerpc_relobj::do_find_special_sections): Find .opd for 64-bit.
>         (ld_2_1, cror_15_15_15, cror_31_31_31): New insn constants.
>         (Output_data_glink): Rename pltresolve_size to pltresolve_size_.
>         (Output_data_glink::pltresolve_size): New function.
>         (Output_data_glink::do_write): Correct toc base.  Don't try to use
>         uint16_t for 24-bit offset.
>         (Target_powerpc::Scan::local): Handle more relocs.
>         (Target_powerpc::do_finalize_sections): Set up DT_PPC64_GLINK.
>         (Target_powerpc::Relocate::relocate): Correct toc base calculation.
>         Plug in toc restoring insn after plt calls.  Translate branches
>         to function descriptor symbols to corresponding entry point.
>         * symtab.h: Comment typo.
>
>    Powerpc_relobj(const std::string& name, Input_file* input_file, off_t offset,
>                  const typename elfcpp::Ehdr<size, big_endian>& ehdr)
>      : Sized_relobj_file<size, big_endian>(name, input_file, offset, ehdr),
> -      got2_section_(0)
> +      special_(0)
>    { }

You should initialize all the new fields in the constructor, even if
you just initialize them to 0.


> +  unsigned int special_;
> +  size_t opd_ent_count_;
> +  unsigned int* opd_ent_shndx_;
> +  typename elfcpp::Elf_types<size>::Elf_Off* opd_ent_off_;

Please add comments for each describing the values they hold,
especially for special_.

Normally a C array should be std::vector in C++.  This should probably be

  std::vector<unsigned int> ent_shndx_;
  std::vector<typename elfcpp::Elf_Types<size>::Elf_Off> opd_ent_off_;

You may want to add a typedef for Elf_Off in the class, e.g., Address
in Sized_relobj in object.h.


> +         unsigned int r_type = elfcpp::elf_r_type<size>(r_info);
> +         if (r_type == elfcpp::R_PPC64_ADDR64)
> +           {
> +             unsigned int r_sym = elfcpp::elf_r_sym<size>(r_info);
> +             typename elfcpp::Elf_types<size>::Elf_Addr value;
> +             bool is_ordinary;
> +             unsigned int shndx;
> +             if (r_sym < this->local_symbol_count())
> +               {
> +                 typename elfcpp::Sym<size, big_endian>
> +                   lsym(plocal_syms + r_sym * sym_size);
> +                 shndx = lsym.get_st_shndx();
> +                 shndx = this->adjust_sym_shndx(r_sym, shndx, &is_ordinary);
> +                 value = lsym.get_st_value();
> +               }
> +             else
> +               shndx = this->symbol_section_and_value(r_sym, &value,
> +                                                      &is_ordinary);

This isn't necessarily something to worry about right now, but
symbol_section_and_value is kind of a slow function.  If there are a
lot of global symbols in .opd sections, it will be better to get a
view of the global symbols once.

> +  if (size == 64)
> +    {
> +      for (Read_relocs_data::Relocs_list::iterator p = rd->relocs.begin();
> +          p != rd->relocs.end();
> +          ++p)
> +       if (p->data_shndx == this->opd_shndx())
> +         {
> +           this->init_opd(this->section_size(this->opd_shndx()));
> +           this->scan_opd_relocs(p->reloc_count, p->contents->data(),
> +                                 rd->local_symbols->data());
> +           break;
> +         }
> +    }

When there is more than a single line in a for body, I prefer to see
curly braces.


>   private:
> -  static const int pltresolve_size = 16*4;
> +  static const int pltresolve_size_ = 16*4;

Although data member names always end in underscore, it's not
necessary for the name of a constant to end in underscore.

> +         if (!can_plt_call)
> +           {
> +             if (parameters->options().output_is_executable())

What should happen for a shared library here?  Should this be
!relocatable() instead?

> +         if (value >= opd_addr
> +             && value < opd_addr + symobj->section_size(shndx))
> +           {
> +             symobj->get_opd_ent(value - opd_addr, &shndx, &value);
> +             value += (symobj->output_section(shndx)->address()
> +                       + symobj->output_section_offset(shndx));

Should get_opd_ent double-check that it is not returning an undefined symbol?

Will this work correctly if the symbol is defined in a SHT_GROUP
section that is discarded?  Code like this usually calls
get_output_section_offset, not output_section_offset, and checks for a
return == invalid_address.  In that case it needs to call
output_section->output_address.  See examples in output.cc.

Ian