This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 1/2] [RFC] Add IFUNC support for MIPS (v4)
- From: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Faraz Shahbazker <faraz dot shahbazker at imgtec dot com>
- Cc: <binutils at sourceware dot org>, Richard Sandiford <rdsandiford at googlemail dot com>
- Date: Fri, 29 Apr 2016 14:57:54 +0100
- Subject: Re: [PATCH 1/2] [RFC] Add IFUNC support for MIPS (v4)
- 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> <871tc0acam dot fsf at googlemail dot com> <5678829D dot 4080108 at imgtec dot com> <877fk6jewx dot fsf at googlemail dot com> <568EFE7B dot 60708 at imgtec dot com> <alpine dot DEB dot 2 dot 00 dot 1602101549000 dot 15885 at tp dot orcam dot me dot uk>
Hi Faraz,
Second part of the review. Apologies about the long RTT.
> bfd/ChangeLog:
>
> * elf32-mips.c
> (elf_mips_eh_howto table): Howto for BFD_RELOC_MIPS_IRELATIVE.
This does not correspond to the change made, you're not adding
`elf_mips_eh_howto table'. Also the convention is to make it unambiguous
that this is a new object, e.g.:
* elf32-mips.c (elf_mips_irelative_howto): New variable.
> (bfd_elf32_bfd_reloc_type_lookup): Case for R_MIPS_IRELATIVE.
This is
<http://www.gnu.org/prep/standards/html_node/Indicating-the-Part-Changed.html>
from The GNU Coding Standard, so:
(bfd_elf32_bfd_reloc_type_lookup) <R_MIPS_IRELATIVE>: New case.
> (bfd_elf32_bfd_reloc_name_lookup): Case for R_MIPS_IRELATIVE.
No switch statement here and no mention of R_MIPS_IRELATIVE, so you need
to describe the change a bit. Please remember to use the imperative mood,
as if you were giving orders to code. So e.g.:
(bfd_elf32_bfd_reloc_name_lookup): Handle R_MIPS_IRELATIVE.
> (mips_elf32_rtype_to_howto): Case for R_MIPS_IRELATIVE
Here we have a switch again. Also missing full stop. So:
(mips_elf32_rtype_to_howto) <R_MIPS_IRELATIVE>: New case.
> * elfxx-mips.c
> (struct mips_got_info): New fields general_gotno and
> assigned_general_gotno.
No `struct' in ChangeLog entries + imperative mood, e.g.:
* elfxx-mips.c (mips_got_info): Add fields general_gotno and
assigned_general_gotno.
> (struct mips_elf_link_hash_entry): New fields for offset in to
> IPLT/IGOT, flags to indicate if symbol needs IPLT/IRELOC/IGOT
> and if it has normal GOT-based relocations.
Likewise. No need to describe the purpose of the change (which is out of
scope for ChangeLog), but please list the fields added.
> (mips_elf_link_hash_table): New fields for size of IPLT stubs
> and hash-table for local IFUNC symbols.
Likewise.
> (MIPS16_P): New macro to check ASE flag.
Just:
(MIPS16_P): New macro.
> (mips16_exec_iplt_entry): Template for mips16 IPLT stub.
(mips16_exec_iplt_entry): New variable.
> (mips32_exec_iplt_entry): Template for mips32 IPLT stub.
> (mips32r6_exec_iplt_entry): Template for mips32 R6 IPLT stub.
> (micromips32_exec_iplt_entry): Template for micromips32 stub.
> (mips64_exec_iplt_entry): Template for mips64 IPLT stub.
> (mips64_48b_exec_iplt_entry): Template for mips64 IPLT stub.
Likewise.
> (mips_elf_link_hash_newfunc): Initialization of new
> mips_elf_link_hash_entry elements.
Use the imperative mood.
> (mips_elf_create_stub_symbol): Set ISA bit in address for micromips
> & mips16 IPLT stubs. New argument to set st_other value of stub.
Likewise (in the second sentence).
> (mips_elf_rel_dyn_section): Moved up to avoid forward declaration.
Likewise.
> (mips_elf_record_local_got_symbol): New argument for pointer to
> mips_elf_link_hash_entry of the local symbol. If non-NULL, this
> is used to set d.h in the GOT has entry instead of d.addend.
No need for the explanation, i.e. keep the first sentence only.
NB all such explanations belong to comments within code and/or the commit
description (as applicable).
> (_bfd_mips_elf_finish_dynamic_symbol): Create IPLT stub/IRELATIVE
> relocation for IFUNC symbols as necessary. Set IFUNC symbol value
> to the IPLT entry address for executable objects.
Two spaces after a full stop. Check elsewhere too in case I missed a
place.
> (_bfd_mips_elf_get_target_dtag): Add cases for dynamic tag -
> DT_MIPS_GENERAL_GOTNO.
Again:
(_bfd_mips_elf_get_target_dtag) <DT_MIPS_GENERAL_GOTNO>: New case.
> binutils/ChangeLog:
>
> * readelf.c:
> (get_mips_dynamic_type): Add case for DT_MIPS_GENERAL_GOTNO.
> (dynamic_section_mips_val): Add case for DT_MIPS_GENERAL_GOTNO.
Likewise.
> elfcpp/ChangeLog:
>
> * elfcpp.h
> (enum DT): Add cases for dynamic tag DT_MIPS_GENERAL_GOTNO.
No `enum' in ChangeLog entries + typo (cases vs case).
> include/elf/ChangeLog:
>
> * mips.h
> (START_RELOC_NUMBERS): Entry for R_MIPS_IRELATIVE.
START_RELOC_NUMBERS is not an object containing relocations:
* mips.h (R_MIPS_IRELATIVE) New relocation.
> (DT_MIPS_GENERAL_GOTNO): New dynamic tag.
(DT_MIPS_GENERAL_GOTNO): New macro.
> diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
> index 752f386..e8f1079 100644
> --- a/bfd/elf32-mips.c
> +++ b/bfd/elf32-mips.c
> @@ -1646,6 +1646,22 @@ static reloc_howto_type elf_mips_eh_howto =
> 0xffffffff, /* dst_mask */
> FALSE); /* pcrel_offset */
>
> +/* STT_GNU_IFUNC support. */
> +static reloc_howto_type elf_mips_irelative_howto =
> + HOWTO (R_MIPS_IRELATIVE, /* type */
> + 0, /* rightshift */
> + 2, /* size (0 = byte, 1 = short, 2 = long) */
> + 32, /* bitsize */
> + FALSE, /* pc_relative */
> + 0, /* bitpos */
> + complain_overflow_bitfield,/* complain_on_overflow */
Missing space after the comma here.
> + bfd_elf_generic_reloc, /* special_function */
Use a tab here after the comma.
Also why `bfd_elf_generic_reloc' and not `_bfd_mips_elf_generic_reloc'?
This is a genuine question, perhaps you're right. See also however:
<https://sourceware.org/ml/binutils/2003-05/msg00779.html>,
<https://sourceware.org/ml/binutils/2003-06/msg00338.html>,
<https://sourceware.org/ml/binutils/2003-12/msg00239.html>,
<https://sourceware.org/ml/binutils/2013-05/msg00060.html>.
> diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
> index 4ece819..6f23387 100644
> --- a/bfd/elfxx-mips.c
> +++ b/bfd/elfxx-mips.c
> @@ -392,6 +403,9 @@ struct mips_elf_link_hash_entry
> cannot possibly be made dynamic). */
> unsigned int has_static_relocs : 1;
>
> + /* True if there is a got16 or call16 relocation against this symbol. */
> + unsigned int has_got_relocs : 1;
Hmm, the comment doesn't cover all the relocations possible, perhaps
just: "True if there is a GOT relocation..."?
> @@ -413,6 +427,15 @@ 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;
> +
> + /* Does this symbol need an IPLT stub? */
> + unsigned int needs_igot : 1;
"... an IGOT entry?"?
> @@ -1185,6 +1219,69 @@ static const bfd_vma mips_vxworks_shared_plt_entry[] =
> 0x10000000, /* b .PLT_resolver */
> 0x24180000 /* li t8, <pltindex> */
> };
> +
> +/* The format of MIPS16 o32 IPLT entries. We use v0 ($2)
> + and v1 ($3) as temporaries because t8 ($24) and t9 ($25) are not
> + directly addressable. */
> +static const bfd_vma mips16_exec_iplt_entry[] =
> +{
> + 0xb202, /* lw $2, 8($pc) */
> + 0x9a60, /* lw $3, 0($2) */
> + 0xeb00, /* jr $3 */
> + 0x653b, /* move $25, $3 */
> + 0x0000, 0x0000 /* .word (.igot address) */
> +};
> +
> +/* The format of 32 bit IPLT entries. */
> +static const bfd_vma mips32_exec_iplt_entry[] =
> +{
> + 0x3c0f0000, /* lui $15, %hi(.igot address) */
> + 0x8df90000, /* lw $25, %lo(.igot address)($15) */
> + 0x03200008, /* jr $25 */
> + 0x00000000 /* nop */
> +};
> +
> +/* 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 */
> +};
> +
> +/* The format of 32-bit micromips IPLT entries. */
> +static const bfd_vma micromips32_exec_iplt_entry[] =
> +{
> + 0x41a30000, /* lui $2, %hi(.igot address) */
> + 0xff230000, /* lw $2, %lo(.igot address)($2) */
> + 0x45b9, /* jrc $25 */
> +};
> +
> +/* The format of 64-bit IPLT entries. */
> +static const bfd_vma mips64_exec_iplt_entry[] =
> +{
> + 0x3c0f0000, /* lui $15, %highest(.got.iplt entry) */
> + 0x3c0e0000, /* lui $14, %hi(.got.iplt entry) */
> + 0x25ef0000, /* addiu $15, $15, %higher(.got.iplt entry) */
> + 0x000f783c, /* dsll32 $15, $15, 0x0 */
> + 0x01ee782d, /* daddu $15, $15, $14 */
> + 0xddf90000, /* ld $25, %lo(.got.iplt entry)($15) */
> + 0x03200008, /* jr $25 */
> + 0x00000000, /* nop */
> +};
> +
> +/* The format of 64-bit IPLT entries for 48bit address. */
> +static const bfd_vma mips64_48b_exec_iplt_entry[] =
> +{
> + 0x3c0f0000, /* lui $15, %higher(.got.iplt entry) */
> + 0x25ef0000, /* addiu $15, $15, %high(.got.iplt entry) */
> + 0x000f7c38, /* dsll $15, $15, 16 */
> + 0xddf90000, /* ld $25, %lo(.got.iplt entry)($15) */
> + 0x03200008, /* jr $25 */
> + 0x00000000, /* nop */
> +};
> +
You're adding 64-bit IPLT entries, but where's complementing NewABI
support which would come in bfd/elf64-mips.c and bfd/elfn32-mips.c? Also
what's the difference between (.igot address) (used in 32-bit code) and
(.got.iplt entry) used here?
> @@ -1963,6 +2069,156 @@ mips_elf_add_la25_stub (struct bfd_link_info *info,
> : mips_elf_add_la25_intro (stub, info));
> }
>
> +/* Return the dynamic relocation section. If it doesn't exist, try to
> + create a new one if CREATE_P, otherwise return NULL. Also return NULL
> + if creation fails. */
> +
> +static asection *
> +mips_elf_rel_dyn_section (struct bfd_link_info *info, bfd_boolean create_p)
> +{
> + const char *dname;
> + asection *sreloc;
> + bfd *dynobj;
> +
> + dname = MIPS_ELF_REL_DYN_NAME (info);
> + dynobj = elf_hash_table (info)->dynobj;
> + sreloc = bfd_get_linker_section (dynobj, dname);
> + if (sreloc == NULL && create_p)
> + {
> + sreloc = bfd_make_section_anyway_with_flags (dynobj, dname,
> + (SEC_ALLOC
> + | SEC_LOAD
> + | SEC_HAS_CONTENTS
> + | SEC_IN_MEMORY
> + | SEC_LINKER_CREATED
> + | SEC_READONLY));
> + if (sreloc == NULL
> + || !bfd_set_section_alignment (dynobj, sreloc,
> + MIPS_ELF_LOG_FILE_ALIGN (dynobj)))
> + return NULL;
> + }
> + return sreloc;
> +}
> +
> +/* Return section for IRELATIVE relocations. If the link is dynamic, the
> + relocations should go in .dynrel, otherwise they should go in the special
Not `.rel.dyn'?
> + .rel.iplt section. */
> +
> +static asection *
> +mips_get_irel_section (struct bfd_link_info *info,
> + struct mips_elf_link_hash_table *htab)
> +{
> + asection *srel = (elf_hash_table (info)->dynamic_sections_created)
> + ? mips_elf_rel_dyn_section (info, FALSE)
> + : htab->root.irelplt;
Formatting mistake, close the paren at the end. Also add an empty line
before the declaration and the rest of the function.
> + BFD_ASSERT (srel != NULL);
> + return srel;
> +}
> +
> +/* Reserve space in the rel.iplt section for an IRELATIVE relocation. */
> +
> +static bfd_boolean
> +mips_elf_allocate_ireloc (struct bfd_link_info *info,
> + struct mips_elf_link_hash_table *mhtab,
> + struct mips_elf_link_hash_entry *mh)
> +{
> + asection *srel;
> + bfd *dynobj;
> +
> + srel = mips_get_irel_section (info, mhtab);
> + dynobj = elf_hash_table (info)->dynobj;
> + if (srel != mhtab->root.irelplt && srel->size == 0)
> + {
> + /* Make room for a null element. */
> + srel->size += MIPS_ELF_REL_SIZE (dynobj);
> + ++srel->reloc_count;
> + }
> + srel->size += MIPS_ELF_REL_SIZE (dynobj);
> + mh->needs_ireloc = TRUE;
> +
> + return TRUE;
> +}
> +
> +/* Reserve space in the iplt and igot tables for an ifunc entry
> + and allocate space for an IRELATIVE relocation. */
> +
> +static bfd_boolean
> +mips_elf_allocate_iplt (struct bfd_link_info *info,
> + struct mips_elf_link_hash_table *mhtab,
> + struct mips_elf_link_hash_entry *mh)
> +{
> + asection *s;
> + bfd *abfd = info->output_bfd;
> + unsigned int other = 0;
> +
> + BFD_ASSERT (!mh->needs_iplt);
> + BFD_ASSERT (mhtab->root.iplt != NULL);
> +
> + s = mhtab->root.iplt;
> + if (ELF_ST_IS_MIPS16 (mh->root.other))
> + other = STO_MIPS16;
No need to handle microMIPS annotation here? However you're fetching it
from the ifunc symbol, which as I explained elsewhere, looks wrong to me.
> +
> + mh->iplt_offset = s->size;
> + mips_elf_create_stub_symbol (info, mh, ".iplt.", mhtab->root.iplt,
> + s->size, mhtab->iplt_entry_size, other);
> + s->size += mhtab->iplt_entry_size;
> +
> + BFD_ASSERT (mhtab->root.igotplt != NULL);
> +
> + /* Only create IGOT entry if there are no GOT relocations, or when
> + there are non-CALL references to the symbol. In the latter case,
> + existing GOT entry must point to IPLT, so an IGOT entry is needed
> + to catch the result of the IRELATIVE relocation resolution. */
> + if (!mh->has_got_relocs || mh->root.pointer_equality_needed)
> + {
> + mh->igot_offset = mhtab->root.igotplt->size;
> + mhtab->root.igotplt->size += MIPS_ELF_GOT_SIZE (abfd);
> + mh->needs_igot = TRUE;
> + }
> +
> + mh->needs_iplt = TRUE;
> +
> + /* IRELATIVE fixup will be needed for each local IFUNC. */
> + if (!mips_elf_allocate_ireloc (info, mips_elf_hash_table (info), mh))
> + return FALSE;
> +
> + return TRUE;
> +}
> +
> +/* hash_traverse callback that is called before sizing sections.
> + DATA points to a mips_htab_traverse_info structure. */
This is `htab_traverse', not `hash_traverse'.
> +
> +static bfd_boolean
> +mips_elf_check_ifunc_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 (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;
Separate declaration from the rest of the block with a new line.
> +
> + /* For global symbols, .iplt entry is needed only for all non-shared-
> + objects. For local symbols, it is needed only if the symbol has
> + static relocations. */
> + if (((h->root.forced_local && h->has_static_relocs)
> + || (!h->root.forced_local && !bfd_link_pic (info)))
> + && !mips_elf_allocate_iplt (info, mips_elf_hash_table (info), h))
I'm not sure what you mean by "all non-shared- objects" here. Perhaps:
/* For global symbols, an .iplt entry is needed in non-PIC
binaries. For local symbols, it is needed only if the symbol
has static relocations. */
-- is that right (as always please try to observe the 74-column limit for
comments)? Also I think the condition will be more readable as:
if ((h->root.forced_local ? h->has_static_relocs : !bfd_link_pic (info))
&& !mips_elf_allocate_iplt (info, mips_elf_hash_table (info), h))
> @@ -1975,6 +2231,12 @@ mips_elf_check_symbols (struct mips_elf_link_hash_entry *h, void *data)
> if (!bfd_link_relocatable (hti->info))
> mips_elf_check_mips16_stubs (hti->info, h);
>
> + /* Create stubs and relocations for IFUNC symbols. */
> + if (h
> + && h->root.type == STT_GNU_IFUNC
> + && !mips_elf_check_ifunc_symbols ((void **)&h, hti))
Missing space after the cast here.
> @@ -1986,13 +2248,15 @@ mips_elf_check_symbols (struct mips_elf_link_hash_entry *h, void *data)
> If we're creating a non-PIC relocatable object, mark H as
> being PIC. If we're creating a non-relocatable object with
> non-PIC branches and jumps to H, make sure that H has an la25
> - stub. */
> + stub. IFUNCs with IPLT stubs don't need an la25 stub. */
> if (bfd_link_relocatable (hti->info))
> {
> if (!PIC_OBJECT_P (hti->output_bfd))
> h->root.other = ELF_ST_SET_MIPS_PIC (h->root.other);
> }
> - else if (h->has_nonpic_branches && !mips_elf_add_la25_stub (hti->info, h))
> + else if (h->has_nonpic_branches
> + && (h->root.type != STT_GNU_IFUNC || !h->needs_iplt)
I think this will be easier to parse without the double negation:
&& !(h->root.type == STT_GNU_IFUNC && h->needs_iplt)
> @@ -3254,8 +3487,21 @@ 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)
> - g->local_gotno += 1;
> + else if (entry->symndx >= 0 || (entry->d.h->global_got_area == GGA_NONE))
Gratuitous change, no need for the extra parentheses.
> + {
> + /* Count IFUNCs as general GOT entries with explicit relocations. */
> + if (entry->symndx < 0
> + && entry->d.h->root.type == STT_GNU_IFUNC
> + && entry->d.h->root.def_regular
> + && !entry->d.h->needs_igot)
> + {
> + g->general_gotno += 1;
> + mips_elf_allocate_ireloc (info, mips_elf_hash_table (info),
> + entry->d.h);
> + }
No error checking here?
> + else
> + g->local_gotno += 1;
> + }
> else
> g->global_gotno += 1;
Can you flatten the conditional:
else if (entry->symndx < 0 && entry->d.h->global_got_area != GGA_NONE)
g->global_gotno += 1;
else if (entry->symndx < 0
&& entry->d.h->root.type == STT_GNU_IFUNC
[...]
else
g->local_gotno += 1;
?
> @@ -3587,7 +3833,8 @@ mips_elf_got_page (bfd *abfd, bfd *ibfd, struct bfd_link_info *info,
>
> static bfd_vma
> mips_elf_got16_entry (bfd *abfd, bfd *ibfd, struct bfd_link_info *info,
> - bfd_vma value, bfd_boolean external)
> + bfd_vma value, bfd_boolean external,
> + struct mips_elf_link_hash_entry *h)
> {
> struct mips_got_entry *entry;
>
> @@ -3602,7 +3849,7 @@ mips_elf_got16_entry (bfd *abfd, bfd *ibfd, struct bfd_link_info *info,
> R_MIPS16_GOT16, R_MIPS_CALL16, etc. The format of the entry is the
> same in all cases. */
> entry = mips_elf_create_local_got_entry (abfd, info, ibfd, value, 0,
> - NULL, R_MIPS_GOT16);
> + h, R_MIPS_GOT16);
> if (entry)
> return entry->gotidx;
> else
> @@ -3691,18 +3938,28 @@ mips_elf_create_local_got_entry (bfd *abfd, struct bfd_link_info *info,
> return entry;
> }
>
> - lookup.abfd = NULL;
> lookup.symndx = -1;
> - lookup.d.address = value;
> + if (h && h->root.type == STT_GNU_IFUNC)
> + {
> + lookup.abfd = ibfd;
> + lookup.d.h = h;
> + }
> + else
> + {
> + lookup.abfd = NULL;
> + lookup.d.address = value;
> + }
> +
> loc = htab_find_slot (g->got_entries, &lookup, INSERT);
> if (!loc)
> return NULL;
>
> entry = (struct mips_got_entry *) *loc;
> - if (entry)
> + if (entry && entry->gotidx >= 0)
> return entry;
Why do you need to check `entry->gotidx' now?
> @@ -3715,7 +3972,15 @@ mips_elf_create_local_got_entry (bfd *abfd, struct bfd_link_info *info,
> if (!entry)
> return NULL;
>
> - if (got16_reloc_p (r_type)
> + if (h && h->needs_ireloc && !h->needs_igot)
> + /* Allocate IFUNC slots in the general GOT region since they
> + will need explicit IRELATIVE relocations. */
> + {
> + lookup.gotidx = MIPS_ELF_GOT_SIZE (abfd) * g->assigned_general_gotno++;
Move the comment into the block.
> @@ -5075,6 +5348,65 @@ mips_elf_create_compact_rel_section
> return TRUE;
> }
>
> +/* Create the .iplt, .rel(a).iplt and .igot sections. */
> +
> +static bfd_boolean
> +mips_elf_create_ifunc_sections (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;
> +
> + htab = mips_elf_hash_table (info);
> + dynobj = htab->root.dynobj;
> + bed = get_elf_backend_data (dynobj);
> + flags = bed->dynamic_sec_flags;
> +
> + if (!bfd_link_pic (info))
> + {
> + if (ABI_64_P (dynobj))
> + htab->iplt_entry_size = 4 * ARRAY_SIZE (mips64_exec_iplt_entry);
> + else if (MIPS16_P (dynobj))
> + htab->iplt_entry_size = 2 * ARRAY_SIZE (mips16_exec_iplt_entry);
> + else if (MICROMIPS_P (dynobj))
> + htab->iplt_entry_size = (4 * ARRAY_SIZE (micromips32_exec_iplt_entry)
> + - 2);
> + else
> + htab->iplt_entry_size = 4 * (ARRAY_SIZE (mips32_exec_iplt_entry)
> + + (LOAD_INTERLOCKS_P (dynobj) ? 0 : 1));
Don't bump the slot size for `!LOAD_INTERLOCKS_P' processors, just bump
the size of `.iplt' by one instruction instead, as with regular `.plt'.
It's safe to have LUI $15, ... from the following IPLT entry in the delay
slot of the trailing jump of the previous one, so we just need an extra
NOP at the end of the section for the last entry only. There's no worry
about the beginning of a MIPS16 entry landing in a delay slot because no
`!LOAD_INTERLOCKS_P' processor supports MIPS16 execution, to say nothing
about the uMIPS instruction set.
> +
> + 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;
> +
> + BFD_ASSERT (htab->root.igotplt == NULL);
> +
> + s = bfd_make_section_anyway_with_flags (dynobj, ".igot", flags);
> + if (s == NULL
> + || !bfd_set_section_alignment (dynobj, s, bed->s->log_file_align))
> + return FALSE;
> + htab->root.igotplt = s;
> + mips_elf_section_data (s)->elf.this_hdr.sh_flags
> + |= (SHF_ALLOC | SHF_WRITE);
Why do you need to set these flags explicitly? They are supposed to be
set automagically from BFD flags by the generic ELF backend.
> @@ -5191,6 +5523,74 @@ 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 = abfd->sections;
> + h = ELF_LOCAL_SYMBOL_HASH (sec->id, ELF_R_SYM (abfd, rel->r_info));
> + isym = bfd_sym_from_r_symndx (&htab->sym_cache, abfd,
> + ELF_R_SYM (abfd, rel->r_info));
No error checking here?
> + symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
> + namep = bfd_elf_string_from_elf_section (abfd, symtab_hdr->sh_link,
> + isym->st_name);
> +
> + e.root.indx = sec->id;
> + e.root.dynstr_index = ELF_R_SYM (abfd, rel->r_info);
> + e.root.root.root.string = namep;
> +
> + slot = htab_find_slot_with_hash (htab->loc_hash_table, &e, h, INSERT);
> + if (!slot)
> + return NULL;
> +
> + /* Found match. */
> + if (*slot)
> + {
> + ret = (struct mips_elf_link_hash_entry *) *slot;
> + return ret;
> + }
> +
> + /* Allocate new slot. */
> + ret = (struct mips_elf_link_hash_entry *)
> + objalloc_alloc ((struct objalloc *) htab->loc_hash_memory,
> + sizeof (struct mips_elf_link_hash_entry));
> +
> + if (ret)
> + {
> + memset (ret, 0, sizeof (*ret));
> + ret->root.indx = sec->id;
> + ret->root.dynstr_index = ELF_R_SYM (abfd, rel->r_info);
> + ret->root.dynindx = -1;
> + ret->root.root.root.string = namep;
> + ret->root.root.u.def.section = sec;
> + ret->root.root.u.def.value = isym->st_value;
> + ret->root.got.offset = (bfd_vma) -1;
> + ret->global_got_area = GGA_NONE;
> + ret->root.type = STT_GNU_IFUNC;
> + ret->root.def_regular = 1;
> + ret->root.ref_regular = 1;
> + ret->root.forced_local = 1;
> + ret->root.root.type = bfd_link_hash_defined;
> + ret->igot_offset = -1;
> + ret->root.other = isym->st_other;
> + ret->got_only_for_calls = TRUE;
> +
> + *slot = ret;
> + }
> +
> + return ret;
> +}
>
This is called by `mips_elf_calculate_relocation' immediately below, so
please keep the FF (^L) character separating the parts of the file above
(I'm not sure if these codes still have any value, but let's not mess them
up while we still have them).
> @@ -5321,6 +5723,13 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>
> target_is_16_bit_code_p = ELF_ST_IS_MIPS16 (sym->st_other);
> target_is_micromips_code_p = ELF_ST_IS_MICROMIPS (sym->st_other);
> +
> + if (sym->st_info == STT_GNU_IFUNC)
> + {
> + h = get_local_sym_hash (mips_elf_hash_table (info), input_bfd,
> + relocation);
No error checking here?
> + local_gnu_ifunc_p = TRUE;
> + }
Keep the `target_is_*' settings last in the block, for consistency.
But does MIPS16/uMIPS annotation in `st_other' have the right meaning
here, if any at all? How is it set in GAS for an IFUNC symbol?
> @@ -5545,6 +5954,21 @@ 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
> + && (h->needs_igot
> + || (!call16_reloc_p (r_type)
> + && !got16_reloc_p (r_type))))
> + {
> + 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;
> + }
Need to set `target_is_*' here too? Need to update `sec' accordingly for
later use (e.g. with R_MIPS_32)? -- all other branches of this conditional
do.
> @@ -5640,6 +6064,16 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
> BFD_ASSERT (h->root.needs_plt);
> g = mips_elf_gotplt_index (info, &h->root);
> }
> + /* IFUNCs use the explicitly-relocated GOT region, however we don't
> + distinguish it from the local GOT at this stage. */
> + else if (h && h->needs_ireloc && !h->needs_igot)
> + {
> + g = mips_elf_local_got_index (abfd, input_bfd, info,
> + symbol + addend, r_symndx,
> + h, r_type);
> + if (g == MINUS_ONE)
> + return bfd_reloc_outofrange;
> + }
This block exactly duplicates one in the final `else' statement below.
Can you rewrite code this such as to avoid the duplication? E.g. I think:
else if (local_p
&& !htab->is_vxworks
[...]
else if (!local_p
&& !(h && h->needs_ireloc && !h->needs_igot))
[...]
else
will do.
> @@ -5931,8 +6365,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);
Please merge `symbol + addend' with the first line.
> @@ -6279,6 +6717,7 @@ mips_elf_perform_relocation (struct bfd_link_info *info,
> && r_type == R_MIPS_26
> && (x >> 26) == 0x3) /* jal addr */
> || (JALR_TO_BAL_P (input_bfd)
> + && !ifunc_p
> && r_type == R_MIPS_JALR
> && x == 0x0320f809) /* jalr t9 */
> || (JR_TO_B_P (input_bfd)
Why for JALR only and not JR?
I think it'll be properly handled in `mips_elf_calculate_relocation'
instead, by discarding R_MIPS_JALR (and R_MICROMIPS_JALR, the handling of
which is currently incomplete, but should be fixed) relocations in the
IFUNC case rather than making them reach this point and only noticing they
shouldn't be here in the first place. So please arrange for this to
happen along the case already handled there, rather than here.
> @@ -6436,31 +6876,44 @@ mips_elf_create_dynamic_relocation (bfd *output_bfd,
> if (defined_p && r_type != R_MIPS_REL32)
> *addendp += symbol;
>
> - if (htab->is_vxworks)
> - /* VxWorks uses non-relative relocations for this. */
> - outrel[0].r_info = ELF32_R_INFO (indx, R_MIPS_32);
> + /* Morph REL32 in to IRELATIVE fix-up for local IFUNC reference. */
> + if (h
> + && h->root.type == STT_GNU_IFUNC
> + && SYMBOL_REFERENCES_LOCAL (info, &h->root))
> + {
> + outrel[0].r_info = ELF_R_INFO (output_bfd, 0,
> + R_MIPS_IRELATIVE);
> + outrel[1].r_info = ELF_R_INFO (output_bfd, 0,
> + R_MIPS_NONE);
> + }
> else
> - /* The relocation is always an REL32 relocation because we don't
> - know where the shared library will wind up at load-time. */
> - outrel[0].r_info = ELF_R_INFO (output_bfd, (unsigned long) indx,
> - R_MIPS_REL32);
> -
> - /* For strict adherence to the ABI specification, we should
> - generate a R_MIPS_64 relocation record by itself before the
> - _REL32/_64 record as well, such that the addend is read in as
> - a 64-bit value (REL32 is a 32-bit relocation, after all).
> - However, since none of the existing ELF64 MIPS dynamic
> - loaders seems to care, we don't waste space with these
> - artificial relocations. If this turns out to not be true,
> - mips_elf_allocate_dynamic_relocation() should be tweaked so
> - as to make room for a pair of dynamic relocations per
> - invocation if ABI_64_P, and here we should generate an
> - additional relocation record with R_MIPS_64 by itself for a
> - NULL symbol before this relocation record. */
> - outrel[1].r_info = ELF_R_INFO (output_bfd, 0,
> - ABI_64_P (output_bfd)
> - ? R_MIPS_64
> - : R_MIPS_NONE);
> + {
> + if (htab->is_vxworks)
> + /* VxWorks uses non-relative relocations for this. */
> + outrel[0].r_info = ELF32_R_INFO (indx, R_MIPS_32);
> + else
> + /* The relocation is always an REL32 relocation because we don't
> + know where the shared library will wind up at load-time. */
> + outrel[0].r_info = ELF_R_INFO (output_bfd, (unsigned long) indx,
> + R_MIPS_REL32);
> +
> + /* For strict adherence to the ABI specification, we should
> + generate a R_MIPS_64 relocation record by itself before the
> + _REL32/_64 record as well, such that the addend is read in as
> + a 64-bit value (REL32 is a 32-bit relocation, after all).
> + However, since none of the existing ELF64 MIPS dynamic
> + loaders seems to care, we don't waste space with these
> + artificial relocations. If this turns out to not be true,
> + mips_elf_allocate_dynamic_relocation() should be tweaked so
> + as to make room for a pair of dynamic relocations per
> + invocation if ABI_64_P, and here we should generate an
> + additional relocation record with R_MIPS_64 by itself for a
> + NULL symbol before this relocation record. */
> + outrel[1].r_info = ELF_R_INFO (output_bfd, 0,
> + ABI_64_P (output_bfd)
> + ? R_MIPS_64
> + : R_MIPS_NONE);
> + }
No R_MIPS_IRELATIVE/R_MIPS_64 composition for n64?
As a side note, the comment is wrong -- whoever wrote it misunderstood
the ABI as it clearly states that R_MIPS_REL32/R_MIPS_64 by definition
*is* R_MIPS_REL64, it's not like R_MIPS_64 just sign-extends the addend
passed from R_MIPS_REL32:
"R_MIPS_REL64
This can be produced by composing R_MIPS_REL with
R_MIPS_64."
(R_MIPS_REL is an alias to R_MIPS_REL32). I'll see if I can get with a
better explanation here.
> @@ -7007,6 +7460,8 @@ _bfd_mips_elf_section_processing (bfd *abfd, Elf_Internal_Shdr *hdr)
> hdr->sh_size += hdr->sh_addralign - adjust;
> }
> }
> + else if (strcmp (name, ".igot") == 0)
> + hdr->sh_entsize = MIPS_ELF_GOT_SIZE (abfd);
Why here rather than in `_bfd_mips_elf_fake_sections'?
> @@ -7920,6 +8375,13 @@ _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;
This sets `elf_hash_table (info)->dynobj' in a disguised way. Please
sort out the assignment to `dynobj' immediately above then, and take care
of code later on in this function which has become dead now.
> @@ -8186,7 +8649,23 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
> r_type = ELF_R_TYPE (abfd, rel->r_info);
>
> if (r_symndx < extsymoff)
> - h = NULL;
> + {
> + Elf_Internal_Sym *isym;
> + isym = bfd_sym_from_r_symndx (&htab->sym_cache, abfd, r_symndx);
> +
> + if (isym == NULL)
> + return FALSE;
Separate the declaration from the rest of the block with a new line, and
remove the new line between the assignment and the error check, we don't
normally put one in such places so that the fragment is seen as a whole.
> +
> + /* Relocation is for local STT_GNU_IFUNC symbol. */
> + if (isym->st_info == STT_GNU_IFUNC)
> + {
> + /* Ensure that we have a hash entry for this symbol. */
> + if ((ih = get_local_sym_hash (htab, abfd, rel)) == NULL)
No assigments in selection or iteration statements' conditions please.
> @@ -8384,7 +8870,8 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
> R_MIPS_CALL_HI16 because these are always followed by an
> R_MIPS_GOT_LO16 or R_MIPS_CALL_LO16. */
> if (!mips_elf_record_local_got_symbol (abfd, r_symndx,
> - rel->r_addend, info, r_type))
> + rel->r_addend, info,
> + r_type, NULL))
Please reformat to fit in two lines.
> @@ -8398,7 +8885,8 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
> case R_MIPS_CALL16:
> case R_MIPS16_CALL16:
> case R_MICROMIPS_CALL16:
> - if (h == NULL)
> + /* Exclude local IFUNCs from check. */
> + if (h == NULL && ih == NULL)
> {
> (*_bfd_error_handler)
> (_("%B: CALL16 reloc at 0x%lx not against global symbol"),
Under which circumstances does this condition trigger for local ifuncs?
CALL16 relocations are used for lazy binding and are explicitly defined in
the MIPS psABI for external symbols only. Can the resolver for a local
ifunc be external and bind lazily?
> @@ -8406,6 +8894,10 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
> bfd_set_error (bfd_error_bad_value);
> return FALSE;
> }
> + if (h && h->type == STT_GNU_IFUNC)
> + ((struct mips_elf_link_hash_entry *)h)->has_got_relocs = TRUE;
Missing space after the cast.
> @@ -8459,9 +8958,18 @@ _bfd_mips_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
> }
> else
> addend = rel->r_addend;
> - if (!mips_elf_record_got_page_ref (info, abfd, r_symndx,
> - h, addend))
> + if (ih &&
> + !mips_elf_record_local_got_symbol (abfd, -1, rel->r_addend,
> + info, r_type, ih))
> return FALSE;
> + else if (!mips_elf_record_got_page_ref (info, abfd, r_symndx,
> + h, addend))
> + return FALSE;
Too much indentation here.
> +
> + if (h && h->type == STT_GNU_IFUNC)
> + ((struct mips_elf_link_hash_entry *)h)->has_got_relocs = TRUE;
Missing space after the cast.
> + else if (ih)
> + ih->has_got_relocs = TRUE;
>
> if (h)
> {
Don't you need to handle R_MIPS_GOT_DISP in a similar manner? You've got
it mentioned in your specification even. It's the NewABI's equivalent of
R_MIPS_GOT16, though valid for both local and external symbols.
> @@ -9242,6 +9751,10 @@ _bfd_mips_elf_always_size_sections (bfd *output_bfd,
> hti.error = FALSE;
> mips_elf_link_hash_traverse (mips_elf_hash_table (info),
> mips_elf_check_symbols, &hti);
> +
> + /* Allocate relocs for local IFUNC symbols. */
> + htab_traverse (htab->loc_hash_table, mips_elf_check_ifunc_symbols, &hti);
> +
> if (hti.error)
> return FALSE;
Please bail out separately on each step. Remove a newline between a call
and its error check.
> @@ -9730,6 +10247,8 @@ _bfd_mips_elf_size_dynamic_sections (bfd *output_bfd,
> else if (! CONST_STRNEQ (name, ".init")
> && s != htab->sgot
> && s != htab->sgotplt
> + && s != htab->root.iplt
> + && s != htab->root.igotplt
> && s != htab->sstubs
> && s != htab->sdynbss)
Hmm, it's interesting to note that we have duplicate `sgot' and `sgotplt'
members in the backend -- how odd... The duplication came with x86 ifunc
support: <https://sourceware.org/ml/binutils/2009-06/msg00213.html>,
<https://sourceware.org/ml/binutils/2009-06/msg00228.html> and was missed
or something and in any case not fixed up. This will have to be sorted
out separately of course.
> @@ -9875,6 +10394,13 @@ _bfd_mips_elf_size_dynamic_sections (bfd *output_bfd,
> if (! MIPS_ELF_ADD_DYNAMIC_ENTRY (info, DT_MIPS_PLTGOT, 0))
> return FALSE;
> }
> +
> + if (elf_tdata (output_bfd)->has_gnu_symbols & elf_gnu_symbol_ifunc)
This looks wrong to me, it will return true for `elf_gnu_symbol_unique'
too (which we don't set, but should; I'll fix it up).
> + {
> + if (! MIPS_ELF_ADD_DYNAMIC_ENTRY (info, DT_MIPS_GENERAL_GOTNO, 0))
No space after `!' please.
> @@ -10019,6 +10547,23 @@ _bfd_mips_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
> {
> sec = local_sections[r_symndx];
> h = NULL;
> +
> + Elf_Internal_Sym *isym;
> + struct mips_elf_link_hash_table *htab;
No declarations in the middle of a block please.
> @@ -10046,6 +10591,8 @@ _bfd_mips_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
> continue;
> }
>
> + gnu_ifunc_p = (local_gnu_ifunc_p || (h && h->type == STT_GNU_IFUNC));
No need for the outer parens.
> @@ -10467,6 +11016,242 @@ mips_elf_irix6_finish_dynamic_symbol (bfd *abfd ATTRIBUTE_UNUSED,
> }
> }
>
> +/* Create the contents of the iplt entry for an IFUNC symbol. */
> +
> +static bfd_boolean
> +mips_elf_create_iplt (bfd *output_bfd,
> + struct mips_elf_link_hash_table *htab,
> + struct mips_elf_link_hash_entry *hmips,
> + bfd_vma igotplt_address)
> +{
> + bfd_byte *loc;
> + const bfd_vma *iplt_entry;
> + bfd_vma high = mips_elf_high (igotplt_address);
> + bfd_vma low = igotplt_address & 0xffff;
> +
> + /* Find out where the .iplt entry should go. */
> + if (!htab->root.iplt->contents)
> + {
> + htab->root.iplt->contents = bfd_zalloc (output_bfd,
> + htab->root.iplt->size);
> + if (!htab->root.iplt->contents)
> + return FALSE;
> + }
> + loc = htab->root.iplt->contents + hmips->iplt_offset;
> +
> + /* Fill in the IPLT entry itself. */
> + if (ABI_64_P (output_bfd))
> + {
> + bfd_vma highest = mips_elf_highest (igotplt_address);
> + bfd_vma higher = mips_elf_higher (igotplt_address);
> + iplt_entry = mips64_exec_iplt_entry;
> +
> + if (highest)
> + {
> + /* Full 64-bit address space. */
> + bfd_put_32 (output_bfd, iplt_entry[0] | highest, loc);
> + bfd_put_32 (output_bfd, iplt_entry[1] | high, loc + 4);
> + bfd_put_32 (output_bfd, iplt_entry[2] | higher, loc + 8);
> + bfd_put_32 (output_bfd, iplt_entry[3], loc + 12);
> + bfd_put_32 (output_bfd, iplt_entry[4], loc + 16);
> + bfd_put_32 (output_bfd, iplt_entry[5] | low, loc + 20);
> + bfd_put_32 (output_bfd, iplt_entry[6], loc + 24);
> + bfd_put_32 (output_bfd, iplt_entry[7], loc + 28);
> + }
> + else if (higher)
> + {
> + /* 48-bit address space. */
> + iplt_entry = mips64_48b_exec_iplt_entry;
> + bfd_put_32 (output_bfd, iplt_entry[0] | higher, loc);
> + bfd_put_32 (output_bfd, iplt_entry[1] | high, loc + 4);
> + bfd_put_32 (output_bfd, iplt_entry[2], loc + 8);
> + bfd_put_32 (output_bfd, iplt_entry[3] | low, loc + 12);
> + bfd_put_32 (output_bfd, iplt_entry[4], loc + 16);
> + bfd_put_32 (output_bfd, iplt_entry[5], loc + 20);
> + }
> + else
> + {
> + /* 32-bit address space. */
> + bfd_put_32 (output_bfd, iplt_entry[0] | high, loc);
> + bfd_put_32 (output_bfd, iplt_entry[5] | low, loc + 4);
> + bfd_put_32 (output_bfd, iplt_entry[6], loc + 8);
> + bfd_put_32 (output_bfd, iplt_entry[7], loc + 12);
> + }
> + }
> + else if (ELF_ST_IS_MIPS16 (hmips->root.other))
> + {
> + iplt_entry = mips16_exec_iplt_entry;
> + bfd_put_16 (output_bfd, iplt_entry[0], loc);
> + bfd_put_16 (output_bfd, iplt_entry[1], loc + 2);
> + bfd_put_16 (output_bfd, iplt_entry[2], loc + 4);
> + bfd_put_16 (output_bfd, iplt_entry[3], loc + 6);
> + bfd_put_32 (output_bfd, igotplt_address, loc + 8);
> + }
> + else if (ELF_ST_IS_MICROMIPS (hmips->root.other))
> + {
> + iplt_entry = micromips32_exec_iplt_entry;
> + bfd_put_micromips_32 (output_bfd, iplt_entry[0] | high, loc);
> + bfd_put_micromips_32 (output_bfd, iplt_entry[1] | low , loc + 4);
Extraneous space before last comma.
> +
> + bfd_put_16 (output_bfd, iplt_entry[2], loc + 8);
> + bfd_put_16 (output_bfd, iplt_entry[3], loc + 10);
> + }
> + else
> + {
> + if (MIPSR6_P (output_bfd))
> + iplt_entry = mips32r6_exec_iplt_entry;
> + else
> + iplt_entry = mips32_exec_iplt_entry;
> + bfd_put_32 (output_bfd, iplt_entry[0] | high, loc);
> + bfd_put_32 (output_bfd, iplt_entry[1] | low, loc + 4);
> + if (LOAD_INTERLOCKS_P (output_bfd))
> + {
> + bfd_put_32 (output_bfd, iplt_entry[2], loc + 8);
> + bfd_put_32 (output_bfd, iplt_entry[3], loc + 12);
> + }
> + else
> + {
> + bfd_put_32 (output_bfd, iplt_entry[3], loc + 8);
> + bfd_put_32 (output_bfd, iplt_entry[2], loc + 12);
> + bfd_put_32 (output_bfd, iplt_entry[3], loc + 16);
> + }
> + }
> +
> + return TRUE;
> +}
> +
> +/* Find local GOT index for VALUE. Return -1 if no GOT slot is found. */
> +
> +static bfd_vma
> +mips_elf_check_local_got_index (bfd *abfd, struct bfd_link_info *info,
> + struct mips_elf_link_hash_entry *h)
> +{
> + struct mips_got_entry lookup, *entry;
> + void **loc;
> + struct mips_got_info *g;
> + struct mips_elf_link_hash_table *htab;
> +
> + htab = mips_elf_hash_table (info);
> + BFD_ASSERT (htab != NULL);
> +
> + g = mips_elf_bfd_got (abfd, FALSE);
> +
> + /* Check for existing local GOT entry. */
> + if (g != NULL)
> + {
> + lookup.abfd = abfd;
> + lookup.symndx = -1;
> + lookup.d.h = h;
> + lookup.tls_type = GOT_TLS_NONE;
> + loc = htab_find_slot (g->got_entries, &lookup, NO_INSERT);
> + }
> + else
> + return -1;
> +
> + if (loc && *loc)
> + {
> + entry = (struct mips_got_entry *) *loc;
> + return entry->gotidx;
> + }
> + else
> + return -1;
> +}
> +
> +/* Create the IRELATIVE relocation for an IFUNC symbol. */
> +
> +static bfd_boolean
> +mips_elf_create_ireloc (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)
> +{
> + bfd_vma igotplt_address = 0;
> + int igot_offset = -1;
> + asection *gotsect, *relsect;
> + bfd_vma value = sym->st_value;
> +
> + if (MIPS16_P (output_bfd) || MICROMIPS_P (output_bfd))
> + value |= 1;
As I previously noted this must not be set globally, but I take it you've
taken care of it already with the update.
> +
> + if (!hmips->needs_igot)
> + {
> + gotsect = htab->sgot;
> + /* Check if IFUNC symbol already has an assigned GOT slot. */
> + igot_offset = mips_elf_check_local_got_index (output_bfd, info, hmips);
> + }
> + 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)
> + {
> + /* Allocate memory for the relocation section contents. */
> + relsect->contents = bfd_zalloc (dynobj, relsect->size);
> + if (relsect->contents == NULL)
> + return FALSE;
> + }
> +
> + if (hmips->needs_iplt || SYMBOL_REFERENCES_LOCAL (info, &hmips->root))
> + /* Emit an IRELATIVE relocation against the [I]GOT entry. */
> + mips_elf_output_dynamic_relocation (output_bfd, relsect,
> + relsect->reloc_count++, 0,
> + R_MIPS_IRELATIVE, igotplt_address);
> + else
> + {
> + /* Emit an R_MIPS_REL32 relocation against global GOT entry for
> + a preemptible symbol. */
> + asection *sec = hmips->root.root.u.def.section;
> + Elf_Internal_Rela rel[3];
> +
> + memset (rel, 0, sizeof (rel));
> + rel[0].r_info = ELF_R_INFO (output_bfd, 0, R_MIPS_REL32);
> + rel[0].r_offset = rel[1].r_offset = rel[2].r_offset = igot_offset;
> +
> + mips_elf_create_dynamic_relocation (output_bfd, info, rel, hmips,
> + sec, value, NULL,
> + gotsect);
This needs to be composed with R_MIPS_64 for n64, doesn't it?
> @@ -10957,9 +11755,41 @@ _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->root.iplt->output_section->vma
> + + htab->root.iplt->output_offset
> + + hmips->iplt_offset);
> + sym->st_info = ELF_ST_INFO (STB_GLOBAL, STT_FUNC);
> + if (ELF_ST_IS_COMPRESSED (hmips->root.other))
> + sym->st_value |= 1;
> + }
> +
> return TRUE;
> }
>
> +/* Finish up local dynamic symbol handling. We set the contents of
> + various dynamic sections here. */
Make a note that this is another `htab_traverse' callback.
> +
> +static bfd_boolean
> +_bfd_mips_elf_finish_local_dynamic_symbol (void **slot, void *inf)
> +{
> + struct mips_elf_link_hash_entry *h = (struct mips_elf_link_hash_entry *) *slot;
Wrap this line please.
> + struct bfd_link_info *info = (struct bfd_link_info *)inf;
Missing space after the cast.
> @@ -13849,6 +14690,48 @@ _bfd_mips_elf_relax_section (bfd *abfd, asection *sec,
> return FALSE;
> }
>
> +/* Compute a hash of a local hash entry. We use elf_link_hash_entry
> + for local symbol so that we can handle local STT_GNU_IFUNC symbols
> + as global symbol. We reuse indx and dynstr_index for local symbol
> + hash since they aren't used by global symbols in this backend. */
> +
> +static hashval_t
> +local_htab_hash (const void *ptr)
> +{
> + struct mips_elf_link_hash_entry *h =
> + (struct mips_elf_link_hash_entry *) ptr;
> + return ELF_LOCAL_SYMBOL_HASH (h->root.indx, h->root.dynstr_index);
Add an empty line between the declaration and the rest of the function.
> @@ -15509,7 +16402,9 @@ _bfd_mips_elf_get_target_dtag (bfd_vma dtag)
> return "DT_MIPS_PLTGOT";
> case DT_MIPS_RWPLT:
> return "DT_MIPS_RWPLT";
> - }
> + case DT_MIPS_GENERAL_GOTNO:
> + return "DT_MIPS_GENERAL_GOTNO";
> + }
Don't move the brace.
> @@ -16123,7 +17018,7 @@ _bfd_mips_elf_get_synthetic_symtab (bfd *abfd,
> void
> _bfd_mips_post_process_headers (bfd *abfd, struct bfd_link_info *link_info)
> {
> - struct mips_elf_link_hash_table *htab;
> + struct mips_elf_link_hash_table *htab = NULL;
Why is this change needed?
> @@ -16141,6 +17036,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)
Same note about `elf_gnu_symbol_unique' as above.
Maciej