Re: [RFC][PATCH] MIPS ifunc for binutils

[Richard Sandiford <rdsandiford at googlemail dot com>]

Jack Carter <Jack.Carter@imgtec.com> writes:
> I attempted to follow the ARM implementation, but the 
> traditional MIPS GOT design forced me to dirverge a bit.
> All ifunc functions are represented in .iplt section stubs and 
> in the .igot.plt section table. Each igot.plt entry has an
> R_MIPS_IRELOCATE relocation record against it with the initial
> igot entry having link time address of the ifunc routine and 
> after the relocation action, the final runtime target routine
> address.
>
> 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.
> This should allow seamless multigot support

Like you said in your earlier message, this seems too big a penalty.
I think instead we should come up with some way of integrating ifuncs
into the ABI's GOT scheme.  E.g. we could add a new dynamic tag that sets
the index of the first local GOT entry, such as DT_MIPS_LOCAL_GOTNO_START.
When that tag is defined, ld.so would use it instead of the current hard-coded
1 or 2 (depending on ELF_MIPS_GNU_GOT1_MASK).

Then the beginning of the primary GOT could be relocated as normal and we'd
have control over the relocation order.  (Of course, we'd only want to use
that area where necessary, since the normal scheme is more efficient for
the cases that it can handle.)

That's just one suggestion.  I'm sure there are other ways of doing it.
But I think we should make whichever change seems best now rather than
treat it as a future enhancement, otherwise we'll be running the risk
of compatibility problems.

Some other more minor comments:

> @@ -741,6 +756,13 @@ 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 *);
> +static bfd_boolean
> +mips_elf_allocate_iplt
> +  (bfd *, struct mips_elf_link_hash_table *, struct bfd_link_info *,
> +   struct mips_elf_link_hash_entry *);

Where possible, please order the functions so that these forward
declarations aren't needed.  (The current ordering is historical
and not ideal...)

> @@ -811,6 +833,11 @@ static bfd *reldyn_sorting_bfd;
>  /* The name of the stub section.  */
>  #define MIPS_ELF_STUB_SECTION_NAME(abfd) ".MIPS.stubs"
>  
> +/* Return the relocation section associated with NAME.  HTAB is the
> +   bfd's elf32_arm_link_hash_entry.  */
> +#define MIPS_ELF_RELOC_SECTION(INFO, NAME) \
> +  (mips_elf_hash_table (INFO)->is_vxworks ? ".rel" NAME : ".rela" NAME)
> +

Looks like this is unused (and also the wrong way around FWIW).

> +/* Is this a non-shared link?  */
> +#define MIPS_STATIC_LINK(info)					\
> +    ((elf_hash_table(info)->dynamic_sections_created) == FALSE)
> +/* Is this a non-fPIC a.out?  */
> +#define MIPS_AOUT_LINK(info)					\
> +    (info->executable == TRUE)

I think it'd be better not to have these and just check the fields directly.

> +/* The format of non-dso IPLT entries.  */
> +static const bfd_vma mips_exec_iplt_entry[] =
> +{
> +  0x3c0f0000,   /* lui $15, %hi(.got.iplt entry)	*/
> +  0x01f90000,   /* l[wd] $25, %lo(.got.iplt entry)($15)	*/
> +  0x03200008,   /* jr $25				*/
> +  0x00000000	/* nop					*/
> +};
> +
> +/* The format of dso IPLT entries.  */
> +static const bfd_vma mips_dso_iplt_entry[] =
> +{
> +  0x03e07821,	/* addu t7, ra, r0				*/
> +  0x04110001,	/* bal 8					*/
> +  0x3c190000,	/* lui t9, %hi(<.got.plt slot offset>)		*/
> +  0x033fc821,	/* addu t9, t9, ra				*/
> +  0x8f390000,	/* l[wd] t9, %lo(<.got.plt slot offset>)(t9) 	*/
> +  0x03200008,	/* jr $t9					*/
> +  0x01e0f821,	/* addu ra, t7, r0				*/
> +  0x00000000	/* nop						*/
> +};

Since we don't use IPLTs for DSOs, just executables, it looks like this
is really a PIE vs. non-PIE distinction.  I think the comments would be
clearer that way.  But why do we need IPLT entries for PIEs but not DSOs?
I'd assumed we didn't need them for DSOs because all calls must go via
the GOT, but that's true of PIEs as well.

I personally don't care much -- in fact argued against it --
but the current PLT entries were chosen to be MIPS I compatible and
so avoided filling the load delay slot.  Whichever way we decide to go,
I think it'd be better for "normal" PLTs and IPLTs to be consistent.

> @@ -1944,6 +2004,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 == FALSE) && (h->root.type == STT_GNU_IFUNC))

Formatting niggle, sorry, but no brackets around == conditions.
Local style is to use !x instead of x == FALSE.

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

Another formatting niggle, but there should be a space before an
opening bracket (other instances too).

> @@ -5261,12 +5394,21 @@ mips_elf_calculate_relocation (bfd *abfd, bfd *input_bfd,
>  	       && h->root.root.u.def.section)
>  	{
>  	  sec = h->root.root.u.def.section;
> -	  if (sec->output_section)
> -	    symbol = (h->root.root.u.def.value
> -		      + sec->output_section->vma
> -		      + sec->output_offset);
> +	  /* If this symbol is an ifunc, point to the iplt stub for it,  */
> +	  if (h->needs_iplt)
> +	    {
> +	      BFD_ASSERT (htab->siplt != NULL);
> +	      symbol = (htab->siplt->output_section->vma
> +		  + htab->siplt->output_offset
> +		  + h->iplt_offset);
> +	    }

This isn't the right place do this, since the surrounding block is just
calculating the symbol value.  If we want to redirect the relocation to
something other than the symbol value, we should do it in the following
block instead (where PLTs are also handled).

> +/* Ifunc variant of mips_elf_create_dynamic_relocation()  */
> +static bfd_boolean
> +ifunc_create_dynamic_relocation(bfd *output_bfd,
> +    struct bfd_link_info *info,
> +    Elf_Internal_Rela *outrel,
> +    struct mips_elf_link_hash_entry *h,
> +    bfd_vma symbol)
> +{
> +  long indx;
> +  bfd_vma t_off;
> +  struct mips_elf_link_hash_table *htab = mips_elf_hash_table (info);
> +  asection *sreloc = mips_elf_rel_dyn_section (info, FALSE);
> +
> +  /* Adjust the output offset of the relocation to reference the
> +	 correct location in the output file.  */
> +  if (h->global_got_area == GGA_NONE)
> +    {
> +      t_off = mips_elf_local_got_index(output_bfd,
> +				       output_bfd,
> +				       info,
> +				       symbol,
> +				       h->root.dynindx,
> +				       h,
> +				       0);
> +    }
> +  else
> +    {
> +      t_off = mips_elf_global_got_index(output_bfd,
> +					info,
> +					output_bfd,
> +					&h->root,
> +					0);
> +    }
> +
> +  t_off += (htab->sgot->output_section->vma +
> +	    htab->sgot->output_offset);
> +
> +  outrel[0].r_offset = t_off;
> +  outrel[1].r_offset = t_off;
> +  outrel[2].r_offset = t_off;
> +
> +  indx = h->root.dynindx;
> +  outrel[0].r_info = ELF_R_INFO (output_bfd, (unsigned long) indx,
> +				 R_MIPS_IRELATIVE);
> +
> +  if (ABI_64_P (output_bfd))
> +    {
> +      (*get_elf_backend_data (output_bfd)->s->swap_reloc_out) (
> +	output_bfd, &outrel[0],
> +	(sreloc->contents
> +	 + sreloc->reloc_count * sizeof (Elf64_Mips_External_Rel)));
> +    }
> +  else
> +    {
> +      bfd_elf32_swap_reloc_out (
> +	output_bfd, &outrel[0],
> +	(sreloc->contents
> +	 + sreloc->reloc_count * sizeof (Elf32_External_Rel)));
> +    }
> +  /* We've now added another relocation.  */
> +  ++sreloc->reloc_count;
> +
> +  return TRUE;
> +}

I didn't really understand this.  Why is the handling so different from
the other dynamic relocations?  We should really relocate the field
specified by "rel", whereas this seems to be finding and relocating a
GOT entry field.  Also, is the global case (h->global_got_area != GGA_NONE)
ever used in practice?  IRELATIVE should only really be for
locally-binding symbols.

> @@ -7695,6 +7942,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;

Formatting should be:

  if (!htab->siplt
      && !mips_elf_create_ifunc_sections (htab->root.dynobj, info))
    return FALSE;

> @@ -8177,10 +8432,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.  */
> +	      if (h->type != STT_GNU_IFUNC)
> +		{
> +		  h->needs_plt = 1;
> +		  h->type = STT_FUNC;
> +		}
> +	      }

I think only the "h->type = STT_FUNC" bit should be conditional.
If a DSO has a call to a preemptible STT_GNU_IFUNC, it needs to use
a normal lazy-binding stub.

Thanks (especially for tackling such a tricky issue!),
Richard