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

[Alan Modra <amodra at gmail dot com>]

On Wed, Sep 05, 2012 at 05:31:40PM -0700, Ian Lance Taylor wrote:
> 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.

No, Powerpc_relobj::do_read_relocs calls scan_opd_relocs, so the opd
info is set up for the current object before gc_process_relocs.  (In
fact, it's set up again in Scan_relocs, which is inefficient.)
However, for the reason below..

> > 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.

..I'll need to record all opd refs and process the lot later.  I
thought this was the case from my admittedly shallow understanding of
the gold task queue.

> > +  // 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.

OK.

> >        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?

Huh, so it is.

> > +         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?

That arranges to keep the .opd section.

> > +  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?

I can do it that way.

> > +  // 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.

This might just be me showing my ignorance, but we can have multiple
src_id references to a given dst_off.  If the find is unnecessary,
why the find in gc.h:Garbage_collection::add_reference?

-- 
Alan Modra
Australia Development Lab, IBM