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

[Faraz Shahbazker <faraz dot shahbazker 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
>