This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
Re: MIPS multigot fixes for Linux
On Wed, Nov 12, 2003 at 09:47:55PM +0000, Richard Sandiford wrote:
> Daniel Jacobowitz <drow@mvista.com> writes:
> > On Tue, Nov 11, 2003 at 11:43:39PM +0000, Richard Sandiford wrote:
> > > 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.
>
> Undefined weak symbols.
>
> > > 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?
>
> Yeah, I think so. h->root.u.def.value would be the value of the symbol
> found in an external object. And if we supported quickstart, that would
> indeed be the right value to use, since it's also the value we'd put in
> the primary GOT.
>
> If we apply your patch, then VALUE is only inserted into the GOT if
> SGI_COMPAT or if no relocation is needed. And going by the table above,
> I think the value we want is always st_value in this case.
>
> > 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.
>
> But doesn't your patch take care of that? My point is that (from the
> irix line above) the "right" thing to do is insert st_value for all
> cases. And your patch successfully works around the cases in glibc
> where that isn't true. Both changes together should do the right thing.
OK, I see where you're going now. And if
> > > || h->root.type == bfd_link_hash_undefined
> > > || h->root.type == bfd_link_hash_undefweak)
then sym->st_value is presumably 0.
"value" is also passed to create_dynamic_relocation but will not be
used, so this should be safe. Revised attached. I haven't got time to
retest it right now, though.
--
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. Simplify calculation of value.
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 22:01:31 -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;
@@ -6670,14 +6694,10 @@ _bfd_mips_elf_finish_dynamic_symbol (out
e.symndx = -1;
e.d.h = (struct mips_elf_link_hash_entry *)h;
- 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;
+ /* Since we don't support Quickstart, the initial value of the GOT
+ entry should be simply the symbol's value. With the caveat about
+ glibc, below. */
+ value = sym->st_value;
memset (rel, 0, sizeof (rel));
rel[0].r_info = ELF_R_INFO (output_bfd, 0, R_MIPS_REL32);
@@ -6693,18 +6713,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);
+ }
}
}
}