Re: [GOLD] powerpc64 --gc-sections RFC

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

On Wed, Sep 5, 2012 at 7:01 AM, Alan Modra <amodra@gmail.com> wrote:
>
> I took the second approach.  A reloc resolving to .opd adds a
> reference from the source section to .opd, *and* to the code section
> given by the appropriate .opd reloc.  The process is complicated by
> .opd relocs not being available if the reference is to another object
> that hasn't yet had its relocs read.  In that case I queue up the
> reference in a container on the destination object, for processing
> along with that object's gc_process_relocs.  I'm not sure whether this
> is a good design.  It appears to me that gold currently serializes
> gc_process_relocs, so we only have one thread fiddling with
> symtab->gc_ and hence we should also only have one thread poking at
> ppc_object->access_from_map_.  Correct?

Correct.  But it makes me a little uncomfortable to see different code
paths based on whether we've already read data for the object.  For
that matter, as far as I can see we never will have read the opd
section at gc_relocs time.  It looks like the opd section is read in
Scan_relocs, but Gc_process_relocs is run before Scan_relocs.  I
think.  So I think you may as well simply record the opd references,
rather than try to optimize.

> Another issue is whether
> do_read_relocs can be running for one object in parallel with
> gc_process_relocs for a different object.  If so, I need to make
> have_opd() thread safe..

Yes, this can happen.  There is only one gc_process_relocs routine
running at a time, but gc_process_relocs routines can overlap with
read_relocs routines.




> +  // Given a reference from SRC_OBJ, SRC_INDX to a location specified
> +  // by DST_OBJ, DST_INDX and DST_OFF, return the true destination
> +  // section that should be marked during --gc-sections processing.
> +
> +  virtual unsigned int
> +  dest_shndx(Object* /* src_obj */,
> +            unsigned int /* src_indx */,
> +            Object* /* dst_obj */,
> +            unsigned int dst_indx,
> +            typename elfcpp::Elf_types<size>::Elf_Addr /* dst_off */)
> +  { return dst_indx; }

This should use the do_dest_shndx approach as in other virtual
functions.  Also dest_shndx isn't the best name, perhaps something
like gc_ref_shndx.



>        if (parameters->options().gc_sections())
>          {
> -         symtab->gc()->add_reference(src_obj, src_indx, dst_obj, dst_indx);
> +         dst_off += Reloc_types<sh_type, size, big_endian>::
> +           get_reloc_addend_noerror(&reloc);

Isn't the result of get_reloc_addend_noerror already in the local
variable addend?


> +         unsigned int code_indx = parameters->sized_target<size, big_endian>()
> +           ->dest_shndx(src_obj, src_indx, dst_obj, dst_indx, dst_off);
> +
> +         symtab->gc()->add_reference(src_obj, src_indx, dst_obj, code_indx);
> +         if (code_indx != dst_indx)
> +           symtab->gc()->add_reference(src_obj, src_indx, dst_obj, dst_indx);

Why do you need this last add_reference?


> +  gc_mark_symbol(Symbol* sym)
> +  {
> +    this->default_gc_mark_symbol(sym);
> +    parameters->target().gc_mark_symbol(this, sym);
> +  }

Why not just call the target gc_mark_symbol from the existing
gc_mark_symbol?  Why introduce the new function and rename the
existing one?

> +  // Add a reference from SRC_OBJ, SRC_INDX to this object's .opd
> +  // section at DST_OFF.
> +  void
> +  add_reference(Object* src_obj,
> +               unsigned int src_indx,
> +               typename elfcpp::Elf_types<size>::Elf_Addr dst_off)
> +  {
> +    // FIXME: Thread safety?  Can multiple threads attempt to update
> +    // access_from_map_?
> +    Section_id src_id(src_obj, src_indx);
> +    typename Access_from::iterator p = this->access_from_map_.find(dst_off);
> +    if (p == this->access_from_map_.end())
> +      this->access_from_map_[dst_off].insert(src_id);
> +    else
> +      p->second.insert(src_id);
> +  }

Seems like this is always
this->access_from_map_[dst_off].insert(src_id).  I'm not sure why you
need the find.

Ian