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


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