Re: powerpc gold work in progress

[Ian Lance Taylor <iant@google.com>]

On Thu, Aug 9, 2012 at 6:27 AM, Alan Modra <amodra@gmail.com> wrote:
> I wrote a lot of this well over a year ago, but
> never found the time to test (or enthusiasm to wrestle with C++ - I'd
> almost be more comfortable if this was fortran).  ;-)

But FORTRAN doesn't have templates.

>         * object.h (Sized_relobj_file::find_shdr): Two off new functions.
>         (Sized_relobj_file::find_special_sections): New function.
>         * object.cc (Sized_relobj_file::find_shdr): Two off new functions.
>         (Sized_relobj_file::find_eh_frame): Use find_shdr.
>         (Sized_relobj_file::find_special_sections): New function, split out..
>         (Sized_relobj_file::do_read_symbols): ..from here.
>         * output.h (Output_data_got::num_entries): New function.
>         (Output_data_got::last_got_offset,set_got_size): Use it.
>         (class Got_entry): No longer private.
>         * powerpc.cc (class Powerpc_relobj): New.
>         (class Powerpc_relocate_functions): Delete all psymval variants or
>         convert to value,addend type.  Delete pcrela, pcrela_unaligned.
>         Implement _ha functions using corresponding _hi function.
>         (Powerpc_relobj::find_special_sections): New function.
>         (Target_powerpc::do_make_elf_object): New function.
>         (class Output_data_got_powerpc): New.
>         (class Output_data_glink): New.
>         (class Powerpc_scan_relocatable_reloc): New.
>         Many more changes througout file.


> --- gold/object.h       26 Apr 2012 00:07:17 -0000      1.117
> +++ gold/object.h       9 Aug 2012 12:12:54 -0000
>    map_to_kept_section(unsigned int shndx, bool* found) const;
>
> +  // Find the section header with the given SH_NAME.
> +  const unsigned char*
> +  find_shdr(const unsigned char* pshdrs, elfcpp::Elf_Word sh_name) const;

I can't see any reason to make this method public.  It should probably
be private.


> +  // Find the section header with the given NAME.  Ignore headers that
> +  // have sh_name less than NAMES + UOFF.  If section header is
> +  // found, updates UOFF so that a subsequent call will find the next
> +  // match.
> +  const unsigned char*
> +  find_shdr(const unsigned char* pshdrs, const char *name,
> +           const char* names, section_size_type names_size,
> +           size_t& uoff) const;

s/const char *name,/const char* name,/

gold doesn't use non-const references as function parameters.  Change
uoff to be size_t*.

gold generally tries to avoid overloading methods that are not
essentially identical, and these two versions of find_shdr are not
essentially identical.  One should be renamed.

I guess I don't understand how this can do what you want, assuming I
understand what you want.  It seems to assume that, if there is more
than one section with the same name, they will have different sh_name
fields.  But that assumption does not seem correct to me.  An ELF file
can certainly have multiple section headers with the same sh_name
value.


> +  // Stash away info for a number of special sections.
> +  // Return true if any of the sections found require local symbols to be read.
> +  virtual bool
> +  find_special_sections(Read_symbols_data* sd);

gold uses a specific pattern for virtual methods, which is intended to
clearly separate the interface of users of the class and the interface
between the class and child classes.  This method should be part of
the public interface of the class.  It should be in the list of
protected methods, and it should start with do_, as in
do_find_special_sections.


> +// Find the section header with the given name.
> +
> +template<int size, bool big_endian>
> +const unsigned char*
> +Sized_relobj_file<size, big_endian>::find_shdr(
> +    const unsigned char* pshdrs,
> +    const char *name,

s/const char */const char* /

> +{
> +  size_t len = strlen(name) + 1;
> +  for (const char *p = names + uoff;
> +       (p = reinterpret_cast<const char*>(memmem(p, names_size - (p - names),
> +                                                name, len))) != NULL;
> +       p += len)

I don't really like putting assignments in a for loop conditional.  I
find it hard to follow.  I would write this as a while loop instead.
But I admit this is pure personal style.


> @@ -520,24 +567,15 @@ Sized_relobj_file<size, big_endian>::fin
>      const char* names,
>      section_size_type names_size) const
>  {
> -  const unsigned int shnum = this->shnum();
> -  const unsigned char* p = pshdrs + This::shdr_size;
> -  for (unsigned int i = 1; i < shnum; ++i, p += This::shdr_size)
> +  const unsigned char* s;
> +  size_t off = 0;
> +
> +  while ((s = this->find_shdr(pshdrs, ".eh_frame",
> +                             names, names_size, off)) != NULL)
>      {
> -      typename This::Shdr shdr(p);
> +      typename This::Shdr shdr(s);
>        if (this->check_eh_frame_flags(&shdr))
> -       {
> -         if (shdr.get_sh_name() >= names_size)
> -           {
> -             this->error(_("bad section name offset for section %u: %lu"),
> -                         i, static_cast<unsigned long>(shdr.get_sh_name()));
> -             continue;
> -           }
> -
> -         const char* name = names + shdr.get_sh_name();
> -         if (strcmp(name, ".eh_frame") == 0)
> -           return true;
> -       }
> +       return true;
>      }
>    return false;
>  }

I'm not sure it's really worth worrying about the possibility of
multiple sections named .eh_frame.  The chances of us handling that
correctly are low.  In the current code the reason that
check_eh_frame_flags is called first is an optimization: it saves the
call to strcmp.  In your code, where you find the section name first,
I think it would be fine to skip the call to check_eh_frame_flags, and
probably just remove that function entirely.

> @@ -2318,7 +2323,6 @@ class Output_data_got : public Output_da
>      void
>      write(unsigned char* pov) const;
>
> -   private:
>      enum
>      {
>        GSYM_CODE = 0x7fffffff,

Please don't just drop the private here.  It's OK to make GSYM_CODE,
etc., public or protected, but the data members should still be
private.

> +protected:
> +  // Post constructor setup.
> +  void
> +  do_setup()
> +  {
> +    // Call parent's setup method.
> +    Sized_relobj_file<size, big_endian>::do_setup();
> +
> +  }

Why override the parent method if all you are going to do is call it?

> +  unsigned int got2_section;

Data members have names ending with an underscore.


I ran out of time to look at all the changes in powerpc.cc, and I
wanted to get this out.  I'll take another look after revisions.

Is it really true that the only way to find the .got2 section in an
input file is by name?

Ian