Re: MIPS multigot fixes for Linux

[Daniel Jacobowitz <drow at mvista dot com>]

On Thu, Nov 13, 2003 at 03:26:27PM +0000, Richard Sandiford wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> > > *************** _bfd_mips_elf_finish_dynamic_symbol (out
> > > *** 6692,6711 ****
> > >   	      offset = p->gotidx;
> > >   	      rel[0].r_offset = rel[1].r_offset = rel[2].r_offset = offset;
> > >   
> > > ! 	      MIPS_ELF_PUT_WORD (output_bfd, value, sgot->contents + offset);
> > > ! 
> > > ! 	      if ((info->shared
> > > ! 		   || (elf_hash_table (info)->dynamic_sections_created
> > > ! 		       && p->d.h != NULL
> > > ! 		       && ((p->d.h->root.elf_link_hash_flags
> > > ! 			    & ELF_LINK_HASH_DEF_DYNAMIC) != 0)
> > > ! 		       && ((p->d.h->root.elf_link_hash_flags
> > > ! 			    & ELF_LINK_HASH_DEF_REGULAR) == 0)))
> > > ! 		  && ! (mips_elf_create_dynamic_relocation
> > > ! 			(output_bfd, info, rel,
> > > ! 			 e.d.h, NULL, value, &addend, sgot)))
> > > ! 		return FALSE;
> > > ! 	      BFD_ASSERT (addend == 0);
> > >   	    }
> > >   	}
> > >       }
> > > --- 6707,6729 ----
> > >   	      offset = p->gotidx;
> > >   	      rel[0].r_offset = rel[1].r_offset = rel[2].r_offset = offset;
> > >   
> > > ! 	      if (info->shared
> > > ! 		  || (elf_hash_table (info)->dynamic_sections_created
> > > ! 		      && p->d.h != NULL
> > > ! 		      && ((p->d.h->root.elf_link_hash_flags
> > > ! 			   & ELF_LINK_HASH_DEF_DYNAMIC) != 0)
> > > ! 		      && ((p->d.h->root.elf_link_hash_flags
> > > ! 			   & ELF_LINK_HASH_DEF_REGULAR) == 0)))
> > > ! 		{
> > > ! 		  entry = 0;
> > > ! 		  if (! (mips_elf_create_dynamic_relocation
> > > ! 			 (output_bfd, info, rel,
> > > ! 			  e.d.h, NULL, sym->st_value, &entry, sgot)))
> > > ! 		    return FALSE;
> > > ! 		}
> > > ! 	      else
> > > ! 		entry = sym->st_value;
> > > ! 	      MIPS_ELF_PUT_WORD (output_bfd, entry, sgot->contents + offset);
> > >   	    }
> > >   	}
> > >       }
> > > 
> > 
> > The BFD_ASSERT used to not trigger.  Therefore, you'll fill the GOT
> > entry with 0 if you create a relocation and st_value if you don't. 
> > That will break Irix, I assume.
> 
> Hmm.... I'm not sure what you mean here.  The BFD_ASSERT didn't used to
> trigger when?  Before your patch or after it?  On irix or glibc?  Like
> I say, I think this patch is fixing a bug for irix too, so what happened
> before on irix isn't necessarily a good benchmark.
> 
> The point is that the in-place addend _should_ contain the symbol value
> in the irix case.  Irix subtracts the old symbol value first, right?
> 
> I.e. the BFD_ASSERT should be removed even if we go with your patch.
> I forgot about this in the review, sorry. ;(
> 
> I think the code above expresses exactly what we're trying to do.
> If the entry can change, we create a standard R_MIPS_REL32 relocation
> for it.  If it can't change, we just store the final symbol value.
> It's just like any other bit of data in most respects.

I've never seen that BFD_ASSERT trigger, on either Irix or GNU/Linux. 
What that meant to me is that mips_elf_create_dynamic_relocation will
not change entry in this circumstance; entry would stay as zero.  Has
that changed recently?  I see that it has - I just can't keep up.  So
the BFD_ASSERT would probably trigger if I tested current CVS on Irix.

Previously the multi-GOT code would arrange to include the symbol val.
But it appears to duplicate what mips_elf_create_dynamic_relocation is
doing.  So we can re-use the SGI_COMPAT you added to that function for
this difference also - and you're completely right, that's more clear.
It's the same fundamental difference in the relocation machinery.

I don't have time to retest right now unfortunately.  Your variant
looks OK to me for glibc, though, so if you prefer that then let's go
with it.  My changes for no_fn_stub are still necessary of course.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer