This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Add IFUNC support for MIPS (v3)


Hi Faraz,

Thanks for the patch.  Some general comments first:

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 don't think this handles multigot correctly, but you probably
alreadly know that :-)

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?

> +/* The format of 32-bit micromips IPLT entries.  */
> +static const bfd_vma micromips32_exec_iplt_entry[] =
> +{
> +  0x41a30000, 	/* lui $2, %hi(.igot address)		*/
> +  0xff230000,	/* ld  $2, %lo(.igot address)($2) 	*/
> +  0x4599,	/* jr  $25				*/
> +  0x0c00,	/* nop					*/
> +};

"lw" rather than "ld".

> @@ -1582,7 +1678,8 @@ mips_elf_create_stub_symbol (struct bfd_link_info *info,
>    struct elf_link_hash_entry *elfh;
>    const char *name;
>  
> -  if (ELF_ST_IS_MICROMIPS (h->root.other))
> +  if (ELF_ST_IS_MICROMIPS (h->root.other)
> +      || (ELF_ST_IS_MIPS16 (h->root.other) && h->root.type == STT_GNU_IFUNC))
>      value |= 1;
>  
>    /* Create a new symbol.  */
> @@ -1598,6 +1695,10 @@ mips_elf_create_stub_symbol (struct bfd_link_info *info,
>    elfh->type = ELF_ST_INFO (STB_LOCAL, STT_FUNC);
>    elfh->size = size;
>    elfh->forced_local = 1;
> +
> +  if (ELF_ST_IS_MIPS16 (h->root.other) && h->needs_iplt)
> +    elfh->other = STO_MIPS16;
> +
>    return TRUE;
>  }
>  

This isn't really a property of the hash table symbol but of the kind of
stub being created by the calling code.  I think it would be better to
pass an "st_other" parameter alongside the other stub properties.

> +/* Return section for R_MIPS_IRELATIVE relocations.  If the link is
[...]
> +/* Reserve space in the rel.iplt section for IRELATIVE relocation.  */

Would be good to be consistent about whether we use R_MIPS_ prefix in
comments or not.  I don't have a preference for which.

s/for/for an/ in the second comment.

> +/* 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;
> +}

There are two copies of this, so please split it out into a subroutine.

Formatting: && should go on the start of the next line.  (The other copy
was correct.)

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.

I think this routine should only allocate a relocation for .igot,
i.e. only in cases where an iplt entry is also allocated.  In other
cases the code is effectively allocating a relocation for the general
GOT area, but there's no guarantee at this stage that the primary GOT
will need such a relocation.  (All references end up in secondary GOTs.)
Instead I think we should allocate GOT relocations at the same time as
counting them, like we do for TLS GOT entries.

Also, returning false on error from a traversal callback isn't enough,
because that just stops the traversal.  We need to indicate to the caller
that a fatal error has occurred so that the caller can return false too.
In mips_elf_check_symbols this is done by setting hti->error.

> @@ -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.

> @@ -3711,7 +3930,8 @@ mips_elf_create_local_got_entry (bfd *abfd, struct bfd_link_info *info,
>    if (entry)
>      return entry;
>  
> -  if (g->assigned_low_gotno > g->assigned_high_gotno)
> +  if (g->assigned_low_gotno > g->assigned_high_gotno ||
> +      g->assigned_general_gotno > g->assigned_low_gotno)
>      {
>        /* We didn't allocate enough space in the GOT.  */
>        (*_bfd_error_handler)

Formatting: || should be on the next line.  I don't think this works
as intended though.  The existing check is OK because we allocate locals
upwards and globals downwards, so checking for overlap is a good test.
General GOT entries and local GOT entries are both allocated upwards,
so checking for overlap there isn't strong enough.

> @@ -3952,7 +4176,11 @@ mips_elf_record_global_got_symbol (struct elf_link_hash_entry *h,
>      }
>  
>    tls_type = mips_elf_reloc_tls_type (r_type);
> -  if (tls_type == GOT_TLS_NONE && hmips->global_got_area > GGA_NORMAL)
> +  /* IFUNC symbols use explicitly relocated GOT region, instead of the ABI
> +     global GOT, but we don't distinguish these from the local GOT region
> +     just yet.  */
> +  if (tls_type == GOT_TLS_NONE && hmips->global_got_area > GGA_NORMAL
> +      && !hmips->needs_ireloc)
>      hmips->global_got_area = GGA_NORMAL;

Formatting: one condition to a line once it gets too long for a single line.
(Yeah, not always followed...)

I don't see how this can trigger though.  mips_elf_record_global_got_symbol
is only called while checking the relocations in the input bfds, to record
things that _might_ need a global GOT entry.  needs_ireloc is only set after
that.

Not needing to change this function would be a good sign.  When looking
at a global symbol in one input bfd, we don't know whether some later bfd
that we haven't analysed yet defines that symbol as an ifunc or not.

> @@ -4398,7 +4626,9 @@ mips_use_local_got_p (struct bfd_link_info *info,
>       local GOT.  This includes symbols that are completely undefined
>       and which therefore don't bind locally.  We'll report undefined
>       symbols later if appropriate.  */
> -  if (h->root.dynindx == -1)
> +  /* Both global & local IFUNC symbols actually use the explicitly relocated
> +     GOT region, but we don't distinguish it from local GOT just yet.  */
> +  if (h->root.dynindx == -1 || h->needs_ireloc)
>      return TRUE;
>  
>    /* Symbols that bind locally can (and in the case of forced-local

s/from local GOT/from the local GOT/.

> @@ -4455,6 +4685,33 @@ mips_elf_count_got_symbols (struct mips_elf_link_hash_entry *h, void *data)
> +/* A elf_link_hash_traverse callback for which INF points to the
> +   link_info structure.  Count the number of GOT entries that need
> +   explicit relocations by iterating over the local hash table.  */
> +
> +static int
> +mips_elf_count_general_got_symbols (void **slot, void *inf)
> +{
> +  struct mips_elf_link_hash_entry *h =
> +    (struct mips_elf_link_hash_entry *) *slot;
> +  struct bfd_link_info *info;
> +  struct mips_elf_link_hash_table *htab;
> +  struct mips_got_info *g;
> +
> +  info = (struct bfd_link_info *) inf;
> +  htab = mips_elf_hash_table (info);
> +
> +  g = htab->got_info;
> +  if (h != NULL && h->needs_ireloc)
> +    g->general_gotno++;
>    return 1;
>  }

Here too I think we should be relying on mips_elf_count_got_entry and
counting GOT entries, rather than counting based on symbols.

> +/* 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;
> +
> +  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;
> +
> +  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))
> +	  /* Multiply by compression ratio for micromips.  */
> +	    htab->iplt_entry_size = 4
> +	      * (ARRAY_SIZE (micromips32_exec_iplt_entry) * 3 / 4);
> +      else
> +	htab->iplt_entry_size = 4 * (ARRAY_SIZE (mips32_exec_iplt_entry)
> +				     + (LOAD_INTERLOCKS_P (dynobj)? 0 : 1));

Space before "?".

> +    }
> +
> +  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;

Do we need to create .iplt and .igot unconditionally when they won't
be used for PIC objects?  If so, please add a comment, otherwise I think
the creation of those sections should be protected by !bfd_link_pic too.

The MIPS16_ and MICROMIPS_p assignments should be indented by a single tab,
like for the other two.

> @@ -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.

> +  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));
> +  symtab_hdr =  &elf_tdata (abfd)->symtab_hdr;

Double space.

> +  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;
> +      if (MIPS16_P (abfd))
> +	ret->root.other = STO_MIPS16;
> +      if (MICROMIPS_P (abfd))
> +	ret->root.other = STO_MICROMIPS;

I don't think this is a safe assumption.  Why not just set root.other
from st_other?

(Remember that abfd is the input here.)

> @@ -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.

> @@ -5649,6 +6053,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 explicit GOT, however we don't distinguish it
> +	     from local GOT at this stage.  */

s/from local GOT/from the local GOT/.  "the explicitly-relocated GOT"
that you used elsewhere seems more descriptive.

> @@ -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.

> @@ -7016,6 +7445,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);
>      }
>  
>    return TRUE;

Double space.

> @@ -7929,6 +8360,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;
> +  if (!htab->root.iplt && !mips_elf_create_ifunc_sections (info))
> +    return FALSE;
> +
>    /* Check for the mips16 stub sections.  */
>  
>    name = bfd_get_section_name (abfd, sec);

After the above comment about not creating .iplt and .igot unnecessarily,
we could use irelplt as the key instead.

> @@ -10088,8 +10587,9 @@ _bfd_mips_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>  		      && mips_elf_local_relocation_p (input_bfd, rel,
>  						      local_sections)))
>  		{
> -		  if (!mips_elf_add_lo16_rel_addend (input_bfd, rel, relend,
> -						     contents, &addend))
> +		  if (!local_gnu_ifunc_p
> +		      && !mips_elf_add_lo16_rel_addend (input_bfd, rel, relend,
> +							contents, &addend))
>  		    {
>  		      if (h)
>  			name = h->root.root.string;

I think conceptually this applies only to the got16_reloc_p case:

	      if (hi16_reloc_p (r_type)
		  || (got16_reloc_p (r_type)
		      && !local_gnu_ifunc_p
		      && mips_elf_local_relocation_p (input_bfd, rel,
						      local_sections)))

Obviously addends don't make much sense for ifuncs, but the LO16 addend
for a HI16 does logically still exist.  We would need to read it if we did
ever add code to raise an error for nonzero addends in future.

> @@ -10476,6 +10976,265 @@ 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)
> +{
> [...]
> +  else
> +    {
> +      if (MIPSR6_P(output_bfd))

Space before "(".

> +/* 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,
> +				bfd_vma value)
> +{
> +  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);
> +  BFD_ASSERT (g != NULL);
> +
> +  /* Check for existing local GOT entry.  */
> +  lookup.abfd = NULL;
> +  lookup.symndx = -1;
> +  lookup.d.address = value;
> +  lookup.tls_type = GOT_TLS_NONE;
> +  loc = htab_find_slot (g->got_entries, &lookup, NO_INSERT);
> +
> +  if (loc && *loc)
> +    entry = (struct mips_got_entry *) *loc;
> +  else
> +    {
> +      /* Need to create and initialize a new GOT entry.  We only get here
> +	 global IFUNC symbol and we need distinct hashes entries for aliased
> +	 symbols.  */
> +      lookup.symndx = h->root.dynindx;
> +      lookup.abfd = abfd;
> +      if (h->root.dynindx < 0)
> +	lookup.d.h = h;
> +      loc = htab_find_slot (g->got_entries, &lookup, INSERT);
> +
> +      if (!loc)
> +	return -1;
> +
> +      entry = (struct mips_got_entry *) bfd_alloc (abfd, sizeof (*entry));
> +      if (!entry)
> +	return -1;
> +
> +      lookup.gotidx = MIPS_ELF_GOT_SIZE (abfd) * g->assigned_general_gotno++;
> +      *entry = lookup;
> +      *loc = entry;
> +      MIPS_ELF_PUT_WORD (abfd, value, htab->sgot->contents + entry->gotidx);
> +    }
> +
> +  return entry->gotidx;
> +}
> +
> +/* 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;
> +
> +  if (!hmips->needs_iplt)
> +    {
> +      gotsect = htab->sgot;
> +      /* Check if IFUNC symbol already has an assigned GOT slots; assign
> +	 a new slot if necessary.  */
> +      igot_offset = mips_elf_check_local_got_index (output_bfd, info,
> +						    hmips, value);
> +      if (igot_offset < 0)
> +	igot_offset = mips_elf_local_got_index (output_bfd, output_bfd, info,
> +						value, hmips->root.dynindx,
> +						hmips, R_MIPS_REL32);
> +    }

I don't think we should be handling this case here.  It's logically
per got entry rather than per symbol, i.e. for multigot we might need
several IRELATIVE GOT relocations.

This is in any case too late to be allocating new GOT entries.
We've already laid out the output object at this point.

> +  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);

Formatting:

      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?

> +	  /* 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.

> +	/* Emit an R_MIPS_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);
> +	}
> +    }
> +  /* If necessary, generate the corresponding .iplt entry.  */
> +  if (hmips->needs_iplt
> +      && !mips_elf_create_iplt (output_bfd, htab, hmips, igotplt_address))
> +    return FALSE;
> +
> +  return TRUE;
> +}
> +
>  /* Finish up dynamic symbol handling.  We set the contents of various
>     dynamic sections here.  */
>  
> @@ -10803,6 +11562,19 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd *output_bfd,
>        sym->st_other = other;
>      }
>  
> +  if (hmips->root.type == STT_GNU_IFUNC)
> +    {
> +      if (hmips->needs_ireloc
> +	  && !mips_elf_create_ireloc (output_bfd, dynobj, htab,
> +				      hmips, sym, info))
> +	return FALSE;
> +
> +      if (!elf_hash_table (info)->dynamic_sections_created)
> +	return TRUE;
> +      if (h->dynindx == -1  && !h->forced_local)

Double space.

> +	return TRUE;
> +    }
> +
>    /* If we have a MIPS16 function with a stub, the dynamic symbol must
>       refer to the stub, since only the stub uses the standard calling
>       conventions.  */
> @@ -10966,9 +11738,39 @@ _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;

Formatting:

      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.  */
> +
> +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;
> +  struct bfd_link_info *info  = (struct bfd_link_info *)inf;
> +  Elf_Internal_Sym isym;
> +
> +  isym.st_value = h->root.root.u.def.section->output_section->vma
> +    + h->root.root.u.def.section->output_offset + h->root.root.u.def.value;

Formatting:

  isym.st_value = (h->root.root.u.def.section->output_section->vma
		   + h->root.root.u.def.section->output_offset
		   + h->root.root.u.def.value);

> +  isym.st_other = h->root.other;
> +  if (ELF_ST_IS_COMPRESSED (isym.st_other))
> +    isym.st_value |= 1;
> +
> +  return _bfd_mips_elf_finish_dynamic_symbol (info->output_bfd, info,
> +					      &h->root, &isym);
> +}
> +
>  /* Likewise, for VxWorks.  */
>  
>  bfd_boolean
> @@ -11354,6 +12156,13 @@ _bfd_mips_elf_finish_dynamic_sections (bfd *output_bfd,
>    sgot = htab->sgot;
>    gg = htab->got_info;
>  
> +  if (htab_elements (htab->loc_hash_table) > 0)
> +  {
> +    /* Fill PLT and GOT entries for local STT_GNU_IFUNC symbols.  */
> +    htab_traverse (htab->loc_hash_table,
> +		   _bfd_mips_elf_finish_local_dynamic_symbol, info);
> +  }
> +

Formatting: "{...}" block needs to be indented two more spaces.

> @@ -13858,6 +14671,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.  */

/* Compute a hash of a local symbol table entry.  We use elf_link_hash_entry
   for local symbols so that we can handle local STT_GNU_IFUNC symbols
   as global symbols.  We reuse indx and dynstr_index for local symbol
   hash since they aren't used by global symbols in this backend.  */

(note indentation)

> +
> +static hashval_t
> +local_htab_hash (const void *ptr)
> +{
> +  struct  mips_elf_link_hash_entry *h =
> +    (struct  mips_elf_link_hash_entry *) ptr;

Stray space after both "struct"s.

> +  return ELF_LOCAL_SYMBOL_HASH (h->root.indx, h->root.dynstr_index);
> +}
> +
> +/* Compare local hash entries.  */
> +
> +static int
> +local_htab_eq (const void *ptr1, const void *ptr2)
> +{
> +  struct mips_elf_link_hash_entry *h1 =
> +    (struct mips_elf_link_hash_entry *) ptr1;
> +  struct  mips_elf_link_hash_entry *h2 =
> +    (struct  mips_elf_link_hash_entry *) ptr2;

Same here.

> +
> +  return h1->root.indx == h2->root.indx &&
> +    h1->root.dynstr_index == h2->root.dynstr_index;

Formatting, should be:

  return (h1->root.indx == h2->root.indx
	  && h1->root.dynstr_index == h2->root.dynstr_index);

> @@ -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.

Thanks,
Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]