Re: [PATCH] [MIPS] Compact EH Infrastructure R_MIPS_PC32

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
>> Sent: Monday, April 22, 2013 2:49 PM
>> Subject: Re: [PATCH] [MIPS] Compact EH Infrastructure R_MIPS_PC32
>> 
>> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> > @@ -17782,6 +17787,10 @@
>> >    if (fixp->fx_addsy == NULL)
>> >      return 1;
>> >
>> > +  /* Alow relocs used for EH tables.  */  if (fixp->fx_r_type ==
>> > + BFD_RELOC_32_PCREL)
>> > +    return 1;
>> > +
>> >    /* If symbol SYM is in a mergeable section, relocations of the form
>> >       SYM + 0 can usually be made section-relative.  The mergeable data
>> >       is then identified by the section offset rather than by the symbol.
>> 
>> I think this is really trying to circumvent the later:
>
> That is likely true, but
>> 
>>   /* There is no place to store an in-place offset for JALR relocations.
>>      Likewise an in-range offset of PC-relative relocations may overflow
>>      the in-place relocatable field if recalculated against the start
>>      address of the symbol's containing section.  */
>>   if (HAVE_IN_PLACE_ADDENDS
>>       && (fixp->fx_pcrel || jalr_reloc_p (fixp->fx_r_type)))
>>     return 0;
>> 
>> I'm not sure it's correct for ELF64 though, where the range of the relocation is
>> still smaller than the address range.  I realise the cases where it matters are
>> very much corner cases, but it still seems wrong.
>> 
>> Maybe something like:
>> 
>>   /* In general, converting to a section-relative relocation can
>>      increase the addend.  Make sure that we won't lose bits.  */
>>   if (HAVE_IN_PLACE_ADDENDS)
>>     {
>>       reloc_howto_type *howto;
>>       bfd_vma addr_mask;
>> 
>>       howto = bfd_reloc_type_lookup (stdoutput, fixp->fx_r_type);
>>       addr_mask = ((bfd_vma) 1 << (HAVE_64BIT_OBJECTS ? 63 : 31) << 1) - 1;
>>       if ((howto->src_mask & addr_mask) != addr_mask)
>>         return 0;
>>     }
>> 
>> would be better (completely untested).
>
> This suggestion isn't correct either.

Oh well, thanks for trying.

> This fails to convert relocs that have a src_mask that is less than 32
> bits such as R_MIPS_LO16.  It also incorrectly converts JALR
> relocations.  Did you really mean to say:
>
> If (HAVE_IN_PLACE_ADDENDS
>     && (fixp->fx_pcrel || jalr_reloc (fixp->fx_r_type))
>
> For the initial condition?  Testing with this looks good. 

Thanks for assuming that I meant something sensible, but I'm afraid
I really did mean the original :-)  I should have predicted the problem
with it though.

I don't understand the "also incorrectly converts JALR relocations"
bit though.  If so, how does keeping the jalr_reloc test help?
I can imagine the same problem applies to JAL relocations.

The blanket fixp->fx_pcrel is in that case probably wrong too.
Although one isn't defined at the moment, there's no reason in
principle why we couldn't have HI/LO pairs for PC-relative relocs.

So, how about:

/* Return true if RELOC is a PC-relative relocation that does not have
   full address range.  */

static inline bfd_boolean
limited_pcrel_reloc_p (bfd_reloc_code_real_type reloc)
(
  switch (reloc)
    {
    case BFD_RELOC_16_PCREL_S2:
    case BFD_RELOC_MICROMIPS_7_PCREL_S1:
    case BFD_RELOC_MICROMIPS_10_PCREL_S1:
    case BFD_RELOC_MICROMIPS_16_PCREL_S1:
      return TRUE;

    case BFD_RELOC_32_PCREL:
      return HAVE_64BIT_ADDRESSES;

    default:
      return FALSE;
    }
}

in the "jalr_reloc_p" part of tc-mips.c and:

  /* There is no place to store an in-place offset for JALR relocations.
     Likewise an in-range offset of limited PC-relative relocations may
     overflow the in-place relocatable field if recalculated against the
     start address of the symbol's containing section.  */
  if (HAVE_IN_PLACE_ADDENDS
      && (limited_pcrel_reloc_p (fixp->fx_r_type)
	  || jalr_reloc_p (fixp->fx_r_type)))
    return 0;

?

Thanks,
Richard