This is the mail archive of the binutils@sources.redhat.com 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: MIPS multigot fixes for Linux


On Tue, Nov 11, 2003 at 11:43:39PM +0000, Richard Sandiford wrote:
> > I humbly request an opinion from Richard S, who understands this stuff a
> > whole lot better than I do.
> 
> Huh.  You give me too much credit, but I'll have a go. ;)
> 
> It looks like the cases are:
> 
>                                    A    B    C    D    E
>     ------------------------------------------------------
>     symbol defined locally?        Y    Y    N    N    N
>     symbol defined externally?     -    -    N    Y    -
>     creating a DSO?                N    Y    N    N    Y
>     ------------------------------------------------------
>     relocation needed?             N    Y    N    Y    Y
>     ------------------------------------------------------
>     glibc relocation subtrahend:   -    0    -    0    0
>     initial glibc entry:           S    0    S=0  S=0  S=0
>     ------------------------------------------------------
>     irix relocation subtrahend:    -    S    -    S=0  S=0
>     initial irix entry:            S    S    S=0  S=0  S=0
>     ------------------------------------------------------
> 
> where S = st_value.  Like you say, none of the symbols can be lazily
> bound, so S should be 0 for all undefined symbols.  And it's really
> just (B) that needs special handling.
> 
> Does that match your thinking?  If so, then your change to
> _bfd_mips_elf_finish_dynamic_symbol looks good to me:

I'm not entirely sure what you mean for case C.  Otherwise:

Glibc's resolver logic is essentially:
  For local symbols (symidx < DT_GOTSYM) add the symbol's relocated
  value.

  For global symbols (symidx >= DT_GOTSYM) add the contents of the GOT
  entry.  Since the emission of an R_MIPS_REL32 relocation against a
  symbol should disable a PLT stub, this is the same as the symbol's
  relocated value.  So R_MIPS_REL32 performs addend + symbol's
  relocated value.
which agrees with your table; initial entry should always be zero
except for symbols which will not be relocated.

SGI presumably performs addend + symbol's relocated value - our old
friend EA:
  The R_MIPS_REL32 relocation type is the only relocation performed by
  the dynamic linker.  The value EA used by the dynamic linker to
  relocate an R_MIPS_REL32 relocation depends on its r_symndx value. 
  If the relocation entry r_symndx is less than DT_MIPS_GOTSYM, the
  value of EA is the symbol st_value plus displacement.  Otherwise, the
  value of EA is the value in the GOT entry corresponding to the
  relocation entry r_symndx.

So it seems to me that the initial entry should always be EA.  For a
relocation against a local (non-section) symbol, this language makes no
sense, and I think we've established that EA is supposed to be st_value
not including the displacement.  And otherwise it's the "initial" GOT
entry.  With that assumption I agree with the SGI portion of your table
also.

> > -	      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);
> > +	      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)))
> > +		{
> > +		  /* Unfortunately this is another one of the differences
> > +		     between glibc and the Irix rld.  */
> > +		  if (!SGI_COMPAT (output_bfd))
> > +		    MIPS_ELF_PUT_WORD (output_bfd, 0, sgot->contents + offset);
> > +
> > +		  if (! (mips_elf_create_dynamic_relocation
> > +			 (output_bfd, info, rel,
> > +			  e.d.h, NULL, value, &addend, sgot)))
> > +		    return FALSE;
> > +		  BFD_ASSERT (addend == 0);
> > +		}
> 
> I guess a more verbose comment might be useful though.  How about changing
> it to something like:
> 
>       /* ??? The dynamic linker should subtract the original value of the
>          symbol's GOT entry (which is always st_value) and add in the final
>          value.  However, glibc's ld.so just adds the final value, so the
>          in-place addend must be zero.  */
> 
> Also, as a very minor clean-up, you could bring "addend" into the new block.
> It shouldn't need to be initialised to zero then.

It still needs to be initialized if we're going to assert on it...

> Looking at the code above, I see VALUE is set by:
> 
>       if (info->shared
> 	  || h->root.type == bfd_link_hash_undefined
> 	  || h->root.type == bfd_link_hash_undefweak)
> 	value = 0;
>       else if (sym->st_value)
> 	value = sym->st_value;
>       else
> 	value = h->root.u.def.value;
> 
> If the table above is right, then I think your patch will make this
> equivalent to:
> 
>       value = st_value;
> 
> which IMO makes things easier to follow.

And later:
 > Uh.  Equivalent for _glibc_.  For irix, it fixes a bug, since the
 > relocation field should contain the symbol value even if info->shared.


I'm not sure about this.  I'd rather not make that change myself, since
I don't understand it - for instance, I don't understand why
h->root.u.def.value was there in the first place.  Did it have
something to do with vestigial quickstart code?

Besides, I don't see what you mean.  If the symbol has a value but we
are emitting a relocation (for data objects, for instance) we must
still put 0 in the GOT for glibc.

> I'll try to look at the no_fn_stub thing tomorrow.

Just as well you waited; it's too eager.  It disables stubs completely
if more than one GOT is needed, where I only wanted to disable stubs
for functions referenced from a secondary GOT.  Though the difference
is mostly one of luck.  Revised version of both patches below.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-11-12  Daniel Jacobowitz  <drow@mvista.com>

	* elfxx-mips.c (mips_elf_set_global_got_offset): Don't set
	no_fn_stub.
	(mips_elf_set_no_stub): New function.
	(mips_elf_multi_got): Call it.
	(_bfd_mips_elf_finish_dynamic_symbol): Fill relocated GOT entries
	with zero for ! SGI_COMPAT.

Index: elfxx-mips.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/bfd/elfxx-mips.c,v
retrieving revision 1.78
diff -u -p -r1.78 elfxx-mips.c
--- elfxx-mips.c	13 Oct 2003 19:51:09 -0000	1.78
+++ elfxx-mips.c	12 Nov 2003 21:04:18 -0000
@@ -2338,10 +2338,6 @@ mips_elf_set_global_got_offset (entryp, 
 	  BFD_ASSERT (g->global_gotsym == NULL);
 
 	  entry->gotidx = arg->value * (long) g->assigned_gotno++;
-	  /* We can't do lazy update of GOT entries for
-	     non-primary GOTs since the PLT entries don't use the
-	     right offsets, so punt at it for now.  */
-	  entry->d.h->no_fn_stub = TRUE;
 	  if (arg->info->shared
 	      || (elf_hash_table (arg->info)->dynamic_sections_created
 		  && ((entry->d.h->root.elf_link_hash_flags
@@ -2357,6 +2353,30 @@ mips_elf_set_global_got_offset (entryp, 
   return 1;
 }
 
+/* Mark any global symbols referenced in the GOT we are iterating over
+   as inelligible for lazy resolution stubs.  */
+static int
+mips_elf_set_no_stub (entryp, p)
+     void **entryp;
+     void *p ATTRIBUTE_UNUSED;
+{
+  struct mips_got_entry *entry = (struct mips_got_entry *)*entryp;
+
+  if (entry->abfd != NULL && entry->symndx == -1
+      && entry->d.h->root.dynindx != -1)
+    {
+      /* We can't do lazy update of GOT entries for non-primary GOTs since
+        the PLT entries don't use the right offsets, so punt at it for now.
+        We set this here because we are called via mips_elf_multi_got
+        before _bfd_mips_elf_adjust_dynamic_symbol reads the no_fn_stub
+        flag; this only matters for the global case, but
+        _bfd_mips_elf_size_dynamic_sections is too late.  */
+      entry->d.h->no_fn_stub = TRUE;
+    }
+
+  return 1;
+}
+
 /* Follow indirect and warning hash entries so that each got entry
    points to the final symbol definition.  P must point to a pointer
    to the hash table we're traversing.  Since this traversal may
@@ -2624,6 +2644,11 @@ mips_elf_multi_got (abfd, info, g, got, 
       g->next = gg->next;
       gg->next = g;
       g = gn;
+
+      /* Mark global symbols in every non-primary GOT as ineligible for
+	 stubs.  */
+      if (g)
+	htab_traverse (g->got_entries, mips_elf_set_no_stub, NULL);
     }
   while (g);
 
@@ -6662,7 +6687,6 @@ _bfd_mips_elf_finish_dynamic_symbol (out
       bfd_vma offset;
       bfd_vma value;
       Elf_Internal_Rela rel[3];
-      bfd_vma addend = 0;
 
       gg = g;
 
@@ -6693,18 +6717,30 @@ _bfd_mips_elf_finish_dynamic_symbol (out
 
 	      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);
+	      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)))
+		{
+		  bfd_vma addend = 0;
+
+		  /* ??? The dynamic linker should subtract the original
+		     value of the symbol's GOT entry (which is always
+		     st_value) and add in the final value.  However, glibc's
+		     ld.so just adds the final value, so the in-place addend
+		     must be zero.  */
+		  if (!SGI_COMPAT (output_bfd))
+		    MIPS_ELF_PUT_WORD (output_bfd, 0, sgot->contents + offset);
+
+		  if (! (mips_elf_create_dynamic_relocation
+			 (output_bfd, info, rel,
+			  e.d.h, NULL, value, &addend, sgot)))
+		    return FALSE;
+		  BFD_ASSERT (addend == 0);
+		}
 	    }
 	}
     }


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