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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Faraz Shahbazker <faraz.shahbazker@imgtec.com> writes:
> On 11/08/2015 04:46 AM, Richard Sandiford wrote:
>>>>> @@ -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?
>> 
>> Well, it's a question of whether the earlier redirections in the
>> if-else are mutually exclusive with the new one or not, and whether
>> we handle redirections correctly for the ifunc resolver.  (The second
>> part isn't really related to this code.)  E.g.:
>
> I've added few test cases to cover these interactions - I don't think
> these are exhaustive, but at least in theory all mixed call redirections
> must take priority over IPLT redirection because the IPLT stub does not
> address mix-call issues. The ifunc redirection will happen either before 
> (loading the resolved ifunc from GOT) or within the stub by redirection 
> to an IPLT stub.
>
>>   - if a MIPS16 hard float function calls an ifunc, do we correctly
>>     redirect the call to a stub that calls the iplt (or, if the stub
>>     uses a CALL16, indirectly by loading the resolved ifunc from the
>>     GOT)?
> Yes for both.
>
>>   - if there are MIPS16 and non-MIPS16 calls to the same iplt,
>>     what happens?
> Since the IPLT redirection is logically 'behind' the MIPS16 stub
> redirection, it should not matter beyond the selection of the correct
> instruction (jal vs. jalx) depending on the iplt type.
>
>>   - if we have a MIPS16 hard float ifunc F, does the IRELATIVE
>>     resolve the stub for F, or to F itself?
> F itself. IRELATIVE is only concerned with the resolver function.
> The stub in turn will refer to the IFUNC through either the iplt
> or a fixed-up GOT entry.
>
>>   - if a PIC ifunc F (i.e. one that needs $25 to be valid on entry)
>>     is used in a non-PIC executable, does the IRELATIVE relocation
>>     refer to the "la $25" stub for F, or to the original F?
> This is the only case where there is a potential overlap in redirections.
> Since all the iplt stubs load $25, the extra la25 stub is not needed.
> I've chosen to modified mips_elf_check_symbols for the next patch, to 
> not create the redundant la25 stub for functions that have iplt stubs.

Thanks for looking at this.

>>>> 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,
>> 
>> The document you attached earlier where you described the new relocations.
>
> This was included towards the end of the last spec document I had attached. 
> But I will restructure and publish with the next patch.

Ah, missed that, sorry.

>> Also in:
>> 
>> +      if (hmips->needs_iplt || hmips->root.dynindx < 0)
>> +	/* 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 the else is for (A) then "hmips->root.dynindx < 0" doesn't seem
>> strong enough, since it would include (2) in cases where the symbol
>> is also exported.
>
> As I understood, if the symbol is also exported, it will have an entry in 
> the dynamic symbol table and an assigned (> 0) dynamic index. Am I wrong?

Sorry, that was an unfortunate use of "it".  What I meant was that the
else would trigger for (2) with exported symbols (as wall as for (A)),
for exactly the reason you say.  In other words, it looks like we'll
end up with an R_MIPS_REL32 in a case where we should have an
R_MIPS_IRELATIVE.

Thanks,
Richard