Re: [PATCH 1/2] [RFC] Add IFUNC support for MIPS (v4)

[Faraz Shahbazker <faraz dot shahbazker at imgtec dot com>]

On 04/29/16 06:57, Maciej W. Rozycki wrote:
>> 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 */
>> +	 bfd_elf_generic_reloc, /* special_function */> 
>  Also why `bfd_elf_generic_reloc' and not `_bfd_mips_elf_generic_reloc'?  

I copied this bit directly from the previous series of MIPS IFUNC patches,
published a couple of years back. My guess is that it was selected based
on whatever architecture was used as a reference, without considering the 
prevalent convention in MIPS-specific code. Will fix for the next iteration.

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

The local GOT entry could have been created earlier by 
mips_elf_record_[global|local]_got_symbols(), but the gotidx is only assigned
from the general GOT pool, when mips_elf_create_local_got_entry() is called.
The major change here is that IFUNC GOT entries are always tracked uniquely
by the symbol's hash-table entry (d.h), as against other local symbols which
could by tracked by d.addend.

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

We can in fact discard these relocs for all IFUNCs that do not have IPLTs,
since there is no canonical address to use for the optimization in that case.
A related problem to tackle -- these relocs count as static relocs and cause
IPLT stubs to be generated for locally bound IFUNCs, even when they are not
otherwise needed (that is, when all references are GOT-based).

>> @@ -6436,31 +6876,44 @@ mips_elf_create_dynamic_relocation (bfd *output_bfd,
> <snip>
>  No R_MIPS_IRELATIVE/R_MIPS_64 composition for n64?

I defined IRELATIVE to just operate on natural width, since it is operating on GOT
entries. Now that you've mentioned it, I agree that composition would keep things
closer to the ABI spec. The glibc patch will need to be modified as well.

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

I've moved this assignment of ->dynobj in to create_ifunc_sections() and arranged
for that function to be called at the end of _bfd_mips_elf_check_relocs(),
and only if an input object declares IFUNCs. This arrangement makes the
'not-generated-if-not-needed' comment above more accurate.
 
>> @@ -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?

Local IFUNCs are treated like global symbols in that they get a full GOT 
(or IGOT) entry each and all references must go through that GOT entry.
This means that call16/got16 references can theoretically work for local
IFUNCs, although not explicitly sanctioned by the ABI.

I just followed gcc's behaviour instead of referring to the ABI on this
point. gcc does not consider IFUNCs to be locally bound, since the 
resolved function could be external. The mips backend, using this criteria,
considers all IFUNCs to be candidates for lazy evaluation and emits call16
for all PIC IFUNC calls without caring about local vs. global binding.
This trips up the error condition above.

Since you pointed it out, I tinkered around with gcc to force got16 to be
generated for local IFUNCs instead of call16, but ran up against instances 
where it is impossible to know that the symbol is a local IFUNC. For example,
the glibc testsuite has IFUNC declarations like this:

	extern int foo (void);
	static void * foo_ifunc (void) __asm__ ("foo");
	__asm__(".type foo, %gnu_indirect_function");

Both `static' and `ifunc' aspects of foo are hidden in inline assembly
and hence not available to the compiler. I don't see a way around this.
We can modify gcc to generate the right reloc for the canonical
local IFUNC declaration and let ^^those glibc tests fail, till such time
when they are modified to use the canonical form (not an easy sell, given
the pretzel declaration above seems to work for other architectures).

This reminds me, another issue with gcc is that it generates GOT+LO16
sequence for non-call references to local IFUNC instead of just GOT().
The GOT load already gives the address of the IFUNC and the subsequent
addiu corrupts it. I'll upload a gcc patch to address both issues and
copy you on it for comments.

Considering that this is an `extension', I think it would be okay to
allow CALL16 on local IFUNCs. But I'll follow your cue on what to
do about it.

Regards,
Faraz Shahbazker