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: Tue, 3 Dec 2013 00:25:02 +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>
Responses embedded below.
I will hopefully have the next patch for review with the suggested changes
by the end of the week. Hope is eternal :-)
Thanks again for taking the time to review this,
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
I think I misunderstood what you were saying on my first read, but now think it
is reasonable.
Basically if the ifunc routine is defined locally it is to go into the local got table
and have its location and size defined by dynamic tags. Probably just one that
will state the size and have it follow directly after LOCAL_GOTNO.
I will implement it and see how it works. It will mean that these got entries will be
separated and laid out apart from the other local got entries.
>
> 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.
Done
>
> > @@ -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.)
Done
Once I get past this ifunc patch series, I could do some cleanup and possible refactoring.
It would help familiarizing me with how binutils links Mips and beat the formatting and style
rules into me.
> > @@ -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 *);
Nits demonstrate a thoughtful review. They are also a repetitive training mechanism.
Done
> > @@ -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.
Done
> > @@ -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.
Done
> > + 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 ",".
Done
> > + 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.
Done
> > @@ -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.
Done
> > @@ -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.
Done
> > +
> > + 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.
Done
> > + 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.
Done
> > @@ -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);
Done
> > @@ -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".
Done
> > @@ -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.
Done
> > +{
> > + 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 ?:).
Done
> > + 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...
Don't be sorry. I should be sorry for the mistakes.
Done
> > @@ -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.
This is really hard for me because I have been doing the opposite for many decades
and it just doesn't look right to me, but I must learn to play by the various open source rules.
Done
> > @@ -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.
I have a small suite of runnable tests for ifunc. It is not exhaustive yet, but it does test
for preemption, at least modestly. I made the change you suggested and it still passes.
Done
> > @@ -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;
I get the point about early returns. Still mulling how to deal with this one though.
This is tied with the early exits in finish_dynamic_symbol(). They assume we are
playing with the got.
>
> 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;
Done
> > @@ -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 "/".
Done
> > + 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.
I am assuming that you mean function and not macro.
Done
> > + /* 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.
Once again, old dog new tricks. I will scan the diff carefully for other instances.
Done
> > + /* 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".
Done
> > + loc = (bfd_byte *)gotsect->contents +
>
> Space after ")".
Done
> > + 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.
Done
> > + 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.
Done
> > + 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))
Good idea
Done
> > + 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)
Prefer the latter for consistency
Done
> > + /* Emit an R_MIPS_IRELATIVE relocation against the igot entry. */
> > + if (relsect->contents == NULL)
>
> Looks like this block isn't indented correctly.
Done
> > + {
> > + /* 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.
Done
> > + 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.
Done
> > @@ -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 "!".
Done
> > + 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.
Here I have a problem. This function was not written with static links in mind.
I have tried to combine and move the code, but it always ends in failure. Also,
ifunc and forced local ends up in here. I don't understand this well, but it didn''t need
a dynamic symbol before ifunc and now I do.
If you really don't want the early exit, I could set a flag and skip all the rest
of the code except the return. This would affect indentation for a lot of code
in the function.
Also, would you prefer mips_elf_create_iplt() to be void and assert upon failure?
> > +# 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.
I have no objection to the test suggestions and am in the progress of creating
them.
There are a few failures I am having that although they might be expected, bother me.
Maybe you have a suggestion to their solution:
When the resolver and the functions it resolve to are external to each
other. I believe this is suppose to be illegal, but hard to debug for the unsuspecting.
The resolver is in a dso loaded before the dso with functions it resolves to.
Jack
>
> Thanks,
> Richard