This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH][MIPS] IFUNC support 2nd round
- From: Jack Carter <Jack dot Carter at imgtec dot com>
- To: Richard Sandiford <rdsandiford at googlemail dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Mon, 25 Nov 2013 15:54:13 +0000
- Subject: RE: [PATCH][MIPS] IFUNC support 2nd round
- Authentication-results: sourceware.org; auth=none
- References: <4CEFBC1BE64A8048869F799EF2D2EEEE4C5160F0 at BADAG02 dot ba dot imgtec dot org>,<87d2lq3sri dot fsf at talisman dot default>
Richard,
And I thought I nailed the formatting :-(
After a first pass of these comments most are clear and will be acted on including new tests.
I had rejected the dynamic tag idea for some good reason and now can't remember why. I'll either come up with a decent rebuttal or attempt to implement the tag.
Thanks for the review,
Jack
________________________________________
From: Richard Sandiford [rdsandiford@googlemail.com]
Sent: Sunday, November 24, 2013 4:10 AM
To: Jack Carter
Cc: binutils@sourceware.org
Subject: Re: [PATCH][MIPS] IFUNC support 2nd round
Thanks for the updates and sorry for the slow reply.
Jack Carter <Jack.Carter@imgtec.com> writes:
> 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 in executables.
> This should allow seamless multigot support. For defined ifunc routines in
> shared objects have no iplts and depend exclusively on the dynamic symbol
> resolution to call the resolver for the correct runtime address.
>
> I believe I have addressed all the issues of the first patch review with the
> exception of one of the forward declarations (fixing it was getting too messy)
> and the penalty of shared executables with ifunc definitions forcing everything
> through iplts stubs. Still not sure how to avoid that one, but the vast majority
> of ifunc use should be by dsos so it may not be a priority.
What did you think of the dynamic tag idea:
http://www.sourceware.org/ml/binutils/2013-08/msg00145.html
The current scheme seems like a relatively big penalty for traditional
dynamic executables. Maybe everyone's using the PLT extensions these
days though. If no-one else objects that I won't press the issue.
> bfd/ChangeLog
> * bfd-in2.h (BFD_RELOC_MIPS_IRELATIVE): New relocation.
Note that this is an automatically-generated file, so you should regenerate
it rather than edit it by hand. The entry would normally be:
* bfd-in2.h: Regenerate.
> @@ -372,6 +372,12 @@ struct mips_elf_link_hash_entry
> being called returns a floating point value. */
> asection *call_fp_stub;
>
> + /* Offset into the IPLT table. */
> + unsigned int iplt_offset;
> +
> + /* Offset into the IGOT table. */
> + unsigned int igot_offset;
These should be bfd_vmas.
> @@ -410,6 +416,9 @@ struct mips_elf_link_hash_entry
>
> /* Does this symbol resolve to a PLT entry? */
> unsigned int use_plt_entry : 1;
> +
> + /* Does this symbol need an IPLT stub? */
> + unsigned int needs_iplt: 1;
> };
>
> /* MIPS ELF linker hash table. */
> @@ -454,6 +463,9 @@ struct mips_elf_link_hash_table
> asection *srelplt2;
> asection *sgotplt;
> asection *splt;
> + asection *sigotplt;
> + asection *siplt;
> + asection *sreliplt;
> asection *sstubs;
> asection *sgot;
We should just use the fields in the generic ELF htab. (Some of the existing
fields are now redundant too, sorry. I should clean that up at some point.)
> @@ -744,6 +759,9 @@ 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 *);
Nit, but:
static void mips_elf_allocate_ireloc
(struct bfd_link_info *, struct mips_elf_link_hash_entry *);
> @@ -1160,6 +1178,32 @@ static const bfd_vma mips_vxworks_shared_plt_entry[] =
> 0x10000000, /* b .PLT_resolver */
> 0x24180000 /* li t8, <pltindex> */
> };
> +
> +/* The format of non-pie IPLT entries.
> + In the case of mips1 the first nop will be issued before the
> + jr instuction */
> +static const bfd_vma mips32_exec_iplt_entry[] =
> +{
> + 0x3c0f0000, /* lui $15, %hi(.got.iplt entry) */
> + 0x01f90000, /* l[wd] $25, %lo(.got.iplt entry)($15) */
> + 0x03200008, /* jr $25 */
> + 0x00000000, /* nop */
> + 0x00000000 /* nop */
> +};
> +
> +/* The format of non-pie 64 bit IPLT entries. */
> +static const bfd_vma mips64_exec_iplt_entry[] =
> +{
> + 0x3c0f0000, /* lui $15, %highest(.got.iplt entry) */
> + 0x65ef0000, /* daddiu $15, $15, %higher(.got.iplt entry) */
> + 0x000f7c38, /* dsll $15,$15, 16 */
> + 0x65ef0000, /* daddiu $15, $15, %hi(.got.iplt entry) */
> + 0x000f7c38, /* dsll $15,$15, 16 */
> + 0x01f90000, /* l[wd] $25, %lo(.got.iplt entry)($15) */
> + 0x03200008, /* jr $25 */
> + 0x00000000, /* nop */
> +};
Since the PIC/PIE versions are gone now, we might as well drop the
"non-pie" bit.
> @@ -1935,6 +1982,40 @@ mips_elf_add_la25_stub (struct bfd_link_info *info,
> : mips_elf_add_la25_intro (stub, info));
> }
>
> +/* Reserve space in the iplt and igot tables for another ifunc entry.
> + Don't do anything if this is a dso link. */
> +static bfd_boolean
> +mips_elf_allocate_iplt (bfd *abfd, struct mips_elf_link_hash_table *mhtab,
> + struct bfd_link_info *info,
> + struct mips_elf_link_hash_entry *mh)
> +{
> +
> + asection *s;
> +
> + if (mh->needs_iplt || !info->executable)
> + return TRUE;
Why is this an executable-based distinction? PIEs can't have .iplts under
this scheme, so I think this should be !info->shared instead.
Minor, sorry, but I think an assert for !mh->needs_iplt would be better
than an early-out.
> + BFD_ASSERT (mhtab->siplt != NULL);
> +
> + s = mhtab->siplt;
> + mh->iplt_offset = s->size;
> + s->size += mhtab->iplt_entry_size;
> +
> + /* Create a symbol for the stub. */
> + mips_elf_create_stub_symbol (info, mh, ".iplt.",s, mh->iplt_offset,
> + mhtab->iplt_entry_size);
Space after ",".
> + BFD_ASSERT (mhtab->sigotplt != NULL);
> + mh->igot_offset = mhtab->sigotplt->size;
> + mhtab->sigotplt->size += MIPS_ELF_GOT_SIZE (abfd);
> + mips_elf_allocate_ireloc (info, mh);
> +
> +/* This should be the only place needs_iplt is set */
> + mh->needs_iplt = TRUE;
Comment should be indented with code.
> @@ -1947,6 +2028,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 && h->root.type == STT_GNU_IFUNC)
> + {
> + struct bfd_link_info *info = hti->info;
> + if (!(mips_elf_allocate_iplt (info->output_bfd,
> + mips_elf_hash_table (info), info, h)))
> + return FALSE;
> + }
No brackets between the "!" and the function call.
> @@ -4998,6 +5088,74 @@ mips_elf_create_compact_rel_section
> return TRUE;
> }
>
> +/* 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)
> +{
> + struct mips_elf_link_hash_table * volatile htab;
> + const struct elf_backend_data *bed;
> + bfd *dynobj;
> + asection *s;
> + flagword flags;
> +
> + /* Don't do anything if a dso link. */
> + if (!info->executable)
> + return TRUE;
As above, I think this should be !info->shared.
> +
> + htab = mips_elf_hash_table (info);
> + dynobj = (htab->root.dynobj) ? htab->root.dynobj : abfd;
This didn't look right at first, but I see it's OK because abfd itself
is always htab->root.dynobj. I think we should remove the abfd parameter
and just have:
dynobj = htab->root.dynobj;
here.
> + bed = get_elf_backend_data (dynobj);
> + flags = bed->dynamic_sec_flags;
> +
> + if (htab->root.iplt == NULL)
> + {
> + s = bfd_make_section_anyway_with_flags (dynobj, ".iplt",
> + flags | SEC_READONLY | SEC_CODE);
> + if (s == NULL
> + || !bfd_set_section_alignment (dynobj, s, bed->plt_alignment))
> + return FALSE;
> + htab->root.iplt = s;
> + htab->siplt = s;
> + if (!info->shared)
> + if (ABI_64_P (abfd))
> + htab->iplt_entry_size = 4 * ARRAY_SIZE (mips64_exec_iplt_entry);
> + else
> + htab->iplt_entry_size = 4 * ARRAY_SIZE (mips32_exec_iplt_entry);
> + else
> + {
> + ;
> + }/* FIXME: None of the ifunc related sections should be built */
With the change above, the outer "if" and FIXME would go away.
> @@ -6829,6 +6995,8 @@ _bfd_mips_elf_section_processing (bfd *abfd, Elf_Internal_Shdr *hdr)
> hdr->sh_size += hdr->sh_addralign - adjust;
> }
> }
> + else if (strcmp (name, ".igot.plt") == 0)
> + hdr->sh_entsize = ABI_64_P (abfd) ? 8 : 4;
hdr->sh_entsize = MIPS_ELF_GOT_SIZE (abfd);
> @@ -7167,6 +7335,10 @@ _bfd_mips_elf_add_symbol_hook (bfd *abfd, struct bfd_link_info *info,
> flagword *flagsp ATTRIBUTE_UNUSED,
> asection **secp, bfd_vma *valp)
> {
> +
> + if (ELF_ST_TYPE (sym->st_info) == STT_GNU_IFUNC)
> + elf_tdata (info->output_bfd)->has_gnu_symbols = TRUE;
No blank line before the "if".
> @@ -7680,6 +7852,29 @@ mips_elf_make_plt_record (bfd *abfd)
> return entry;
> }
>
> +/* Reserve space for R_MIPS_IRELATIVE relocations. If the link is
> + dynamic, the relocations should go in SRELOC, otherwise they should
> + go in the special .rel.iplt section. */
> +
> +static void
> +mips_elf_allocate_ireloc (struct bfd_link_info *info,
> + struct mips_elf_link_hash_entry *hmips ATTRIBUTE_UNUSED)
The comment doesn't seem to match the function: there's no sreloc parameter.
Might as well remove the unused hmips parameter.
> +{
> + struct mips_elf_link_hash_table *htab;
> + asection *srel = NULL;
> + bfd *dynobj = elf_hash_table (info)->dynobj;
> + BFD_ASSERT (dynobj != NULL);
> +
> + htab = mips_elf_hash_table (info);
> + srel = (!elf_hash_table (info)->dynamic_sections_created)
> + ? htab->sreliplt
> + : mips_elf_rel_dyn_section (info, FALSE);
Nit, sorry, but:
srel = (elf_hash_table (info)->dynamic_sections_created
? mips_elf_rel_dyn_section (info, FALSE)
: htab->sreliplt);
(with the brackets around the whole ?:).
> + BFD_ASSERT (srel != NULL);
> +
> + srel->size += ((htab->is_vxworks) ? MIPS_ELF_RELA_SIZE (dynobj)
> + : MIPS_ELF_REL_SIZE (dynobj));
srel->size += (htab->is_vxworks
? MIPS_ELF_RELA_SIZE (dynobj)
: MIPS_ELF_REL_SIZE (dynobj));
Sorry for being so picky about formatting...
> @@ -7716,6 +7911,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;
"&&" should go on the following line.
> @@ -8217,10 +8420,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. */
s/and not plt/rather than have a lazy-binding stub/. But...
> + if (h->type != STT_GNU_IFUNC)
> + {
> + h->needs_plt = 1;
> + h->type = STT_FUNC;
> + }
> + }
...I'm still uncomfortable with this. We shouldn't stop using lazy-binding
stubs just because we've seen an ifunc symbol. There's no guarantee at this
stage that the call to the ifunc binds locally. If the call doesn't bind
locally then we should still have a lazy-binding stub. (If the symbol is
preempted, the preempting definition doesn't need to be an ifunc, and we
still want lazy binding in that case.)
So I think this should be:
h->needs_plt = 1;
if (h->type != STT_GNU_FUNC)
h->type = STT_FUNC;
Code that runs later needs to work regardless of whether an ifunc symbol
is seen before or after a CALL* reloc. That means that it needs to be
able to handle cases where needs_plt is set for ifuncs.
> @@ -8669,6 +8876,13 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void *inf)
> dynobj = elf_hash_table (info)->dynobj;
> hmips = (struct mips_elf_link_hash_entry *) h;
>
> + /* Record any ifunc symbols */
> + if (h && hmips->needs_iplt)
> + {
> + mips_elf_allocate_ireloc (info, hmips);
> + return TRUE;
> + }
> +
In general I'd prefer not to have an early exit here. I.e.:
/* Allocate space for .igot.plt relocations. */
if (h && hmips->needs_iplt && !mips_elf_allocate_ireloc (info, hmips))
return FALSE;
Although in practice it shouldn't matter, I think this should go after:
/* Ignore indirect symbols. All relocations against such symbols
will be redirected to the target symbol. */
if (h->root.type == bfd_link_hash_indirect)
return TRUE;
> @@ -10244,6 +10461,134 @@ mips_elf_irix6_finish_dynamic_symbol (bfd *abfd ATTRIBUTE_UNUSED,
> }
> }
>
> +/* Create the contents of a iplt entry for the ifunc symbol. */
> +
> +static bfd_boolean
> +mips_elf_create_iplt (bfd *output_bfd,
> + bfd *dynobj,
> + struct mips_elf_link_hash_table *htab,
> + struct mips_elf_link_hash_entry *hmips,
> + Elf_Internal_Sym *sym,
> + struct bfd_link_info *info)
> +{
> + /* We've decided to create an IPLT entry for this symbol. */
> + bfd_byte *loc;
> + bfd_vma igot_index = 0;
> + bfd_vma igotplt_address = 0;
> + bfd_vma load;
> + const bfd_vma *iplt_entry;
> + asection *gotsect, *relsect;
> +
> + gotsect = htab->sigotplt;
> + igot_index = hmips->igot_offset/MIPS_ELF_GOT_SIZE (dynobj);
Spaces around "/".
> + relsect = (!elf_hash_table (info)->dynamic_sections_created)
> + ? htab->sreliplt
> + : mips_elf_rel_dyn_section (info, FALSE);
Might as well split this out into a minifunction since it occurs
elsewhere.
> + /* Calculate the address of the .igot.plt entry. */
> + igotplt_address = (gotsect->output_section->vma +
> + gotsect->output_offset +
> + hmips->igot_offset);
"+"s at the start of lines, here and elsewhere.
> + /* Initially point the .got.iplt entry at the user ifunc routine. */
> + if (!gotsect->contents)
> + gotsect->contents = bfd_zalloc(output_bfd,gotsect->size);
Space after "bfd_zalloc".
> + loc = (bfd_byte *)gotsect->contents +
Space after ")".
> + igot_index * MIPS_ELF_GOT_SIZE (dynobj);
> +
> + if (ABI_64_P (output_bfd))
> + bfd_put_64 (output_bfd, sym->st_value, loc);
> + else
> + bfd_put_32 (output_bfd, sym->st_value, loc);
> +
> + /* Find out where the .iplt entry should go. */
> + if (!htab->siplt->contents)
> + htab->siplt->contents = bfd_zalloc (output_bfd,htab->siplt->size);
Space after ",". This should return false if the allocation fails.
> + loc = htab->siplt->contents + hmips->iplt_offset;
> +
> + /* Pick the load opcode. */
> + load = MIPS_ELF_LOAD_WORD (output_bfd);
> +
> + /* Fill in the IPLT entry itself. */
> + if (!info->shared)
> + {
As above, I think the !info->shared is redundant. We should only create
IPLTs if !info->shared.
> + if (ABI_64_P (output_bfd) && igotplt_address > 0xffffffffffffL)
> + {
> + /* 64 bit */
> + bfd_vma highest = mips_elf_highest (igotplt_address);
> + bfd_vma higher = mips_elf_higher (igotplt_address);
> + bfd_vma high = mips_elf_high (igotplt_address);
> + bfd_vma low = igotplt_address & 0xffff;
> +
> + iplt_entry = mips64_exec_iplt_entry;
> + bfd_put_32 (output_bfd, iplt_entry[0] | highest, loc);
I don't think the "igotplt_address > 0xffffffffffffL" is correct,
since LUI sign-extends. If we had an address 0x8000_0000_0000 then
the next arm would give 0xffff_8000_0000_0000 instead.
Since we support ILP32 hosts too, maybe:
if (ABI_64_P (output_bfd) && igotplt_address >= ((bfd_vma) 1 << 47))
> + bfd_put_32 (output_bfd, iplt_entry[1] | higher, loc + 4);
> + bfd_put_32 (output_bfd, iplt_entry[2], loc + 8);
> + bfd_put_32 (output_bfd, iplt_entry[3] | high , loc + 12);
> + bfd_put_32 (output_bfd, iplt_entry[4], loc + 16);
> + bfd_put_32 (output_bfd, iplt_entry[5] | low | load, loc + 20);
> + bfd_put_32 (output_bfd, iplt_entry[6], loc + 24);
> + bfd_put_32 (output_bfd, iplt_entry[7], loc + 28);
> + }
> + else if (ABI_64_P (output_bfd) && igotplt_address > 0xffffffffL)
Same here but with igotplt_address >= 0x80000000
(or ((bfd_vma) 1 << 31), don't mind which)
> + /* Emit an R_MIPS_IRELATIVE relocation against the igot entry. */
> + if (relsect->contents == NULL)
Looks like this block isn't indented correctly.
> + {
> + /* Allocate memory for the relocation section contents. */
> + relsect->contents = bfd_zalloc (dynobj, relsect->size);
> + if (relsect->contents == NULL)
> + {
> + bfd_set_error (bfd_error_no_memory);
> + return FALSE;
> + }
> + }
No need for the bfd_set_error; bfd_zalloc does that itself. Just returning
FALSE is enough.
> + mips_elf_output_dynamic_relocation (output_bfd, relsect,
> + relsect->reloc_count++,
> + 0 /* sym_indx */,
> + R_MIPS_IRELATIVE, igotplt_address);
Would prefer to have a separate increment, since it's easy to miss
in the long call.
> @@ -10568,6 +10913,17 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd *output_bfd,
> sym->st_other = other;
> }
>
> + /* IFUNC symbols get an iplt stub */
> + if (hmips->needs_iplt)
> + {
> + if (!(mips_elf_create_iplt (output_bfd, dynobj, htab, hmips, sym, info)))
> + return FALSE;
No "(" after "!".
> + if (!elf_hash_table (info)->dynamic_sections_created)
> + return TRUE;
> + if (h->dynindx == -1 && !h->forced_local)
> + return TRUE;
Please drop these early exits. Also, please combine this block and:
> @@ -10731,6 +11087,15 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd *output_bfd,
> sym->st_other -= STO_MICROMIPS;
> }
>
> + if (hmips->needs_iplt)
> + {
> + /* Point at the iplt stub for this ifunc symbol. */
> + sym->st_value = htab->siplt->output_section->vma +
> + htab->siplt->output_offset +
> + hmips->iplt_offset;
> + sym->st_info = ELF_ST_INFO (STB_GLOBAL, STT_FUNC);
> + }
this.
> +# STT_GNU_IFUNC testing:
> +#
> +# 1. Dso with ifunc defined code
> +# 2. Dso that references external ifunc'ed routines
> +# 3. Dynamic executable with ifunc defined code
> +# 4. Static executable with ifunc defined and referenced code
> +# 5. Dso with with ifunc defined and referenced code
> +# 6. Dynamic executable with ifunc defined and referenced code
We ought to test PIEs too.
> +# STT_GNU_IFUNC tests.
> +set abis [concat o32 [expr {$has_newabi ? "n32 n64" : ""}]]
> +foreach { abi } $abis {
> + run_ld_link_tests [list \
> + [list \
> + "IFUNC 1 (Simple dso with def) ${abi}" \
> + "$abi_ldflags($abi) -shared -T ifunc-dyn.ld" "" \
> + "$abi_asflags($abi)" \
> + [list ifunc-dyn-def.s] \
> + [list "readelf -Ds libifunc-1-${abi}.sym"] \
> + "libifunc-1-${abi}.so" \
> + ] \
> + [list \
> + "IFUNC 2 (Simple dso with ref) ${abi}" \
> + "$abi_ldflags($abi) -shared -T ifunc-dyn.ld" "" \
> + "$abi_asflags($abi)" \
> + [list ifunc-dyn-ref.s] \
> + [list "readelf -Ds libifunc-2-${abi}.sym"] \
> + "libifunc-2-${abi}.so" \
> + ] \
AFAICT this doesn't see the function as an ifunc, since there's no
DSO input to provide a definition. I think this should link against
libifunc-1-${abi}.so. It might even be worth running the test twice,
once with the DSO before the .o (added after "-T ...") and once with
the DSO after the .o (in the "").
> + [list \
> + "IFUNC 3 (Simple dynamic executable with def) ${abi}" \
> + "$abi_ldflags($abi) -Bdynamic -L./tmpdir -lifunc-2-${abi} -T ifunc-dyn.ld" "" \
> + "$abi_asflags($abi)" \
> + [list ifunc-dyn-main.s ifunc-dyn-def.s] \
> + [list "readelf -Ds ifunc-3-${abi}.sym" \
> + "readelf -r ifunc-3-${abi}.r" \
> + "objdump -dj.iplt ifunc-3-${abi}.t"] \
> + "ifunc-3-${abi}" \
> + ] \
Should have dynamic executable with ref too, along the lines above.
> + [list \
> + "IFUNC 5 (Dynamic shared object with def and ref) ${abi}" \
> + "$abi_ldflags($abi) -shared -T ifunc-dyn.ld" "" \
> + "$abi_asflags($abi) -KPIC" \
> + [list ifunc-dyn-def.s ifunc-dyn-ref.s] \
> + [list "readelf -Ds ifunc-5-${abi}.sym" \
> + "readelf -r ifunc-5-${abi}.r"] \
> + "ifunc-5-${abi}" \
> + ] \
> + [list \
> + "IFUNC 6 (Dynamic executable with def and ref) ${abi}" \
> + "$abi_ldflags($abi) -Bdynamic -L./tmpdir -lifunc-2-${abi} -T ifunc-dyn.ld" "" \
> + "$abi_asflags($abi)" \
> + [list ifunc-dyn-main.s ifunc-dyn-def.s ifunc-dyn-ref.s] \
> + [list "readelf -Ds ifunc-6-${abi}.sym" \
> + "readelf -r ifunc-6-${abi}.r"] \
> + "ifunc-6-${abi}" \
Would be worth testing the reverse orders too: ref before def.
We should also have tests for direct calls (JALs) in dynamic executables.
It would also be good to have tests that include non-call references to
the function, e.g. via %got, .word or (in executables) %hi/%lo.
The easiest way of doing both of the last two would be to write the
asm by hand and use macros to generate multiple functions, with each
function being referenced by a different combination of access patterns.
E.g. see the compressed-plt*.s tests for one possible approach. There's
no need to follow those slavishly, but in general hand-written asm
without cut-&-paste is easier to maintain than compile-generated asm
with all its repetition.
I think the main missing feature I could see was support for local
ifunc symbols, i.e. those that are STB_LOCAL in the input .o.
Thanks,
Richard