[PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Maciej W. Rozycki]

Hi,

 Regrettably my changes:

	opcodes/
	* mips-opc.c (mips_builtin_opcodes): Move M_LD_OB and M_SD_OB 
	macros before their corresponding MIPS III hardware instructions.

	gas/
	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Handle 64-bit ABIs.

(patch 02/15 of the recent long series) to make the LD/SD macro behave 
consistently on o32 regressed the handling of relocations on NewABI 
targets.  Which of course shows our lack of test coverage there, but I 
should have caught that regardless. :(

 Considering the following (not necessarily sensible) program:

$ cat ld-n64.s
	lui	$2, %got_hi(foo)
	ld	$31, %got_lo(foo)($2)
	lui	$3, %hi(%neg(%gp_rel(bar)))
	ld	$16, %lo(%neg(%gp_rel(bar)))($3)

We used to and should get the following output:

$ mips64-linux-as -64 -o ld-n64.o ld-n64.s
$ mips64-linux-objdump -dr ld-n64.o

ld-n64.o:     file format elf64-tradbigmips


Disassembly of section .text:

0000000000000000 <.text>:
   0:	3c020000 	lui	v0,0x0
			0: R_MIPS_GOT_HI16	foo
			0: R_MIPS_NONE	*ABS*
			0: R_MIPS_NONE	*ABS*
   4:	dc5f0000 	ld	ra,0(v0)
			4: R_MIPS_GOT_LO16	foo
			4: R_MIPS_NONE	*ABS*
			4: R_MIPS_NONE	*ABS*
   8:	3c030000 	lui	v1,0x0
			8: R_MIPS_GPREL16	bar
			8: R_MIPS_SUB	*ABS*
			8: R_MIPS_HI16	*ABS*
   c:	dc700000 	ld	s0,0(v1)
			c: R_MIPS_GPREL16	bar
			c: R_MIPS_SUB	*ABS*
			c: R_MIPS_LO16	*ABS*

Instead we get this:

$ mips64-linux-as -64 -o ld-n64-bad.o ld-n64.s
$ mips64-linux-objdump -dr ld-n64-bad.o

ld-n64-bad.o:     file format elf64-tradbigmips


Disassembly of section .text:

0000000000000000 <.text>:
   0:	3c020000 	lui	v0,0x0
			0: R_MIPS_GOT_HI16	foo
			0: R_MIPS_NONE	*ABS*
			0: R_MIPS_NONE	*ABS*
   4:	dc5f0000 	ld	ra,0(v0)
			4: R_MIPS_LO16	foo
			4: R_MIPS_NONE	*ABS*
			4: R_MIPS_NONE	*ABS*
   8:	3c030000 	lui	v1,0x0
			8: R_MIPS_GPREL16	bar
			8: R_MIPS_SUB	*ABS*
			8: R_MIPS_HI16	*ABS*
   c:	dc700000 	ld	s0,0(v1)
			c: R_MIPS_LO16	bar
			c: R_MIPS_NONE	*ABS*
			c: R_MIPS_NONE	*ABS*

The problem is with the change in place M_LD_OB and M_SD_OB macros take 
precedence over the corresponding hardware instructions and our macros 
generally have no support for reloc operators.

 All the infrastructure is in place though and here is my proposal to fix 
the problem.  A side effect is these macros now handle relocations other 
than LO16 even on o32.  While easily doable I think it makes no sense to 
insist on the previous behaviour of these macros on o32 -- I think where 
the address offset is hardware-expressable (i.e. matches the "o(b)" 
format) it makes perfect sense to respect percent-ops even in macros.  
What do you think?

 Tested with the usual set of targets: mips-linux, mips64-linux, 
mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their respective 
little-endian counterparts.  At least a smoke test for the regression 
caused here would be a good idea; I'll think of something.

2010-10-24  Maciej W. Rozycki  <macro@linux-mips.org>

	gas/
	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use any offset
	reloc supplied.

 OK to apply?

  Maciej

binutils-2.20.51-20100925-mips-gas-ld-reloc.patch
Index: binutils-2.20.51/gas/config/tc-mips.c
===================================================================
--- binutils-2.20.51.orig/gas/config/tc-mips.c
+++ binutils-2.20.51/gas/config/tc-mips.c
@@ -4822,7 +4822,8 @@ macro (struct mips_cl_insn *ip)
   int call = 0;
   int off;
   offsetT maxnum;
-  bfd_reloc_code_real_type r;
+  bfd_reloc_code_real_type r[3]
+    = {BFD_RELOC_UNUSED, BFD_RELOC_UNUSED, BFD_RELOC_UNUSED};
   int hold_mips_optimize;
 
   gas_assert (! mips_opts.mips16);
@@ -7026,7 +7027,7 @@ macro (struct mips_cl_insn *ip)
 	      break;
 	    }
 	  breg = mips_gp_register;
-	  r = BFD_RELOC_MIPS_LITERAL;
+	  r[0] = BFD_RELOC_MIPS_LITERAL;
 	  goto dob;
 	}
       else
@@ -7049,23 +7050,23 @@ macro (struct mips_cl_insn *ip)
 	      break;
 	    }
 	  breg = AT;
-	  r = BFD_RELOC_LO16;
+	  r[0] = BFD_RELOC_LO16;
 	  goto dob;
 	}
 
     case M_L_DOB:
       /* Even on a big endian machine $fn comes before $fn+1.  We have
 	 to adjust when loading from memory.  */
-      r = BFD_RELOC_LO16;
+      r[0] = BFD_RELOC_LO16;
     dob:
       gas_assert (mips_opts.isa == ISA_MIPS1);
       macro_build (&offset_expr, "lwc1", "T,o(b)",
-		   target_big_endian ? treg + 1 : treg, r, breg);
+		   target_big_endian ? treg + 1 : treg, r[0], breg);
       /* FIXME: A possible overflow which I don't know how to deal
 	 with.  */
       offset_expr.X_add_number += 4;
       macro_build (&offset_expr, "lwc1", "T,o(b)",
-		   target_big_endian ? treg : treg + 1, r, breg);
+		   target_big_endian ? treg : treg + 1, r[0], breg);
       break;
 
     case M_L_DAB:
@@ -7350,12 +7351,20 @@ macro (struct mips_cl_insn *ip)
     case M_SD_OB:
       s = HAVE_64BIT_GPRS ? "sd" : "sw";
     sd_ob:
-      macro_build (&offset_expr, s, "t,o(b)", treg, BFD_RELOC_LO16, breg);
+      r[0] = BFD_RELOC_LO16;
+      if (offset_expr.X_op == O_symbol)
+	{
+	  r[0] = offset_reloc[0];
+	  r[1] = offset_reloc[1];
+	  r[2] = offset_reloc[2];
+	}
+      macro_build (&offset_expr, s, "t,o(b)", treg,
+		   -1, r[0], r[1], r[2], breg);
       if (!HAVE_64BIT_GPRS)
 	{
 	  offset_expr.X_add_number += 4;
 	  macro_build (&offset_expr, s, "t,o(b)", treg + 1,
-		       BFD_RELOC_LO16, breg);
+		       -1, r[0], r[1], r[2], breg);
 	}
       break;
 

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Richard Sandiford]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
>  All the infrastructure is in place though and here is my proposal to fix 
> the problem.  A side effect is these macros now handle relocations other 
> than LO16 even on o32.  While easily doable I think it makes no sense to 
> insist on the previous behaviour of these macros on o32 -- I think where 
> the address offset is hardware-expressable (i.e. matches the "o(b)" 
> format) it makes perfect sense to respect percent-ops even in macros.  
> What do you think?

Yeah, the previous o32 behaviour was a silent wrong-code bug.

>  Tested with the usual set of targets: mips-linux, mips64-linux, 
> mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their respective 
> little-endian counterparts.  At least a smoke test for the regression 
> caused here would be a good idea; I'll think of something.

Yeah, I think the patch does need a testcase.  Like you say, it doesn't
have to be anything fancy.

> 2010-10-24  Maciej W. Rozycki  <macro@linux-mips.org>
>
> 	gas/
> 	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use any offset
> 	reloc supplied.

Looks good, but I think it would be easier to make the 'o' case in
mips_ip set offset_expr appropriately, so that we can use it
unconditionally.  I.e.:

	    case 'o':		/* 16 bit offset */
	      offset_reloc[0] = BFD_RELOC_LO16;
	      offset_reloc[1] = BFD_RELOC_UNUSED;
	      offset_reloc[2] = BFD_RELOC_UNUSED;

This won't affect non-macro uses, since 'o' only accepts non-reloc
constants that don't overflow a LO16.

(As far as setting [1] and [2] goes: I think it would be better if all
the cases in mips_ip set all three reloc elements, rather than just the
first as some do.  That's a separate change though.)

With the change to 'o', let's leave "r" as a scalar.

Also, while we're here, would you mind fixing the unaligned load/store
macros in the same way?

This may be a known bug, and is certainly nothing to do with your patches,
but I notice:

	ld	$4,0x7ffc($5)

fails to work correctly (or trigger a diagnostic) in o32 mode.
0x8000 works fine of course.

Ah, the fun of macros.

Richard

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Maciej W. Rozycki]

On Mon, 25 Oct 2010, Richard Sandiford wrote:

> >  Tested with the usual set of targets: mips-linux, mips64-linux, 
> > mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their respective 
> > little-endian counterparts.  At least a smoke test for the regression 
> > caused here would be a good idea; I'll think of something.
> 
> Yeah, I think the patch does need a testcase.  Like you say, it doesn't
> have to be anything fancy.

 Given the current state of affairs I'll post the testcase I have in mind 
separately later on.  Note that the LD failures spotted by Alan are 
indirect only -- we seem to be lacking a direct test, so one will be good 
to have IMO.

> > 2010-10-24  Maciej W. Rozycki  <macro@linux-mips.org>
> >
> > 	gas/
> > 	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use any offset
> > 	reloc supplied.
> 
> Looks good, but I think it would be easier to make the 'o' case in
> mips_ip set offset_expr appropriately, so that we can use it
> unconditionally.  I.e.:
> 
> 	    case 'o':		/* 16 bit offset */
> 	      offset_reloc[0] = BFD_RELOC_LO16;
> 	      offset_reloc[1] = BFD_RELOC_UNUSED;
> 	      offset_reloc[2] = BFD_RELOC_UNUSED;
> 
> This won't affect non-macro uses, since 'o' only accepts non-reloc
> constants that don't overflow a LO16.

 Yes, that has a potential for making changes to other macros simpler.

> (As far as setting [1] and [2] goes: I think it would be better if all
> the cases in mips_ip set all three reloc elements, rather than just the
> first as some do.  That's a separate change though.)

 Agreed.

> With the change to 'o', let's leave "r" as a scalar.

 (Un)done now.

> Also, while we're here, would you mind fixing the unaligned load/store
> macros in the same way?

 And l.d, etc. presumably too?  I'll post a separate change, but not 
tonight, sorry (all this stuff took longer than I had thought it would due 
to numerous testsuite adjustments).

> This may be a known bug, and is certainly nothing to do with your patches,
> but I notice:
> 
> 	ld	$4,0x7ffc($5)
> 
> fails to work correctly (or trigger a diagnostic) in o32 mode.
> 0x8000 works fine of course.

 You didn't like my patch addressing this issue back here:

http://sourceware.org/ml/binutils/2005-02/msg00610.html

(originally here: http://sourceware.org/ml/binutils/2004-06/msg00530.html)

but I've kept maintaining it locally over the years (and got it up to 
2.20; obviously with the recent changes it'll require an update, but I 
planned to do that anyway while upgrading the RPM packages I maintain).  
If you'd like me to get it refreshed and resubmitted, then I am all for 
it.

 Note that at the moment we are a little bit inconsistent here -- when 
executed for an unaligned address (such as 4 mod 8) 64-bit LD, etc. will 
trigger an address error exception that will either be emulated (such as 
under Linux) or presumably trigger diagnostics (such as with bare-iron 
programs).  On o32 our macro however will silently transfer corrupt data.

 Also note that I don't think an offset of 4 mod 8 is inherently illegal 
for 64-bit data quantities as MIPS IV ISA's alignment restriction on 
offsets was never enforced and has been lifted since, and when combined 
with a base address of 4 mod 8, the resulting sum will be correctly 
aligned.

> Ah, the fun of macros.

 Indeed.

 Here's a new version -- tested as previously (plus for binutils and LD).  
OK?

2010-10-31  Maciej W. Rozycki  <macro@linux-mips.org>

	gas/
	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use the offset
	reloc supplied.
	(mips_ip)['o']: Initialise offset_reloc.

  Maciej

binutils-2.20.51-20100925-mips-gas-ld-reloc.patch
Index: binutils-2.20.51/gas/config/tc-mips.c
===================================================================
--- binutils-2.20.51.orig/gas/config/tc-mips.c
+++ binutils-2.20.51/gas/config/tc-mips.c
@@ -7350,12 +7350,15 @@ macro (struct mips_cl_insn *ip)
     case M_SD_OB:
       s = HAVE_64BIT_GPRS ? "sd" : "sw";
     sd_ob:
-      macro_build (&offset_expr, s, "t,o(b)", treg, BFD_RELOC_LO16, breg);
+      macro_build (&offset_expr, s, "t,o(b)", treg,
+		   -1, offset_reloc[0], offset_reloc[1], offset_reloc[2],
+		   breg);
       if (!HAVE_64BIT_GPRS)
 	{
 	  offset_expr.X_add_number += 4;
 	  macro_build (&offset_expr, s, "t,o(b)", treg + 1,
-		       BFD_RELOC_LO16, breg);
+		       -1, offset_reloc[0], offset_reloc[1], offset_reloc[2],
+		       breg);
 	}
       break;
 
@@ -10001,6 +10004,10 @@ do_msbd:
 	      continue;
 
 	    case 'o':		/* 16 bit offset */
+	      offset_reloc[0] = BFD_RELOC_LO16;
+	      offset_reloc[1] = BFD_RELOC_UNUSED;
+	      offset_reloc[2] = BFD_RELOC_UNUSED;
+
 	      /* Check whether there is only a single bracketed expression
 		 left.  If so, it must be the base register and the
 		 constant must be zero.  */

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Richard Sandiford]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> On Mon, 25 Oct 2010, Richard Sandiford wrote:
>> >  Tested with the usual set of targets: mips-linux, mips64-linux, 
>> > mipstx39-elf, mipsisa64-elf and mips-ecoff targets and their respective 
>> > little-endian counterparts.  At least a smoke test for the regression 
>> > caused here would be a good idea; I'll think of something.
>> 
>> Yeah, I think the patch does need a testcase.  Like you say, it doesn't
>> have to be anything fancy.
>
>  Given the current state of affairs I'll post the testcase I have in mind 
> separately later on.  Note that the LD failures spotted by Alan are 
> indirect only -- we seem to be lacking a direct test, so one will be good 
> to have IMO.

It cuts both ways.  I see the binutils testsuite as effectively a unit,
rather than as a collection of separate component testsuites (gas/, ld/,
binutils/).  So while you could say that assembler-only relocation tests
are more "direct" than ld tests, you could also say that they're less
complete than the assembler+linker tests in ld/.

> 	gas/
> 	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use the offset
> 	reloc supplied.
> 	(mips_ip)['o']: Initialise offset_reloc.

OK.

Richard

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Richard Sandiford]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
> On Mon, 25 Oct 2010, Richard Sandiford wrote:
>> This may be a known bug, and is certainly nothing to do with your patches,
>> but I notice:
>> 
>> 	ld	$4,0x7ffc($5)
>> 
>> fails to work correctly (or trigger a diagnostic) in o32 mode.
>> 0x8000 works fine of course.
>
>  You didn't like my patch addressing this issue back here:
>
> http://sourceware.org/ml/binutils/2005-02/msg00610.html
>
> (originally here: http://sourceware.org/ml/binutils/2004-06/msg00530.html)
>
> but I've kept maintaining it locally over the years (and got it up to 
> 2.20; obviously with the recent changes it'll require an update, but I 
> planned to do that anyway while upgrading the RPM packages I maintain).  
> If you'd like me to get it refreshed and resubmitted, then I am all for 
> it.

No, I stand by what I said there.  IMO the reloc case isn't interesting
for the reasons discussed in that thread.  The o(b) case _is_
interesting because it is inconsistent with the corresponding
o(b) behaviour for unaligned loads and stores.

Richard

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Maciej W. Rozycki]

On Mon, 1 Nov 2010, Richard Sandiford wrote:

> >  Given the current state of affairs I'll post the testcase I have in mind 
> > separately later on.  Note that the LD failures spotted by Alan are 
> > indirect only -- we seem to be lacking a direct test, so one will be good 
> > to have IMO.
> 
> It cuts both ways.  I see the binutils testsuite as effectively a unit,
> rather than as a collection of separate component testsuites (gas/, ld/,
> binutils/).  So while you could say that assembler-only relocation tests
> are more "direct" than ld tests, you could also say that they're less
> complete than the assembler+linker tests in ld/.

 You're right, but that's not what I had in mind writing that -- these 
tests (be they from the binutils or GAS or LD subset -- as you say that 
really does not matter here) have not been specifically designed to test 
compound relocations, but some other features and they trigger and fail by 
chance only.  That is of course a good and desired side effect and as you 
may have noticed I even try to induce some side effects deliberately (or 
just notice their presence ;) ) with some test cases I make, but I think 
that's a feature that deserves an explicit check guaranteeing a better 
coverage.

 I have a rough idea how to do that, but I'll need some time to implement 
it properly and I simply ran out of last weekend.

> > 	gas/
> > 	* config/tc-mips.c (macro)[M_LD_OB, M_SD_OB]: Use the offset
> > 	reloc supplied.
> > 	(mips_ip)['o']: Initialise offset_reloc.
> 
> OK.

 Applied now, thanks.

  Maciej

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Maciej W. Rozycki]

On Mon, 1 Nov 2010, Richard Sandiford wrote:

> >> This may be a known bug, and is certainly nothing to do with your patches,
> >> but I notice:
> >> 
> >> 	ld	$4,0x7ffc($5)
> >> 
> >> fails to work correctly (or trigger a diagnostic) in o32 mode.
> >> 0x8000 works fine of course.
> >
> >  You didn't like my patch addressing this issue back here:
> >
> > http://sourceware.org/ml/binutils/2005-02/msg00610.html
> >
> > (originally here: http://sourceware.org/ml/binutils/2004-06/msg00530.html)
> >
> > but I've kept maintaining it locally over the years (and got it up to 
> > 2.20; obviously with the recent changes it'll require an update, but I 
> > planned to do that anyway while upgrading the RPM packages I maintain).  
> > If you'd like me to get it refreshed and resubmitted, then I am all for 
> > it.
> 
> No, I stand by what I said there.  IMO the reloc case isn't interesting
> for the reasons discussed in that thread.  The o(b) case _is_
> interesting because it is inconsistent with the corresponding
> o(b) behaviour for unaligned loads and stores.

 I am confused.  The very purpose of that patch 
(binutils-*-mips-dword-reloc.patch, for the avoidance of doubt) is to 
avoid an overflow from 0x7ffc wrapping to -0x8000.

 If you're concerned about my proposal pessimising code for a corner case 
where one of two LO16 relocs sharing a common HI16 reloc has a carry at 
the link stage into the HI16 reloc while the other one does not, then 
perhaps it would be enough if LD just failed the link complaining about 
this problem instead?  Then we would only have to take care of the 
immediate addend of 0x7ffc.  The drawback is if the LD error is hit, the 
user would have to sort out the problem himself and frankly "rearranging 
code so that the symbol <foo> has its final address different to <addr>" 
sounds rather like a dodgy solution to me.

 It looks I've had that binutils-*-mips-reloc-got-offset.patch change 
outstanding for a couple of years too. ;)

  Maciej

Re: [PATCH] MIPS/GAS: Fix NewABI reloc handling with the LD/SD macro

[Richard Sandiford]

"Maciej W. Rozycki" <macro@linux-mips.org> writes:
>> >> This may be a known bug, and is certainly nothing to do with your patches,
>> >> but I notice:
>> >> 
>> >> 	ld	$4,0x7ffc($5)
>> >> 
>> >> fails to work correctly (or trigger a diagnostic) in o32 mode.
>> >> 0x8000 works fine of course.
>> >
>> >  You didn't like my patch addressing this issue back here:
>> >
>> > http://sourceware.org/ml/binutils/2005-02/msg00610.html
>> >
>> > (originally here: http://sourceware.org/ml/binutils/2004-06/msg00530.html)
>> >
>> > but I've kept maintaining it locally over the years (and got it up to 
>> > 2.20; obviously with the recent changes it'll require an update, but I 
>> > planned to do that anyway while upgrading the RPM packages I maintain).  
>> > If you'd like me to get it refreshed and resubmitted, then I am all for 
>> > it.
>> 
>> No, I stand by what I said there.  IMO the reloc case isn't interesting
>> for the reasons discussed in that thread.  The o(b) case _is_
>> interesting because it is inconsistent with the corresponding
>> o(b) behaviour for unaligned loads and stores.
>
>  I am confused.  The very purpose of that patch 
> (binutils-*-mips-dword-reloc.patch, for the avoidance of doubt) is to 
> avoid an overflow from 0x7ffc wrapping to -0x8000.
>
>  If you're concerned about my proposal pessimising code for a corner case 
> where one of two LO16 relocs sharing a common HI16 reloc has a carry at 
> the link stage into the HI16 reloc while the other one does not, then 
> perhaps it would be enough if LD just failed the link complaining about 
> this problem instead?  Then we would only have to take care of the 
> immediate addend of 0x7ffc.  The drawback is if the LD error is hit, the 
> user would have to sort out the problem himself and frankly "rearranging 
> code so that the symbol <foo> has its final address different to <addr>" 
> sounds rather like a dodgy solution to me.

I'm talking specifically about the case where no relocations are involved,
i.e. where the offset is a constant integer.  And yeah, the inconsistency
I mentioned is precisely that uld & co.  will issue a diagnostic in this
case.  I think that's what we should do for ld & sd as well.

Richard