This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] [MIPS] Compact EH Infrastructure R_MIPS_PC32
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: "Moore\, Catherine" <Catherine_Moore at mentor dot com>
- Cc: "binutils\ at sourceware dot org" <binutils at sourceware dot org>
- Date: Sat, 04 May 2013 09:32:34 +0100
- Subject: Re: [PATCH] [MIPS] Compact EH Infrastructure R_MIPS_PC32
- References: <FD3DCEAC5B03E9408544A1E416F11242F8F96D19 at NA-MBX-04 dot mgc dot mentorg dot com> <87a9oql7u3 dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F11242F8FB332D at NA-MBX-01 dot mgc dot mentorg dot com> <87li7x1n1q dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <FD3DCEAC5B03E9408544A1E416F11242F8FB496D at NA-MBX-01 dot mgc dot mentorg 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