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: 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.  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,
Catherine



> 
> Looks good otherwise.
> 
> Richard

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

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