Re: Another MIPS multigot patch

[Daniel Jacobowitz <drow at mvista dot com>]

On Thu, Jan 29, 2004 at 03:55:45PM +0000, Richard Sandiford wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> > On Sat, Nov 22, 2003 at 08:24:06PM -0500, Daniel Jacobowitz wrote:
> >> On Sat, Nov 22, 2003 at 07:14:06PM +0000, Richard Sandiford wrote:
> >> >     (a) _bfd_mips_elf_check_relocs assumes print-rtl.c can use a local
> >> >         page entry for GOT_PAGE/GOT_OFST accesses to flag_dump_unnumbered.
> >> >         f_d_u is therefore not entered into print-rtl's got_entries.
> >> > 
> >> >     (b) toplev uses GOT_DISP, so we end up needing a global GOT
> >> >         entry for f_d_u.
> >> 
> >> 000000001028  00a900000012 R_MIPS_64         0000000000000000 flag_dump_unnumbered + 0
> >>                     Type2: R_MIPS_NONE      
> >>                     Type3: R_MIPS_NONE      
> >> 
> >> So, close enough for our purposes.
> >> 
> >> >     (c) Because of (a), no bfd that uses the primary GOT seems to need
> >> >         an entry for f_d_u.  We therefore give it a higher index than
> >> >         symbols that _do_ need an entry.
> >> > 
> >> >     (d) Because of (c), mips_elf_calculate_relocation assumes that f_d_u
> >> >         is !local_p, and it therefore decays the GOT_PAGE reloc into a
> >> >         GOT_DISP.  So print-rtl uses f_d_u's GOT entry after all.
> >> > 
> >> > If so, then IMO the bug is with (d).  We can still use local pages
> >> > for print-rtl, despite the entry in .dynsym.
> >> 
> >> This all makes sense to me.  I'll think about what the local_p
> >
> > Notice this in _bfd_mips_elf_check_relocs:
> >                   /* If we've encountered any other relocation
> >                      referencing the symbol, we'll have marked it as
> >                      dynamic, and, even though we might be able to get
> >                      rid of the GOT entry should we know for sure all
> >                      previous relocations were GOT_PAGE ones, at this
> >                      point we can't tell, so just keep using the
> >                      symbol as dynamic.  This is very important in the
> >                      multi-got case, since we don't decide whether to
> >                      decay GOT_PAGE to GOT_DISP on a per-GOT basis: if
> >                      the symbol is dynamic, we'll need a GOT entry for
> >                      every GOT in which the symbol is referenced with
> >                      a GOT_PAGE relocation.  */
> 
> I'm not really sure what your point is here.  FWIW, this was the code I
> was referring to in (a), on the assumption that the following condition:
> 
> 		  && hmips->root.dynindx == -1)
> 
> is true.  Is that right?

Yes, hmips->root.dynindx == -1 does apply here.

> > Fortunately, the comment is misleading.  We can't decide to decay on a
> > per-GOT basis, but we can decide not to decay even later, on a
> > per-symbol basis.
> 
> I don't really follow this explanation.  From what I can remember
> of the test case, my assumption was:
> 
>     - print-rtl uses the primary GOT
>     - toplev uses a secondary GOT
>     - toplev needs a global GOT entry for f_d_u, print-rtl doesn't
>     - nothing else uses f_d_u
>     - we put f_d_u after the global entries for the primary GOT

Yes, that's what happened.

> So why isn't it possible to decay on a per-GOT basis?  Objects that
> use the primary GOT (print-rtl) can use a page entry.  Objects that use
> toplev's GOT can use the global entry in that GOT.  Not that there's any
> benefit to doing so -- they both might as well use page entries if the
> symbol binds locally -- but I don't see why "we can't".

Remember, we're in check_relocs here.  At this point all objects have
not been loaded, so we can not make any 100% certain decisions based on
the type or visibility of a symbol.  It's not that we can't decide not
to decay on a per-GOT basis, it's that we can't decide in check_relocs
not to do so.

Later, we can say "for all GOTs, PAGE references to this symbol can
remain".  And then, if the particular GOT has no decayed entry for this
symbol, it so happens that all references for that GOT will be handled
as PAGE.

> 
> > @@ -3247,12 +3248,16 @@
> >      {
> >      case R_MIPS_GOT_PAGE:
> >      case R_MIPS_GOT_OFST:
> > -      /* If this symbol got a global GOT entry, we have to decay
> > -	 GOT_PAGE/GOT_OFST to GOT_DISP/addend.  */
> > +      /* If this symbol got a global GOT entry, we might have to decay
> > +	 GOT_PAGE/GOT_OFST to GOT_DISP/addend.  We check whether we need
> > +	 to decay, because if we don't need to it's possible that no GOT
> > +	 entry was allocated in this input BFD's GOT for this reference
> > +	 (it might be in some other GOT, and out of range).  */
> >        local_p = local_p || ! h
> >  	|| (h->root.dynindx
> >  	    < mips_elf_get_global_gotsym_index (elf_hash_table (info)
> > -						->dynobj));
> > +						->dynobj))
> > +	|| SYMBOL_REFERENCES_LOCAL (info, h);
> >        if (local_p || r_type == R_MIPS_GOT_OFST)
> >  	break;
> >        /* Fall through.  */
> 
> A few questions: (probably dumb ones ;)
> 
> - Are the !h and h->root.dynindx checks still needed after this?
>   (Not sure off-hand.)

The !h test is no longer necessary.  The dynindx check still is.
h->root.dynindx == -1 will be handled by SYMBOL_REFERENCES_LOCAL, but
otherwise (if info->shared) a low dynindx and a high dynindx will be
both return FALSE.

> - Will this fix the problem even for protected symbols?
>   A direct call to _bfd_elf_symbol_refs_local_p (h, info, 1)
>   seems more correct.

That's SYMBOL_CALLS_LOCAL; not the clearest name for this circumstance.

  /* Function pointer equality tests may require that STV_PROTECTED
     symbols be treated as dynamic symbols, even when we know that the
     dynamic linker will resolve them locally.  */
  return local_protected;

I guess that doesn't apply here, so SYMBOL_CALLS_LOCAL should be used.

> - Is the hmips->root.dynindx == -1 check above still needed after
>   your patch?

In check_relocs?  I'm not sure.  Certainly anything touching dynindx at
this point is suspect - remember, again, that check_relocs gets called
on a per-input-file basis, so this code is sensitive to the order of
input files.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer