Re: Fix undefined weak hidden syms for MIPS shared libraries

[Daniel Jacobowitz <drow at false dot org>]

On Tue, Jul 29, 2008 at 08:48:27PM +0100, Richard Sandiford wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> > This patch goes along with the testcase in my previous message.  We
> > were always emitting a dynamic relocation for even undefined weak
> > symbols, but nothing marked them as dynamic, so the relocation got
> > symndx 0xffffff.  The consensus seems to be that relocations should be
> > emitted in shared libraries if default visibility, and clearly no
> > relocation is necessary for non-default visibility, so that's what
> > I've implemented.
> >
> > Like other ports, MIPS now needs an allocate_dynrelocs routine.
> > adjust_dynamic_symbol is not good enough, since the symbol might not
> > be dynamic.
> 
> I'm probably being dense, but why might it not be dynamic?  Are you
> thinking about the post-non-PIC code or the current code?

If it's undefined/weak/hidden, it won't be dynamic and
adjust_dynamic_symbol won't be called.  Various other cases also; the
function is called in limited circumstances.

The first two hunks (specifically, the second) without the call to
allocate_dynrelocs will break the linker.  Nothing will allocate space
for relocations against global symbols.  We can't allocate this early,
because of the first hunk: we don't know whether the relocation is
necessary yet.

> In the current code, I'd have expected mips_elf_create_got_section
> to be called by this point.  We can't go adding new dynamic
> GOT-referenced symbols in size_dynamic_sections, so even if the
> new bfd_elf_link_record_dynamic_symbol call is currently dead,
> it looks like dangerous dead code.  I don't see why the first
> two hunks aren't currently enough.

The call to bfd_elf_link_record_dynamic_symbol is probably dead.  It
came from elf32-i386.c (or most other allocate_dynrelocs variants).
That's for the undefined, weak, non-hidden case (I didn't explicitly
test that).  I can try removing it.

> 
> As far as the implementation goes:
> 
> > +  /* VxWorks executables are handled elsewhere; we only need to
> > +     allocate relocations in shared objects.  */
> > +  if (htab->is_vxworks && !info->shared)
> > +    return TRUE;
> 
> Where's "elsewhere" in this case?

Anything necessary would happen in
_bfd_mips_vxworks_adjust_dynamic_symbol.  There isn't much worth
speaking of; I wrote the patch in a tree with non-PIC, which has a
similar issue.

> > +      if (h->root.type == bfd_link_hash_undefweak)
> > +	{
> > +	  /* Do not copy relocations for undefined weak symbols with
> > +	     non-default visibility.  */
> > +	  if (ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
> > +	    do_copy = FALSE;
> > +
> > +	  /* Make sure undefined weak symbols are output as a dynamic
> > +	     symbol in PIEs.  */
> > +	  else if (h->dynindx == -1 && !h->forced_local)
> > +	    {
> > +	      if (! bfd_elf_link_record_dynamic_symbol (info, h))
> > +		return FALSE;
> > +	    }
> 
> This implies that "h->forced_local && ELF_ST_VISIBILITY (h->other)
> == STV_DEFAULT" is a valid combination.  Is it one we can really
> create?  If so, why do want do_copy to be true in that case?

I don't know the answer to that, sorry; see any number of elf backends
for where I got this construct.  I figure the smart people who wrote
those knew something I didn't, but perhaps not.

-- 
Daniel Jacobowitz
CodeSourcery