Re: Another MIPS multigot patch

[Daniel Jacobowitz <drow at mvista dot com>]

On Thu, Jan 29, 2004 at 05:39:56PM +0000, Richard Sandiford wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> >> 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.
> 
> Ah.  I wasn't ;)  I realise the comment you quoted was from check_relocs,
> but check_relocs doesn't make the final decision about whether a GOT_PAGE
> will decay, because it can't know in general.  I think we're in violent
> agreement on that.
> 
> So I thought you were saying it was impossible for calculate_relocation()
> to decide on a per-GOT basis.  Whereas, AFAICT, it can.  Sorry about that.

Indeed, we agree.

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

> 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.  So maybe the existing
test of dynindx is downright incorrect.

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

> >> - 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.
> 
> Well, OK, turn the question around then: under what circumstances do you
> think this test is still needed?  If the rest of the condition is only
> true for symbols that are known to bind locally (and I think that should
> be the case, otherwise the condition is buggy), then with your change to
> calculate_relocations(), the condition should be safe.

I think the whole section is broken.  Let's start at the top:

              while (hmips->root.root.type == bfd_link_hash_indirect
                     || hmips->root.root.type == bfd_link_hash_warning)
                hmips = (struct mips_elf_link_hash_entry *)
                  hmips->root.root.u.i.link;

              if ((hmips->root.root.type == bfd_link_hash_defined
                   || hmips->root.root.type == bfd_link_hash_defweak)

If the symbol has been defined, OK.  But we're in check_relocs.  Here's
some sample code:

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.

I'd rather fix the one thing I know how to fix than dig neck-deep into
this mess I don't understand :)  But I suspect this entire segment
should just be ripped out, since it demonstrably doesn't do whatever it
is that it's supposed to do.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer