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: "Maciej W. Rozycki" <macro at imgtec dot com>
- To: Faraz Shahbazker <faraz dot shahbazker at imgtec dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, Richard Sandiford <rdsandiford at googlemail dot com>
- Date: Fri, 29 Apr 2016 15:19:06 +0100
- 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>
> >> @@ -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?
> > 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.
How do you know that the stub matches the IFUNC given that it is only
resolved at the run time? You could have some regular and some compressed
IFUNC variants provided to choose from, perhaps deliberately.
> 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?
If you have a tail call in an non-PIC executable, it'll be made with a J
(or sometimes B) instruction. This instruction cannot be patched by LD to
make a mode switch, because there is no suitable instruction in the ISA.
Consequently the target of the jump (an IPLT stub in this case) and the
jump itself have to match each other as far as the ISA mode is concerned.
So it's not even a performance issue, but it's a code correctness issue.
There's no J instruction in the MIPS16 instruction set, so you won't
encounter MIPS16 tail calls, but still avoiding a mode switch is
preferable, to prevent a long pipeline stall.
This is not an issue with an IPLT stub jumping to the IFUNC itself,
because it's an indirect (register) jump, so the ISA mode can be switched
if necessary. Also, as I noted above, you don't know if a switch will be
necessary up until the run time. However by optimising to avoid a switch
on jumping to an IPLT stub you're minimising the number of mode switches
required to at most one.
> 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.
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.
> > 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.
The creation conditions are handled elsewhere, but the ISA modes of the
callers are recorded as relocations are seen. This information is then
used to choose the correct kind of PLT entries in one place, and you can
also use it to choose the correct kind of IPLT entries where appropriate
in your code. For why you need it -- please see my note above.
Let me know if you have find anything unclear or have any other questions