RE: [PATCH, MIPS] Relax alignment check for LDPC

[Matthew Fortune <Matthew dot Fortune at imgtec dot com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> > The R6 LDPC instruction requires an 8-byte aligned load address but
> > does not require LDPC itself to be 8-byte aligned. Unfortunately the
> > tests for LPDC were aligning the LDPC instruction so did not catch
> > this issue.
> >
> > The alignment check for BFD_RELOC_MIPS_18_PCREL_S3 was incorrectly
> > using the valP value as the target address of the reloc when it is
> > actually the calculated value to be patched into an instruction.
> > Since BFD_RELOC_MIPS_18_PCREL_S3 is recorded as relative to the
> > current PC rather than PC & ~7 then valP can end up as a multiple of 4
> > rather than 8 even if the target address is correctly 8-byte aligned.
> > Instead of recalculating the true target address from the fixup I just
> > check that valP is a 4-byte multiple leaving the linker to do the
> > accurate check as it could end up misaligning the target anyway.
> > [...]
> > diff --git a/gas/config/tc-mips.c b/gas/config/tc-mips.c index
> > c9266db..3adebf4 100644
> > --- a/gas/config/tc-mips.c
> > +++ b/gas/config/tc-mips.c
> > @@ -15009,7 +15009,11 @@ md_apply_fix (fixS *fixP, valueT *valP, segT
> seg ATTRIBUTE_UNUSED)
> >        break;
> >
> >      case BFD_RELOC_MIPS_18_PCREL_S3:
> > -      if ((*valP & 0x7) != 0)
> > +      /* There is little point checking for a correctly aligned
> destination
> > +	 for R_MIPS_PC18_S3 as only the linker will know for sure if the
> > +	 destination address ends up 8-byte aligned or not.  Check for a
> > +	 minimum of 4-byte alignment instead.  */
> > +      if ((*valP & 0x3) != 0)
> >  	as_bad_where (fixP->fx_file, fixP->fx_line,
> >  		      _("PC-relative access to misaligned address (%lx)"),
> >  		      (long) *valP);
> 
> It seems a bit dangerous to drop a bit on the floor though (which I
> assume we'd end up doing for REL).  Also, in practice, the relocation is

I suppose it is actually a bit dangerous, I do hate this instruction.
I guess with 'rel' then a PC18_S3 reloc with only 4-byte multiple addend is
going to fall 8-bytes short of where the user intended:

0x4 LD $4, 4($pc)
0x8 0xvalue

But this is actually not supported as the displacement on that variant of
LDPC is required to be a multiple of 8. However using LD $4, 8($pc) does
not actually load from $PC+8 it loads from ($pc&~7) + 8. Slightly strange
but probably OK.

This is accepted:

0x4 LDPC $4, .+4
0x8 0xvalue

But leads to *valP being 0x4 which will be lost when the value is written into
the insn. LD will then report that it is misaligned which it wasn't from the
user's code but is now. What does work is using a symbol and an offset that
is a multiple of 8, this is the expected use case.

When I first implemented PCREL18_S3 I had md_pcrel_from set to base it at
$pc&~7 (which is accurate). As I recall this was a bigger mess still as trying
to account for the masked base in the assembler doesn't mean much if the
base ends up aligned in the linker. I am still relatively sure I should not
do this adjustment in md_pcrel_from.

> only safe if the target has 8-byte alignment at assembly time.  I.e. the
> section that the target is in should have at least 8 bytes alignment and
> the symbol itself should have an offset that's divisible by 8.
> 
> So I think we should either check S_GET_VALUE (fx_addsy) + fx_offset is
> aligned or (probably better, if it works) check that the low 3 bits of
> *valP are the same as the low three bits of:
> 
>     fixP->fx_where + fixP->fx_frag->fr_address

OK. I almost did something like this but essentially wasn't confident enough.
I'm not sure I follow on the low three bits matching part?

So the extra questions are is it OK that:

LD <Reg>,<off>($pc) requires 'off' to be a multiple of 8 and the calculation 
here is ($pc&~7 + off).

LDPC <reg>, <4-byte-aligned-label>+<4-byte-multiple-offset> is
meaningless and will break for REL.

Personally I think the former is not overly harmful but I suspect some would
prefer the latter to raise an error rather than silently lose bits.

Thanks,
Matthew