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

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Thursday, May 02, 2013 10:20 AM
> To: Moore, Catherine
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH] [MIPS] Compact EH Infrastructure R_MIPS_PC32
> 
> "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.

I meant that without the jalr_reloc test, the jalr relocations weren't being handled properly.

> 
> 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.

Thanks,
Catherine

Attachment: mips-reloc-pc32.patch3
Description: mips-reloc-pc32.patch3