This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFC][PATCH] MIPS ifunc for binutils
- From: Jack Carter <jack dot carter at imgtec dot com>
- To: <binutils at sourceware dot org>, <rdsandiford at googlemail dot com>
- Date: Tue, 20 Aug 2013 19:12:02 -0700
- Subject: Re: [RFC][PATCH] MIPS ifunc for binutils
- References: <4CEFBC1BE64A8048869F799EF2D2EEEE01AAE1F0 at BADAG02 dot ba dot imgtec dot org> <87k3jgf6bb dot fsf at talisman dot default>
On 08/20/2013 01:36 PM, Richard Sandiford wrote:
Jack Carter <Jack.Carter@imgtec.com> writes:
I attempted to follow the ARM implementation, but the
traditional MIPS GOT design forced me to dirverge a bit.
All ifunc functions are represented in .iplt section stubs and
in the .igot.plt section table. Each igot.plt entry has an
R_MIPS_IRELOCATE relocation record against it with the initial
igot entry having link time address of the ifunc routine and
after the relocation action, the final runtime target routine
address.
The only change to the traditional MIPS GOT was to use the .iplt stub
address for the defined ifunc function instead of the function address.
This should allow seamless multigot support
Like you said in your earlier message, this seems too big a penalty.
I think instead we should come up with some way of integrating ifuncs
into the ABI's GOT scheme. E.g. we could add a new dynamic tag that sets
the index of the first local GOT entry, such as DT_MIPS_LOCAL_GOTNO_START.
When that tag is defined, ld.so would use it instead of the current hard-coded
1 or 2 (depending on ELF_MIPS_GNU_GOT1_MASK).
Then the beginning of the primary GOT could be relocated as normal and we'd
have control over the relocation order. (Of course, we'd only want to use
that area where necessary, since the normal scheme is more efficient for
the cases that it can handle.)
That's just one suggestion. I'm sure there are other ways of doing it.
But I think we should make whichever change seems best now rather than
treat it as a future enhancement, otherwise we'll be running the risk
of compatibility problems.
It's good to have someone to discuss this with.
My current scheme uses the iplt stubs only with a.outs. If I am not
mistaken, I have to use them with non-shared no matter what and in the
case of shared a.outs the use defined ifunc will be very limited if at all.
Does this warrant messing with the current got resolution order?
Some other more minor comments:
@@ -741,6 +756,13 @@ static bfd_boolean mips_elf_create_dynamic_relocation
bfd_vma *, asection *);
static bfd_vma mips_elf_adjust_gp
(bfd *, struct mips_got_info *, bfd *);
+static void
+mips_elf_allocate_ireloc
+ (struct bfd_link_info *, struct mips_elf_link_hash_entry *);
+static bfd_boolean
+mips_elf_allocate_iplt
+ (bfd *, struct mips_elf_link_hash_table *, struct bfd_link_info *,
+ struct mips_elf_link_hash_entry *);
Where possible, please order the functions so that these forward
declarations aren't needed. (The current ordering is historical
and not ideal...)
Good to know. I was following what I saw. Will change it.
@@ -811,6 +833,11 @@ static bfd *reldyn_sorting_bfd;
/* The name of the stub section. */
#define MIPS_ELF_STUB_SECTION_NAME(abfd) ".MIPS.stubs"
+/* Return the relocation section associated with NAME. HTAB is the
+ bfd's elf32_arm_link_hash_entry. */
+#define MIPS_ELF_RELOC_SECTION(INFO, NAME) \
+ (mips_elf_hash_table (INFO)->is_vxworks ? ".rel" NAME : ".rela" NAME)
+
Looks like this is unused (and also the wrong way around FWIW).
Right. Missed that in my cleanup.
+/* Is this a non-shared link? */
+#define MIPS_STATIC_LINK(info) \
+ ((elf_hash_table(info)->dynamic_sections_created) == FALSE)
+/* Is this a non-fPIC a.out? */
+#define MIPS_AOUT_LINK(info) \
+ (info->executable == TRUE)
I think it'd be better not to have these and just check the fields directly.
I am happy to change this, but while we are at it are my assumptions
correct? It was not clear to me how to make the distinctions. There is a
flag for static_link, but does not seem to be used.
+/* The format of non-dso IPLT entries. */
+static const bfd_vma mips_exec_iplt_entry[] =
+{
+ 0x3c0f0000, /* lui $15, %hi(.got.iplt entry) */
+ 0x01f90000, /* l[wd] $25, %lo(.got.iplt entry)($15) */
+ 0x03200008, /* jr $25 */
+ 0x00000000 /* nop */
+};
+
+/* The format of dso IPLT entries. */
+static const bfd_vma mips_dso_iplt_entry[] =
+{
+ 0x03e07821, /* addu t7, ra, r0 */
+ 0x04110001, /* bal 8 */
+ 0x3c190000, /* lui t9, %hi(<.got.plt slot offset>) */
+ 0x033fc821, /* addu t9, t9, ra */
+ 0x8f390000, /* l[wd] t9, %lo(<.got.plt slot offset>)(t9) */
+ 0x03200008, /* jr $t9 */
+ 0x01e0f821, /* addu ra, t7, r0 */
+ 0x00000000 /* nop */
+};
Since we don't use IPLTs for DSOs, just executables, it looks like this
is really a PIE vs. non-PIE distinction. I think the comments would be
clearer that way. But why do we need IPLT entries for PIEs but not DSOs?
I'd assumed we didn't need them for DSOs because all calls must go via
the GOT, but that's true of PIEs as well.
I personally don't care much -- in fact argued against it --
but the current PLT entries were chosen to be MIPS I compatible and
so avoided filling the load delay slot. Whichever way we decide to go,
I think it'd be better for "normal" PLTs and IPLTs to be consistent.
The DSO version is no longer needed. I just hated to rip it out after I
got it working and then figured out how to not need the iplt for dso
ifunc definitions.
I'll rip it out.
@@ -1944,6 +2004,15 @@ mips_elf_check_symbols (struct mips_elf_link_hash_entry *h, void *data)
if (!hti->info->relocatable)
mips_elf_check_mips16_stubs (hti->info, h);
+ /* If the referenced symbol is ifunc, allocate an iplt stub for it. */
+ if (h && (h->needs_iplt == FALSE) && (h->root.type == STT_GNU_IFUNC))
Formatting niggle, sorry, but no brackets around == conditions.
Local style is to use !x instead of x == FALSE.
I need to follow the rules so please point out these things. I'll make
the change.
+/* Create the .iplt, .rel(a).iplt and .igot.plt sections. */
+
+static bfd_boolean
+mips_elf_create_ifunc_sections(bfd *abfd, struct bfd_link_info *info)
Another formatting niggle, but there should be a space before an
opening bracket (other instances too).
I'll make the change
@@ -5261,12 +5394,21 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
&& h->root.root.u.def.section)
{
sec = h->root.root.u.def.section;
- if (sec->output_section)
- symbol = (h->root.root.u.def.value
- + sec->output_section->vma
- + sec->output_offset);
+ /* If this symbol is an ifunc, point to the iplt stub for it, */
+ if (h->needs_iplt)
+ {
+ BFD_ASSERT (htab->siplt != NULL);
+ symbol = (htab->siplt->output_section->vma
+ + htab->siplt->output_offset
+ + h->iplt_offset);
+ }
This isn't the right place do this, since the surrounding block is just
calculating the symbol value. If we want to redirect the relocation to
something other than the symbol value, we should do it in the following
block instead (where PLTs are also handled).
I kept running into timing issues. I will follow your suggestion and see
what if any issues arise.
+/* Ifunc variant of mips_elf_create_dynamic_relocation() */
+static bfd_boolean
+ifunc_create_dynamic_relocation(bfd *output_bfd,
+ struct bfd_link_info *info,
+ Elf_Internal_Rela *outrel,
+ struct mips_elf_link_hash_entry *h,
+ bfd_vma symbol)
+{
+ long indx;
+ bfd_vma t_off;
+ struct mips_elf_link_hash_table *htab = mips_elf_hash_table (info);
+ asection *sreloc = mips_elf_rel_dyn_section (info, FALSE);
+
+ /* Adjust the output offset of the relocation to reference the
+ correct location in the output file. */
+ if (h->global_got_area == GGA_NONE)
+ {
+ t_off = mips_elf_local_got_index(output_bfd,
+ output_bfd,
+ info,
+ symbol,
+ h->root.dynindx,
+ h,
+ 0);
+ }
+ else
+ {
+ t_off = mips_elf_global_got_index(output_bfd,
+ info,
+ output_bfd,
+ &h->root,
+ 0);
+ }
+
+ t_off += (htab->sgot->output_section->vma +
+ htab->sgot->output_offset);
+
+ outrel[0].r_offset = t_off;
+ outrel[1].r_offset = t_off;
+ outrel[2].r_offset = t_off;
+
+ indx = h->root.dynindx;
+ outrel[0].r_info = ELF_R_INFO (output_bfd, (unsigned long) indx,
+ R_MIPS_IRELATIVE);
+
+ if (ABI_64_P (output_bfd))
+ {
+ (*get_elf_backend_data (output_bfd)->s->swap_reloc_out) (
+ output_bfd, &outrel[0],
+ (sreloc->contents
+ + sreloc->reloc_count * sizeof (Elf64_Mips_External_Rel)));
+ }
+ else
+ {
+ bfd_elf32_swap_reloc_out (
+ output_bfd, &outrel[0],
+ (sreloc->contents
+ + sreloc->reloc_count * sizeof (Elf32_External_Rel)));
+ }
+ /* We've now added another relocation. */
+ ++sreloc->reloc_count;
+
+ return TRUE;
+}
I didn't really understand this. Why is the handling so different from
the other dynamic relocations? We should really relocate the field
specified by "rel", whereas this seems to be finding and relocating a
GOT entry field. Also, is the global case (h->global_got_area != GGA_NONE)
ever used in practice? IRELATIVE should only really be for
locally-binding symbols.
I'll go back to the drawing board with this. It was done early in my
development and the logic of why I did it this way may not be based on
reality.
@@ -7695,6 +7942,14 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
bed = get_elf_backend_data (abfd);
rel_end = relocs + sec->reloc_count * bed->s->int_rels_per_ext_rel;
+ /* This needs to happen early. If the sections aren't needed
+ they will not get generated. */
+ if (htab->root.dynobj == NULL)
+ htab->root.dynobj = abfd;
+ if (!htab->siplt &&
+ !(mips_elf_create_ifunc_sections (htab->root.dynobj, info)))
+ return FALSE;
Formatting should be:
if (!htab->siplt
&& !mips_elf_create_ifunc_sections (htab->root.dynobj, info))
return FALSE;
Will make the change.
@@ -8177,10 +8432,14 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
/* We need a stub, not a plt entry for the undefined
function. But we record it as if it needs plt. See
- _bfd_elf_adjust_dynamic_symbol. */
- h->needs_plt = 1;
- h->type = STT_FUNC;
- }
+ _bfd_elf_adjust_dynamic_symbol. If it is an ifunc
+ symbol it will go into an iplt section and not plt. */
+ if (h->type != STT_GNU_IFUNC)
+ {
+ h->needs_plt = 1;
+ h->type = STT_FUNC;
+ }
+ }
I think only the "h->type = STT_FUNC" bit should be conditional.
If a DSO has a call to a preemptible STT_GNU_IFUNC, it needs to use
a normal lazy-binding stub.
Not sure about this one. I have test cases that call from and to ifunc
symbols defined in dsos and visa versa. That said, I will look at this
again and either change it or come up with a decent defence.
Thank you for giving this a hard look. I will have to live the result of
the final patch and would like it to be a good one.
You may also want to look at the glibc.ports patch that goes with the
binutils one.
Thanks (especially for tackling such a tricky issue!),
Richard