This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [RFC] Add IFUNC support for MIPS (v3)
- From: Faraz Shahbazker <faraz dot shahbazker at imgtec dot com>
- To: "binutils at sourceware dot org" <binutils at sourceware dot org>, <rdsandiford at googlemail dot com>
- Date: Mon, 2 Nov 2015 12:40:42 -0800
- Subject: Re: [RFC] Add IFUNC support for MIPS (v3)
- Authentication-results: sourceware.org; auth=none
- References: <5583540C dot 7070800 at imgtec dot com> <87381jtr31 dot fsf at googlemail dot com> <55899D52 dot 1050000 at imgtec dot com> <87vbeegucz dot fsf at googlemail dot com> <5589AFCD dot 10905 at imgtec dot com> <DCB1C42372B1674B8F912A294CCB775A71680718 at BADAG02 dot ba dot imgtec dot org> <87615awnv8 dot fsf at googlemail dot com> <5600517C dot 1030608 at imgtec dot com> <874mig14xs dot fsf at googlemail dot com> <561D2820 dot 10107 at imgtec dot com>
Bump.
On 10/13/2015 08:49 AM, Faraz Shahbazker wrote:
> Hi Richard,
>
> Thank you for the detailed review. I've incorporated most of your suggestions.
> I need clarification on a few points, before I re-post the patch.
>
>
> On 09/27/2015 04:23 AM, Richard Sandiford wrote:
>> I know you borrowed it from the x86 port, but I'm not really a fan of
>> (ab?)using link_hash_entry for local symbols. It just seems likely
>> to create confusion when so many of the fields don't apply or don't
>> carry useful information, especially with the hack:
>>
>> We reuse indx and dynstr_index for local symbol hash since they
>> aren't used by global symbols in this backend.
>>
>> I prefer the way the powerpc port handles it (which I borrowed for
>> AArch32). I won't ask you to change it though.
>
> I did look around a bit, but could relate best to x86. There were too many
> unrelated [to IFUNC] differences in the other architectures.
>
>> I don't think this handles multigot correctly, but you probably
>> alreadly know that :-)
>
> Yes.
>
>> Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
>>> +/* Format of 32 bit IPLT entries for R6. JR encoding differs. */
>>> +static const bfd_vma mips32r6_exec_iplt_entry[] =
>>> +{
>>> + 0x3c0f0000, /* lui $15, %hi(.igot address) */
>>> + 0x8df90000, /* lw $25, %lo(.igot address)($15) */
>>> + 0x03200009, /* jr $25 */
>>> + 0x00000000 /* nop */
>>> +};
>> No need to change, just curious: why not use JRC here?
>
> I assume you meant this for the micromips32 stub? Will modify it.
>
>>> +/* hash_traverse callback that is called before sizing sections.
>>> + DATA points to a mips_htab_traverse_info structure. */
>>> +
>>> +static bfd_boolean
>>> +mips_elf_check_local_symbols (void **slot, void *data)
>>> +{
>>> + struct mips_htab_traverse_info *hti =
>>> + (struct mips_htab_traverse_info *) data;
>>> + struct mips_elf_link_hash_entry *h =
>>> + (struct mips_elf_link_hash_entry *) *slot;
>>> +
>>> + /* If the referenced symbol is ifunc, allocate an iplt for it. */
>>> + if (h && !h->needs_iplt && h->root.type == STT_GNU_IFUNC
>>> + && h->root.def_regular)
>>> + {
>>> + struct bfd_link_info *info = hti->info;
>>> + elf_tdata (info->output_bfd)->has_gnu_symbols |= elf_gnu_symbol_ifunc;
>>> +
>>> + /* .iplt entry is needed only for executable objects. */
>>> + if (!bfd_link_pic (info) &&
>>> + !mips_elf_allocate_iplt (info, mips_elf_hash_table (info), h))
>>> + return FALSE;
>>> +
>>> + /* IRELATIVE fixup will be needed for each local IFUNC. */
>>> + if (!mips_elf_allocate_ireloc (info, mips_elf_hash_table (info), h))
>>> + return FALSE;
>>> + }
>>> +
>>> + return TRUE;
>>> +}
>>
>> I might be missing something, but I think for local symbols the
>> condition for whether an IPLT is needed should be h->has_static_relocs
>> rather than !bfd_link_pic. E.g., if an executable uses GOT and CALL
>> relocs for all references to a symbol, it would be more efficient not
>> to have an IPLT and simply use an entry in the general GOT area
>> with an IRELATIVE relocation against it.
>
> has_static_relocs does not currently track local IFUNCs or relocations in
> data sections. The first seems easy to fix, but I couldn't figure out the latter.
>
>>> @@ -3263,9 +3478,12 @@ mips_elf_count_got_entry (struct bfd_link_info *info,
>>> entry->symndx < 0
>>> ? &entry->d.h->root : NULL);
>>> }
>>> - else if (entry->symndx >= 0 || entry->d.h->global_got_area == GGA_NONE)
>>> + /* Skip IFUNCs from local/global GOT, they are already counted as general
>>> + GOT entries with explicit relocations. */
>>> + else if (entry->symndx >= 0 || (entry->d.h->global_got_area == GGA_NONE
>>> + && !entry->d.h->needs_ireloc))
>>> g->local_gotno += 1;
>>> - else
>>> + else if (!entry->d.h->needs_ireloc)
>>> g->global_gotno += 1;
>>> }
>>
>> I think this is the place where we should be setting general_gotno,
>> rather than mips_elf_count_got_symbols. mips_elf_count_got_symbols
>> is supposed to:
>>
>> Count the number of global symbols that are in the primary GOT only
>> because they have relocations against them (reloc_only_gotno).
>>
>> and shouldn't be counting explicit GOT entries.
>
> Are we not expected to have a GOT entry for a global IFUNC symbol if there are
> no references to it (say within a shared library)? Something in your discussions
> with jcarter led me to believe that this was expected. If not it can removed.
> As it is, these entries do not factor in to the ABI global GOT mechanism.
>
>>> @@ -5200,6 +5515,75 @@ mips_elf_relocation_needs_la25_stub (bfd *input_bfd, int r_type,
>>> return FALSE;
>>> }
>>> }
>>> +
>>> +/* Find and/or create a hash entry for local symbol. */
>>> +
>>> +static struct mips_elf_link_hash_entry *
>>> +get_local_sym_hash (struct mips_elf_link_hash_table *htab,
>>> + bfd *abfd, const Elf_Internal_Rela *rel)
>>> +{
>>> + struct mips_elf_link_hash_entry e, *ret;
>>> + asection *sec;
>>> + hashval_t h;
>>> + void **slot;
>>> + Elf_Internal_Sym *isym;
>>> + Elf_Internal_Shdr *symtab_hdr;
>>> + char *namep;
>>> +
>>> + sec = bfd_get_section_by_name (abfd, ".text");
>>
>> I'm not sure it's valid to assume that every input has a .text section.
>> At least we should have some error code if it doesn't.
>>
>> But I suppose this comes back to my objection to the "local hash" stuff.
>> It just seems wrong to pick an arbitrary section index, regardless of
>> where the symbol is actually defined.
>
> I see the pitfall of ".text". I think I'll stick to the i386 method, which is
> to pick the first section id.
>
>>> @@ -5554,6 +5947,17 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>>> target_is_16_bit_code_p = !micromips_p;
>>> target_is_micromips_code_p = micromips_p;
>>> }
>>> + /* If this symbol is an ifunc, point to the iplt stub for it. */
>>> + else if (h && h->needs_iplt)
>>> + {
>>> + BFD_ASSERT (htab->root.iplt != NULL);
>>> + symbol = (htab->root.iplt->output_section->vma
>>> + + htab->root.iplt->output_offset
>>> + + h->iplt_offset);
>>> + /* Set ISA bit in address for compressed code. */
>>> + if (ELF_ST_IS_COMPRESSED (h->root.other))
>>> + symbol |= 1;
>>> + }
>>
>> I think you need to set target_is_16_bit_code_p and
>> target_is_micromips_code_p too (based on the equivalent conditions
>> for abfd).
>>
>> I'm a bit nervous about how this interacts with the other redirections,
>> e.g. for la25 and mips16 hard-float stubs.
>
> I will dig further in to this. Any hints on what I should look for?
>
>>> @@ -5940,8 +6354,11 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>>> R_MIPS*_GOT16; every relocation evaluates to "G". */
>>> if (!htab->is_vxworks && local_p)
>>> {
>>> + /* Local IFUNC symbols must be accessed through GOT, similar to
>>> + global symbols, to allow for indirection. */
>>> value = mips_elf_got16_entry (abfd, input_bfd, info,
>>> - symbol + addend, !was_local_p);
>>> + symbol + addend,
>>> + !was_local_p || local_gnu_ifunc_p, h);
>>> if (value == MINUS_ONE)
>>> return bfd_reloc_outofrange;
>>> value
>>
>> I agree this is correct, but I think it should be specified explicitly
>> in the ABI document that R_MIPS_GOT16 relocations against local IFUNC
>> symbols are treated in this way. Using the usual R_MIPS_GOT16/R_MIPS_LO16
>> pair for local symbols would give the wrong result for ifuncs.
>
> I am not quite sure which ABI document you mean, but we are planning a change
> in gcc to reflect this. It shouldn't be generating GOT+LO sequences for IFUNCs.
> In theory, LO16(ifunc) is nonsensical for PIC, but we can force it to zero
> just to be defensive.
>
>>
>>> + else
>>> + {
>>> + bfd_byte *loc;
>>> + bfd_vma igot_index;
>>> + gotsect = htab->root.igotplt;
>>> + igot_offset = hmips->igot_offset;
>>> +
>>> + /* Calculate the address of the IGOT entry. */
>>> + igot_index = igot_offset / MIPS_ELF_GOT_SIZE (dynobj);
>>> +
>>> + if (!gotsect->contents)
>>> + {
>>> + gotsect->contents = bfd_zalloc (output_bfd, gotsect->size);
>>> + if (!gotsect->contents)
>>> + return FALSE;
>>> + }
>>> +
>>> + /* Initially point the .igot entry at the IFUNC resolver routine. */
>>> + loc = (bfd_byte *) gotsect->contents
>>> + + igot_index * MIPS_ELF_GOT_SIZE (dynobj);
>>> +
>>> + if (ABI_64_P (output_bfd))
>>> + bfd_put_64 (output_bfd, value, loc);
>>> + else
>>> + bfd_put_32 (output_bfd, value, loc);
>>> + }
>>> +
>>> + igotplt_address = (gotsect->output_section->vma + gotsect->output_offset
>>> + + igot_offset);
>>> +
>>> + relsect = mips_get_irel_section (info, htab);
>>> +
>>> + if (igot_offset >= 0)
>>> + {
>>> + if (hmips->needs_iplt && relsect->contents == NULL)
>>> + {
>>
>> Isn't needs_iplt always true in this case?
>
> No. The code is generalized so that igot_offset could mean got_offset or
> igot_offset. For the former, needs_plt is false. In other words, needs_iplt implies
> (igot_offset >= 0), but not the converse. OTOH, memory for contents is allocated
> as usual in _bfd_mips_elf_size_dynamic_sections so I could just replace this block
> with an assertion.
>
>>> + /* Allocate memory for the relocation section contents. */
>>> + relsect->contents = bfd_zalloc (dynobj, relsect->size);
>>> + if (relsect->contents == NULL)
>>> + return FALSE;
>>> + }
>>> +
>>> + if (hmips->needs_iplt || hmips->root.dynindx < 0)
>>
>> Same here. Unless I'm missing something, the preemptible case should
>> never actually be used for MIPS, since we only have iplts for executables.
>
> As above. igot_offset is actually [i]got_offset. Pre-emption is needed for
> global IFUNC which is defined and referenced in a shared libraries. In that
> case there would be a general GOT entry with a symbolic REL32 fixup against it.
>
>>> @@ -16130,6 +16997,9 @@ _bfd_mips_post_process_headers (bfd *abfd, struct bfd_link_info *link_info)
>>> if (mips_elf_tdata (abfd)->abiflags.fp_abi == Val_GNU_MIPS_ABI_FP_64
>>> || mips_elf_tdata (abfd)->abiflags.fp_abi == Val_GNU_MIPS_ABI_FP_64A)
>>> i_ehdrp->e_ident[EI_ABIVERSION] = 3;
>>> +
>>> + if (elf_tdata (abfd)->has_gnu_symbols & elf_gnu_symbol_ifunc)
>>> + i_ehdrp->e_ident[EI_ABIVERSION] = 4;
>>
>> Is it worth also making this explicitly dependent on whether
>> DT_MIPS_GENERAL_GOTNO is present? It doesn't make a difference now,
>> since it's a subcondition of whether ifuncs are used, but it might
>> be more future-proof.
>
> Yes, I think that would be good. As things stand, the higher ABI version also
> gets attached to static executables where we don't really care about it.
>
> Regards,
> Faraz Shahbazker
>