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: "Maciej W. Rozycki" <macro at imgtec dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, Richard Sandiford <rdsandiford at googlemail dot com>
- Date: Fri, 20 May 2016 20:17:24 -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> <56E9CE2D dot 1000607 at imgtec dot com> <alpine dot DEB dot 2 dot 00 dot 1604280258340 dot 21846 at tp dot orcam dot me dot uk>
On 04/29/2016 07:19 AM, Maciej W. Rozycki wrote:
>>>> @@ -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.
> Would you please give me a reference? All I came across is this:
>> This isn't really a property of the hash table symbol but of the kind of
>> stub being created by the calling code. I think it would be better to
>> pass an "st_other" parameter alongside the other stub properties.
> -- which only comments on *how* to set `other' rather than whether to set
> it at all in the first place.
> Also please answer my original question, that is: what is the intent for
> this setting -- or IOW what changes if you remove it?
Nothing. FWIW, even if both are set here, _bfd_mips_elf_link_output_symbol_hook
takes care of resetting the ISA bit. A similar thing happens with
mips_elf_set_plt_sym_value, for example.
Before this patch, mips_elf_create_stub_symbol was only being used for
la25 stubs for MIPS or micromips and the pre-existing usage prefers to set
the ISA bit. On one hand, I am inclined to stick with that and remove the symbol
annotation. On the other hand, the behaviour of output_symbol_hook() suggests
that annotation is the preferred way of indicating ISA, since it retains that
setting over the ISA bit, when both are set. What would you recommend?
>>> 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.
Finally got your point! Using the referring reloc to choose between regular
& compressed stub makes sense for locally bound IFUNCs. For an exported IFUNC
with external references and non-call relocs, the exported symbol has to
be bound to a specific IPLT stub. Okay to pick the stub from the ISA type
of the resolver in that case?
This change takes the number of IFUNC-related fields in the struct
mips_elf_link_hash_entry to 7 (4 bit-flags and 3 offsets). Would it be better
to pack them all in to a struct which we allocate only for IFUNCs? Should
definitely save memory with large links, especially on 64-bit hosts.
> There are two reasons for regular PLT to have entries sorted.
> First is cache line alignment -- regular PLT entries are 16-byte long
> making them align to cache lines of up to 16 bytes in size, while
> microMIPS entries are 12-byte long (unless `--insn32' has been used)
> guaranteeing alignment to cache lines of up to 4 bytes only. By placing
> all regular entries first they have the guaranteed 16-byte alignment.
> Second is the handling of synthetic symbols which is much easier with
> such an arrangement -- see `_bfd_mips_elf_get_synthetic_symtab'.
> If neither were an issue with IPLT, then you could leave entries in a
> random order. But the IPLT entries also have different lengths, even the
> regular ones, e.g. with n64, so there will be performance implications
> from different ordering.
> Can you see if you can do something about it then? The best would be
> sorting entries in the decreasing alignment order and aligning the section
> according to the largest one. So e.g. given the sizes of 32, 24, 20, 16,
> the order would be: 32, 16, 24, 20, and the section alignment -- 5.
I get the general idea. But given that we're selecting 32 vs. 64/
micromips vs. mips16 and those combinations cannot be inter-linked, I
don't see how we can have more than 2 stub sizes in a given link. Besides
after performing the delay slot optimization that you suggested elsewhere,
all variations of mips32 will be 12 bytes. All variations of mips64 will
be 28-bytes, except mips16/64 which could be 16-bytes.
Also, note that although we have 32/48-bit variants for 64-bit, the IPLT stub
size is still the same as the regular entry. Only the # of instructions that
need to be executed is fewer. The selection is made based on the actual
address bits needed to get to the GOT/IGOT entry of that symbol. I should
probably be filling the tail part of these stubs with NOPs.
The delay slot optimization itself requires all regular entries to be
grouped together. Beyond that, stub sizes would only matter for mips16/64
combination. Okay then, to just sort on regular vs. compressed and keep
all compressed stubs before all regular ones? Compressed stubs generally
don't have the lagging delay-slot problem, except for the micromips_insn32
Related point: Should I go ahead and add all mips64 variants? Right now we
only support regular mips64 - no compressed stubs.