This is the mail archive of the
binutils@sources.redhat.com
mailing list for the binutils project.
Re: MIPS multigot fixes for Linux
- From: Daniel Jacobowitz <drow at mvista dot com>
- To: Richard Sandiford <rsandifo at redhat dot com>
- Cc: binutils at sources dot redhat dot com, Atsushi Nemoto <anemo at mba dot ocn dot ne dot jp>,Eric Christopher <echristo at redhat dot com>
- Date: Wed, 12 Nov 2003 16:07:18 -0500
- Subject: Re: MIPS multigot fixes for Linux
- References: <20031111173516.GA27967@nevyn.them.org> <87n0b2a2tw.fsf@redhat.com>
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);
+ }
}
}
}