Re: Another MIPS multigot patch

[Richard Sandiford <rsandifo at redhat dot com>]

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?

> 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

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

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

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

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

Richard