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

[Richard Sandiford <rdsandiford at googlemail dot com>]

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> 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.
>
> I meant that without the jalr_reloc test, the jalr relocations weren't
> being handled properly.

Sorry, I still don't get it.  We're talking about the difference between:

  if (HAVE_IN_PLACE_ADDENDS)
    {
      BLAH;
    }

which you said didn't handle jalr relocs correctly, and:

  if (HAVE_IN_PLACE_ADDENDS
      && (fixp->fx_pcrel || jalr_reloc_p (fixp->fx_r_type)))
    {
      BLAH;
    }

where jalr_reloc_p returns true for jalr relocs.  Aren't the two the same
for jalr relocs?  The same BLAH is executed in both cases.  I suppose it
doesn't matter now though.

>> 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;
>> 
>> ?
>> 
> Yes.  This looks much better.  A new version of the patch is attached,
> although I would like to commit the mips_fix_adjustable/limited_pc_reloc
> change separately from the relocation patch, if that's okay.

Either way's fine.  Patch is OK with a suitable changelog.

Thanks,
Richard