[PATCH] microMIPS/GCC: Fix PIC call relaxation

[Maciej W. Rozycki]

On Wed, 16 Nov 2016, Matthew Fortune wrote:

> > Fix `-mrelax-pic-calls' support for microMIPS code where the relocation
> > produced is supposed to be R_MICROMIPS_JALR rather than R_MIPS_JALR.
> > The lack of short delay support comes from a missed update to this code
> > for microMIPS support and can be relieved as JALRS and JRS instructions
> > can be relaxed to BALS and B instructions respectively, so do that as
> > well.
> 
> Thanks. I didn't follow the background to this optimisation when I added
> the compact branch support so opted to retain the pre-existing behaviour.

 In any case there was no deliberate choice made here, but just a missed 
update along the original microMIPS change.

> > By doing so complement commit r196828 ("microMIPS gcc support"),
> > <https://gcc.gnu.org/ml/gcc-patches/2013-02/msg01103.html>, which is the
> > original change that introduced microMIPS support, in particular to
> > MIPS_CALL, which is where this code previously resided.
> > 
> > Adjust the test suite accordingly, limiting R_MICROMIPS_JALR cases to
> 
> Typo fwiw: Should say R_MIPS_JALR for MIPS code.

 Indeed.  We don't use GIT style commit logs though so I can't correct it 
and your spotting will just stay here for posterity.

> >  NB the use of this feature for microMIPS is limited because short
> > encodings of register jump instructions usually do not have their branch
> > counterparts and long encodings typically are not used.  However at
> > least
> > tail calls can be converted if the jump target is in range, as can calls
> > in `-minsn32' code.  Perhaps we could switch to producing `j[al]r[s].32'
> > in the `-mrelax-pic-calls' mode like GAS does with the `jal' and `j'
> > microMIPS macros in PIC code.
> 
> Does the linker do anything for R_MICROMIPS_JALR currently? From memory
> I seem to think it was mostly ignored. Is there any risk with older linkers
> by introducing R_MICROMIPS_JALR in GCC generated code?

 For the o32 ABI the R_MICROMIPS_JALR reloc is currently effectively 
ignored by the BFD linker, although some preparations for handling have 
been made and therefore there are two switch cases where it appears.

 Actual handling for PIC call relaxation is only included for R_MIPS_JALR 
in `mips_elf_perform_relocation' though, where the actual JALR and JR 
instruction encoding is checked, which is different between the regular 
MIPS and the microMIPS ISAs.

 The presence of R_MIPS_JALR in microMIPS code will lead to an incorrect 
internal `cross_mode_jump_p' setting in `mips_elf_calculate_relocation', 
however this does not cause harm because `mips_elf_perform_relocation' 
code will check instruction encoding.

 For n32 and n64 ABIs the BFD linker handling of R_MIPS_JALR is done in 
`_bfd_mips_relax_section' instead and does not take `cross_mode_jump_p' 
into account, however the instruction encoding is likewise checked, so no 
harm there either.  I plan to remove this function, perhaps before 2.28 
even, as it is duplicated by the corresponding o32 handler now, but 
suffers from considerable bitrot.

 I plan to implement the missing R_MICROMIPS_JALR handling in the BFD 
linker sometime as well, although it may take a little bit yet.  As I 
noted in the original change description, the usable cases are limited, so 
there isn't as much incentive to have this implemented as there was with 
the regular MIPS ISA, which is also probably the reason why this was 
missed from the original microMIPS support patches.

> >  Let me know if the lack of microMIPS results would be a problem for
> > this
> > patch's acceptance.
> 
> Not a problem. We will pick this up as part of testing for the release.
> 
> There is a bit of coding style fuzz in the testcases but it is
> pre-existing from the code you duplicated so I don't think it needs
> fixing.

 I missed that indeed, having copied the test cases verbatim.  I wonder if 
we shouldn't actually factor out these test case bodies to shared .h files 
included from corresponding regular MIPS and microMIPS test cases, and 
then have the formatting fixes applied there instead.

> >  Otherwise, OK to apply?
> 
> OK, as long as you can say there is no risk with the new reloc and older
> linkers.

 As double-checked and documented above I can say there is no risk, so I 
have committed this change now.  Thanks for your review.

  Maciej