This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix undefined weak hidden syms for MIPS shared libraries


Daniel Jacobowitz <drow@false.org> writes:
> 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.

Oh, duh, sorry.  I hadn't quite adjusted to the fact that
we're relying on deferred allocation for all symbols in
the hash table.

(undefined/weak/hidden isn't all that interesting since
it's one the cases where we don't want a dynamic reloc anyway.
But defined/hidden is, of course.)

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

Thanks.  I think that mips_elf_create_got_section call (and the MIPS
attribute to the GOT) makes it sufficiently different for it to be
worth trying.

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

OK. ;(  Cut-&-paste folklore really isn't nice though.  If the person
adding the code doesn't know why it is the way it is, the person changing
it later has even less chance...

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]