Re: [PATCH] microMIPS/GAS: Avoid relocation overflow with forced 16-bit branches

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Thu, 25 Oct 2012, Richard Sandiford wrote:

> > Overall it looks to me the whole idea of using fixp->fx_size to check for 
> > relocatable field overflows is missing the point, at least for the MIPS 
> > target and relocations in text, where the size of the fixup reflects the 
> > width of the instruction a relocation is applied to and has little to do 
> > with the range supported.
> >
> >  Anyway, to fix this immediate problem I have gladly accepted Chao-ying's 
> > proposal.
> >
> >  No regressions in testing across the usual MIPS targets.  OK to apply?
> >
> > 2012-10-24  Chao-ying Fu  <fu@mips.com>
> >
> > 	gas/
> > 	* config/tc-mips.c (append_insn): Set fx_no_overflow for 16-bit
> > 	microMIPS branch relocations.
> >
> > 	gas/testsuite/
> > 	* gas/mips/micromips-b16.d: New test.
> > 	* gas/mips/micromips-b16.s: New test source.
> > 	* gas/mips/mips.exp: Run the new test.
> 
> I take your point about the write.c check (been hit by that before too),
> but I suppose the obvious question is: do we still report branches that
> genuinely overflow?

 Of course we do, for symbols resolved at the assembly time GAS does that 
and for all the others the linker does:

$ cat b.s
	.text
foo:
	bc1f	0f
	.space	262144
0:
	.space	262144
	bc1f	0b
$ mips-linux-gnu-as -o b.o b.s
b.s: Assembler messages:
b.s:3: Error: Branch out of range
b.s:7: Error: Branch out of range
$ cat b1.s
	.text
	.globl	foo
	.type	foo, @function
	.space	262144
foo:
	bc1f	bar
	.space	262144
$ cat b2.s
	.text
	.globl	bar
	.type	bar, @function
	.space	262144
bar:
	bc1f	foo
	.space	262144
$ mips-linux-gnu-as -o b1.o b1.s
$ mips-linux-gnu-as -o b2.o b2.s
$ mips-linux-gnu-ld -e 0 -o b12 b1.o b2.o
b1.o: In function `foo':
(.text+0x40000): relocation truncated to fit: R_MIPS_PC16 against `bar'
b2.o: In function `bar':
(.text+0x40000): relocation truncated to fit: R_MIPS_PC16 against `foo'
$

Unsure of MIPS16; from experience its reloc overflow handling may be 
suboptimal.

>  Although you've used the same wording, this isn't
> quite the same as the preceding:
> 
>       /* These relocations can have an addend that won't fit in
> 	 4 octets for 64bit assembly.  */
>       if (HAVE_64BIT_GPRS
> 	  && ! howto->partial_inplace
> 
> because in that case the RELA entry contains the full, untruncated addend.

 I saw this check while looking into this issue and frankly it makes 
little sense to me.  First of all it looks to me it shouldn't be checking 
howto->partial_inplace -- while our 64-bit ABIs want RELA relocations we 
do support REL for them, at least to some extent, and the overflow does 
not depend on whether the addend is stored in the reloc entry or in the 
instruction.  Offhand at least HIGHEST, HIGHER, HI16 and LO16 relocations 
never overflow no matter the addend, the ABI and the reloc style.

 Furthermore for many relocations fixp->fx_size is 4 and the size of the 
relocatable field -- only 2.  Examples: HI16, LO16, GPREL16, GOT16, PC16, 
etc.  As noted above, of these HI16 and LO16 never overflow, GPREL16 and 
GOT16 overflow at 2 (rather than 4) and PC16 overflow at 18 *bits*.  Etc., 
etc...

> In contrast, 16-bit branches _can_ overflow, just not in the way that
> write.c expects.

 As can 32-bit branches, regardless of the ISA mode.  In the microMIPS 
mode we don't however check for branch range overflows in GAS as we always 
produce a relocation for the linker to resolve once any linker relaxation 
has been made.  The linker then complains about any relocation overflows 
as shown above.

 I realise this is suboptimal and Iain has been working on making things 
better.  I am not sure what his schedule exactly is though -- Iain?

> Would be nice to have a test for that too, if we don't already.

 We do have some tests, but only for BPOSGE32. ;)  And for the linker we 
check some other relocations, but we don't check the branch ones.  I'll 
see what I can do, but I hope that is not a prerequisite for this bug fix, 
I'd like to tick it off.

> I realise this fix is the same as that used in md_convert_frag for
> relaxing microMIPS branches, but the same question applies there
> in the "forced to fit" case.

 In md_convert_frag we only produce a 16-bit branch once we've determined 
its destination is in range, see relaxed_micromips_16bit_branch_length.

 Overall thanks for your review.  Also I forgot to cc you on the LUI issue 
-- would you please have a look too?

  Maciej