Re: Another MIPS multigot patch

[Richard Sandiford <rsandifo at redhat dot com>]

Daniel Jacobowitz <drow@mvista.com> writes:
>> >> - 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.
>> 
>> I don't know what you mean here.  Do you have an example?
>
> Well, at the basic level, for a symbol with dynindx > 0 and dynindx <
> mips_elf_get_global_gotsym SYMBOL_REFERENCES_LOCAL will return false
> (if info->shared).

That shouldn't happen.  mips_elf_get_global_gotsym_index is the value
associated with DT_MIPS_GOTSYM, i.e., the index of the first global GOT
entry.  Anything in the range [0,mips_elf_get_global_gotsym_index] is
supposed to bind locally.

>> As I understand it, the whole point of your change is that if a symbol
>> binds locally, we can use a normal page/ofst access for it, regardless
>> of whether we happened to allocate a global entry as well.  So in what
>> cases will _bfd_elf_symbol_refs_local_p return true for something that
>> doesn't bind locally?
>
> It's the other way around, _bfd_elf_symbol_refs_local_p will (may)
> return false for something that we would previously have accessed using
> GOT_PAGE.  Iff we are linking a shared library.  Now, on most targets
> the symbol could be overridden at this point.

I don't see what you're getting at.  Your patch changed the condition
in calculate_relocations() to:

      local_p = local_p || ! h
	|| (h->root.dynindx
	    < mips_elf_get_global_gotsym_index (elf_hash_table (info)
						->dynobj))
	|| SYMBOL_REFERENCES_LOCAL (info, h);

and my point was that:

      local_p = local_p || SYMBOL_REFERENCES_LOCAL (info, h);

ought to be just as safe.  But you said that the "h->root.dynindx < ..."
part was still needed, which presumably means that there are cases in
which _bfd_elf_symbol_refs_local_p will return FALSE and local_p should
nevertheless be set to TRUE.  That seems unlikely, since setting local_p
to TRUE will prevent symbol preemption.  Hence the question above.

>> >> - 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.
>> 
>> Exactly, that's why I suggested calling the function directly. ;)
>> The two macros are set up for a distinction that doesn't really
>> apply on MIPS.
>
> Except it's the sort of thing I'd go around and blindly clean up later. 

;)

> I try not to introduce things that I would then clean up :)  Perhaps a
> macro MIPS_REFERENCES_LOCAL?

Well, I still think it's cleaner to call the function directly.  OK, so
we have two convenience macros for it, but I don't see that _all_ calls
to the function have to be through convenience macros.

Mind you, I can easily get carried away over minor stuff like this ;)

> main.c:
>
> int foo;
> extern int foo_func(void);
>
> int
> main(void)
> {
>   return foo_func();
> }
>
> func.c:
>
> extern int foo;
>
> int
> foo_func(void)
> {
>   return foo;
> }
>
> Try "mips64-linux-ld -o main main.o func.o" and "mips64-linux-ld -o
> main func.o main.o".  In one of them, root.root.type is
> bfd_link_hash_undefined.  In the other, it's bfd_link_hash_common. So,
> things are already wrong, since we don't check for common.  If I
> initialize it, then in one case it becomes bfd_link_hash_defined.
> Now, in one link order mips_elf_record_global_got_symbol gets called,
> and in the other it doesn't.

But I don't see why that's a problem.  If main.o is linked first, then
check_relocs() knows that foo binds locally when processing both input
files.  There's no point adding a global GOT entry for it.

If func.o is linked first, then check_relocs() _doesn't_ know for certain
whether foo is local or global when processing the relocations in func.o.
It has to add the global entry, just to be on the safe side.

It sounds from your message like you think that check_relocs() is doing
the wrong thing if it's sensitive to link order.  But I don't think it is.
The GOT_PAGE code in check_relocs() is purely an optimisation.  It's trying
to avoid adding GOT entries when it knows that calculate_relocation() won't
need them.  The optimisation will work better for some link orders than
others, but that's just the way of things.

FWIW, here's how I understand the current code.  Bear in mind
that I didn't write any of it, so this could well be wrong. ;)

  (1) check_relocs(): Decide whether the referenced symbol is already
      known to bind locally.  If so, do nothing, otherwise add a
      global GOT entry just in case it _doesn't_ bind locally.

  (2) calculate_relocation(): Be aggressive about decaying PAGE/OFST
      to DISP/addend.  If a symbol has a global GOT entry, use it and
      decay the relocation, even if a PAGE/OFST combination would have
      been correct.  The check for whether the symbol had a global GOT
      entry is _the inverse of_:

	   (h->root.dynindx
	    < mips_elf_get_global_gotsym_index (elf_hash_table (info)
						->dynobj))

This doesn't work because a symbol might have global entries in some
GOTs but not others.  The existing code only checks whether the "master"
GOT has a global entry.

On the scale of possible fixes, it seems like there are two extremes:

  (a) Change the condition in (2) so that it checks whether the input
      bfd's GOT has a global entry.  We'd still be as aggressive as
      possible about decaying PAGE/OFST to DISP/addend.

  (b) Use the opposite approach to decaying in (2).  Never decay a
      PAGE/OFST to DISP/addend if the symbol binds locally, even if
      the symbol has an in-range global entry.

Your patch did (b), which I agree is the better choice.  It's simpler,
it's less likely to lead to overflow, and it might just give better cache
locality, since we'll be reusing the same entries wherever possible.

With either fix, I think the hmips->root.dynindx == -1 in check_relocs()
can go.

Richard