This is the mail archive of the
mailing list for the binutils project.
Re: [PATCH 1/2] [RFC] Add IFUNC support for MIPS (v4)
- From: Faraz Shahbazker <faraz dot shahbazker at imgtec dot com>
- To: "binutils at sourceware dot org" <binutils at sourceware dot org>, <macro at imgtec dot com>
- Cc: "rdsandiford at googlemail dot com" <rdsandiford at googlemail dot com>
- Date: Wed, 16 Mar 2016 14:20:45 -0700
- Subject: Re: [PATCH 1/2] [RFC] Add IFUNC support for MIPS (v4)
- Authentication-results: sourceware.org; auth=none
- References: <5583540C dot 7070800 at imgtec dot com> <87381jtr31 dot fsf at googlemail dot com> <55899D52 dot 1050000 at imgtec dot com> <87vbeegucz dot fsf at googlemail dot com> <5589AFCD dot 10905 at imgtec dot com> <DCB1C42372B1674B8F912A294CCB775A71680718 at BADAG02 dot ba dot imgtec dot org> <87615awnv8 dot fsf at googlemail dot com> <5600517C dot 1030608 at imgtec dot com> <874mig14xs dot fsf at googlemail dot com> <561D2820 dot 10107 at imgtec dot com> <871tc0acam dot fsf at googlemail dot com> <5678829D dot 4080108 at imgtec dot com> <877fk6jewx dot fsf at googlemail dot com> <568EFE7B dot 60708 at imgtec dot com> <alpine dot DEB dot 2 dot 00 dot 1602101549000 dot 15885 at tp dot orcam dot me dot uk>
On 03/05/2016 11:13 PM, Maciej W. Rozycki wrote:
>> * bfd-in2.h (BFD_RELOC_MIPS_IRELATIVE): New relocation.
> This is a generated file, so please list it at the end and say:
> "Regenerate." only, i.e.:
> * bfd-in2.h: Regenerate.
> You did regenerate it according to the instructions at the top rather than
> hand-editing, right?
Yes. That's how I got the group spacing wrong.
>> @@ -1598,6 +1702,8 @@ mips_elf_create_stub_symbol (struct bfd_link_info *info,
>> elfh->type = ELF_ST_INFO (STB_LOCAL, STT_FUNC);
>> elfh->size = size;
>> elfh->forced_local = 1;
>> + elfh->other = other;
> Why do you need to set `other' while you have already set the ISA bit in
> `value'? Our rule for handling is to set either rather than both at a
> time, why do you need an exception here?
I added `other' based on a suggestion from Richard that it was the preferred
method. The ISA bit setting for MIPS16 was just carried forward from an earlier
patch and can be removed.
> Also you cannot fix IPLT entries based on global MIPS16/microMIPS object
> flags as you may have to create mixed IPLT entries in a single output
> binary if mixed regular/compressed code is present in input. You may even
> have to create two IPLT entries for a single function symbol. Otherwise
> you'll have issues with tail calls and short delay slots and even if you
> escape them you may cause unnecessary mode switches that are costly. So
> you need to decide based on the referring reloc type.
If the type of the stub matches the IFUNC, then there shouldn't be any
unnecessary mode switches. I don't quite see the pitfalls of tails calls
that would require both compressed and regular IPLT for an IFUNC symbol.
Could you point me to an approximate test-case?
I have added mixed-mode handling for IPLT entries and related test-cases
for the next review. Do you see any advantage in having the regular and
compressed entries grouped within the IPLT section, versus just occurring
intermixed as they are encountered? For ordinary PLTs we create all regular
entries before all compressed entries, but I don't see any problem with
mixing them up, as long as the offset and symbol type information is correct.
> Fortunately all the necessary details will have already been collected in
> case ordinary PLT is to be produced. So you can just reuse that stuff for
> your purpose as well.
I don't see much opportunity for reuse, since the conditions that mandate
creation of IPLT entries are distinct from ordinary PLTs, so that code is not
guaranteed to execute in all cases where IPLTs are needed. But I have
restructured this condition code to look similar to PLT handling, including
support for a micromips-insn32 mode. N64/mips16/micromips/insn32 will still be
determined at the global level, but regular vs. compressed will be selected
individually for each entry.