[committed] small one liner in gas

[committed] small one liner in gas

[Eric Christopher]

Fixes a small bug Zack ran into with:

lw $3,%half($gp)

where he was getting an internal error in gas for this.

The fx_size was 4 because that's the size of the data worked on, not 2
which is the size in the howto. Anyhow, instead of changing the assert
to 4 I just removed it since none of the other relocations have asserts
in them.

-eric

-- 
Eric Christopher <echristo@redhat.com>

2004-10-04  Eric Christopher  <echristo@redhat.com>

	* config/tc-mips.c (md_apply_fix3): Remove erroneous assert.\

Index: config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.270
diff -u -p -w -r1.270 tc-mips.c
--- config/tc-mips.c	18 Aug 2004 15:58:11 -0000	1.270
+++ config/tc-mips.c	4 Oct 2004 23:16:13 -0000
@@ -11004,7 +11004,6 @@ md_apply_fix3 (fixS *fixP, valueT *valP,
     case BFD_RELOC_16:
       /* If we are deleting this reloc entry, we must fill in the
          value now.  */
-      assert (fixP->fx_size == 2);
       if (fixP->fx_done)
 	md_number_to_chars (buf, *valP, 2);
       break;

Re: [committed] small one liner in gas

[Daniel Jacobowitz]

On Mon, Oct 04, 2004 at 04:25:06PM -0700, Eric Christopher wrote:
> Fixes a small bug Zack ran into with:
> 
> lw $3,%half($gp)
> 
> where he was getting an internal error in gas for this.
> 
> The fx_size was 4 because that's the size of the data worked on, not 2
> which is the size in the howto. Anyhow, instead of changing the assert
> to 4 I just removed it since none of the other relocations have asserts
> in them.

Waitasec...

> -      assert (fixP->fx_size == 2);
>        if (fixP->fx_done)
>  	md_number_to_chars (buf, *valP, 2);

Won't that get the wrong location for big-endian?  The code for LO16
has an endian correction.

[Zack, I think that doesn't matter for us, since the only place we'll
be generating %half fx_done won't get set.]

-- 
Daniel Jacobowitz

Re: [committed] small one liner in gas

[Eric Christopher]

> Won't that get the wrong location for big-endian?  The code for LO16
> has an endian correction.
> 

Doesn't look like the code for regular 16 does though. Of course, my ABI
manual is at home so if you can look... :)

-eric

-- 
Eric Christopher <echristo@redhat.com>

MIPS %half in gas

[Daniel Jacobowitz]

A message from so long ago that almost none of the addresses are still
valid... CC's trimmed :-)

On Mon, Oct 04, 2004 at 09:49:27PM -0400, Daniel Jacobowitz wrote:
> On Mon, Oct 04, 2004 at 04:25:06PM -0700, Eric Christopher wrote:
> > Fixes a small bug Zack ran into with:
> > 
> > lw $3,%half($gp)
> > 
> > where he was getting an internal error in gas for this.
> > 
> > The fx_size was 4 because that's the size of the data worked on, not 2
> > which is the size in the howto. Anyhow, instead of changing the assert
> > to 4 I just removed it since none of the other relocations have asserts
> > in them.
> 
> Waitasec...
> 
> > -      assert (fixP->fx_size == 2);
> >        if (fixP->fx_done)
> >  	md_number_to_chars (buf, *valP, 2);
> 
> Won't that get the wrong location for big-endian?  The code for LO16
> has an endian correction.

I was right.  Removing the assert was wrong, and the code has since
changed to get even wronger.  Assemble this for MIPS big-endian or
little-endian:

	lw $3,%half(x)($gp)
	.set x, 0x1234

Either way you get:

00000000 <.text>:
   0:   00001234        0x1234

If x is defined in another file, so that the linker has to do the
fixup, the right thing happens.

It got wronger here:

2006-08-01  Thiemo Seufer  <ths@mips.com>

        * config/tc-mips.c (macro_build_lui): Fix comment formatting.
        (md_apply_fix): Likewise. Unify handling of BFD_RELOC_RVA,
        BFD_RELOC_32 and BFD_RELOC_16.
        (s_align, s_cpload, s_cplocal, s_cprestore, s_mips_stab,
        md_convert_frag, md_obj_end): Fix comment formatting.

It looked as if fx_size was always 2 for BFD_RELOC_16, but that's not
true; it's always 4.

I came up with the attached patch and testcase.  Does this look OK?
I've only tested it as below plus some manual verification of the
error messages; I don't have any use for %half at the moment, just
didn't want to leave it broken.

-- 
Daniel Jacobowitz
CodeSourcery

2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* config/tc-mips.c (append_insn): Handle BFD_RELOC_16.
	(md_apply_fix): Likewise.

2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* gas/mips/half.d, gas/mips/half.s: New.
	* gas/mips/mips.exp: Run %half test.

Index: config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.376
diff -u -p -r1.376 tc-mips.c
--- config/tc-mips.c	8 Oct 2007 16:09:34 -0000	1.376
+++ config/tc-mips.c	12 Oct 2007 15:41:19 -0000
@@ -2742,6 +2742,15 @@ append_insn (struct mips_cl_insn *ip, ex
 	      ip->insn_opcode |= (address_expr->X_add_number >> 16) & 0xffff;
 	      break;
 
+	    case BFD_RELOC_16:
+	      if (address_expr->X_add_number & ~0xffffull)
+		{
+		  char value [32];
+
+		  sprintf_vma (value, offset_expr.X_add_number);
+		  as_bad (_("Number (0x%s) larger than 16 bits"), value);
+		}
+	      /* FALLTHROUGH */
 	    case BFD_RELOC_UNUSED:
 	    case BFD_RELOC_LO16:
 	    case BFD_RELOC_MIPS_GOT_DISP:
@@ -11925,7 +11934,6 @@ md_apply_fix (fixS *fixP, valueT *valP, 
 
     case BFD_RELOC_RVA:
     case BFD_RELOC_32:
-    case BFD_RELOC_16:
       /* If we are deleting this reloc entry, we must fill in the
 	 value now.  This can happen if we have a .word which is not
 	 resolved when it appears but is later defined.  */
@@ -11933,6 +11941,27 @@ md_apply_fix (fixS *fixP, valueT *valP, 
 	md_number_to_chars ((char *) buf, *valP, fixP->fx_size);
       break;
 
+    case BFD_RELOC_16:
+      /* If we are deleting this reloc entry, we must fill in the
+	 value now.  This can happen if we have a .word which is not
+	 resolved when it appears but is later defined.  */
+      if (fixP->fx_done)
+	{
+	  if (*valP & ~0xffffull)
+	    {
+	      char value [32];
+
+	      sprintf_vma (value, *valP);
+	      as_bad_where (fixP->fx_file, fixP->fx_line,
+			    _("Number (0x%s) larger than 16 bits"), value);
+	    }
+
+	  if (target_big_endian)
+	    buf += 2;
+	  md_number_to_chars ((char *) buf, *valP, 2);
+	}
+      break;
+
     case BFD_RELOC_LO16:
     case BFD_RELOC_MIPS16_LO16:
       /* FIXME: Now that embedded-PIC is gone, some of this code/comment
Index: testsuite/gas/mips/half.d
===================================================================
RCS file: testsuite/gas/mips/half.d
diff -N testsuite/gas/mips/half.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gas/mips/half.d	12 Oct 2007 15:41:20 -0000
@@ -0,0 +1,12 @@
+#objdump: -dr --prefix-addresses --show-raw-insn
+#name: MIPS %half relocations
+#as: -32 -EB
+
+.*: +file format .*mips.*
+
+Disassembly of section .text:
+0+0000 <[^>]*> 8f831234 	lw	v1,4660\(gp\)
+0+0004 <[^>]*> 8f830000 	lw	v1,0\(gp\)
+			4: R_MIPS_16	y
+0+0008 <[^>]*> 8f831234 	lw	v1,4660\(gp\)
+#pass
Index: testsuite/gas/mips/half.s
===================================================================
RCS file: testsuite/gas/mips/half.s
diff -N testsuite/gas/mips/half.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gas/mips/half.s	12 Oct 2007 15:41:20 -0000
@@ -0,0 +1,6 @@
+	.text
+	.set x, 0x1234
+	lw $3,%half(x)($gp)
+	lw $3,%half(y)($gp)
+	lw $3,%half(z)($gp)
+	.set z, 0x1234
Index: testsuite/gas/mips/mips.exp
===================================================================
RCS file: /cvs/src/src/gas/testsuite/gas/mips/mips.exp,v
retrieving revision 1.132
diff -u -p -r1.132 mips.exp
--- testsuite/gas/mips/mips.exp	8 Oct 2007 16:41:34 -0000	1.132
+++ testsuite/gas/mips/mips.exp	12 Oct 2007 15:41:20 -0000
@@ -677,6 +677,8 @@ if { [istarget mips*-*-vxworks*] } {
 
 	run_list_test "tls-ill" "-32"
 	run_dump_test "tls-o32"
+
+	run_dump_test "half"
     }
 
     if $has_newabi {

Re: MIPS %half in gas

[Maciej W. Rozycki]

On Fri, 12 Oct 2007, Daniel Jacobowitz wrote:

> A message from so long ago that almost none of the addresses are still
> valid... CC's trimmed :-)

 Hmm, I find it nice to discover yet another MIPS relocation type/operator 
I was not aware of. ;-)  I can hardly see a use for this operator and I 
have no idea as to where it is coming from, but your change looks 
reasonable to me.

 Though the test case may be confusing -- given the relocation operates on 
an unsigned value, the use of "lw" in the test case is misleading.  I 
think "ori", "andi" or "xori" would be better.  Although of these only 
"ori" looks remotely useful and I am not sure if using this relocation on 
code rather than data (".hword %half(foo)") makes sense at all.

  Maciej

Re: MIPS %half in gas

[Daniel Jacobowitz]

On Fri, Oct 12, 2007 at 05:06:38PM +0100, Maciej W. Rozycki wrote:
>  Hmm, I find it nice to discover yet another MIPS relocation type/operator 
> I was not aware of. ;-)  I can hardly see a use for this operator and I 
> have no idea as to where it is coming from, but your change looks 
> reasonable to me.

I've got no idea where it comes from either.

>  Though the test case may be confusing -- given the relocation operates on 
> an unsigned value, the use of "lw" in the test case is misleading.  I 
> think "ori", "andi" or "xori" would be better.  Although of these only 
> "ori" looks remotely useful and I am not sure if using this relocation on 
> code rather than data (".hword %half(foo)") makes sense at all.

This is where my lack of knowing what things are for comes into play.
The example in Eric's original mail from long ago uses a gp-relative
address.  GCC uses it for exactly that purpose, in a VxWorks PIC
model:

(define_split
  [(unspec_volatile [(match_operand:P 0 "symbol_ref_operand")
                     (match_operand:P 1 "symbol_ref_operand")]
		     UNSPEC_LOADGP)]
  "mips_current_loadgp_style () == LOADGP_RTP"
  [(set (match_dup 2) (high:P (match_dup 3)))
   (set (match_dup 2) (unspec:P [(match_dup 2)
                                 (match_dup 3)] UNSPEC_LOAD_GOT))
   (set (match_dup 2) (unspec:P [(match_dup 2)
                                 (match_dup 4)] UNSPEC_LOAD_GOT))]
{
  operands[2] = pic_offset_table_rtx;
  operands[3] = mips_unspec_address (operands[0], SYMBOL_ABSOLUTE);
  operands[4] = mips_unspec_address (operands[1], SYMBOL_HALF);
})

That expands to something like %half(__GOTT_INDEX__)($gp) which is a
magic symbol defined by the runtime to a small value, so the
signedness does not much matter.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MIPS %half in gas

[Richard Sandiford]

Daniel Jacobowitz <drow@false.org> writes:
> +    case BFD_RELOC_16:
> +      /* If we are deleting this reloc entry, we must fill in the
> +	 value now.  This can happen if we have a .word which is not
> +	 resolved when it appears but is later defined.  */

Minor nit, but this comment is for BFD_RELOC_16 in isolation, which can
never come from a .word directive.  I think you should just generalise
".word" to "instruction", or something like that.

> +      if (fixP->fx_done)
> +	{
> +	  if (*valP & ~0xffffull)
> +	    {
> +	      char value [32];
> +
> +	      sprintf_vma (value, *valP);
> +	      as_bad_where (fixP->fx_file, fixP->fx_line,
> +			    _("Number (0x%s) larger than 16 bits"), value);
> +	    }
> +
> +	  if (target_big_endian)
> +	    buf += 2;
> +	  md_number_to_chars ((char *) buf, *valP, 2);
> +	}
> +      break;
> +

Regarding the discussion downthread, the ABI specifically says that an
R_MIPS_16 addend is signed rather than an unsigned, so (a) it really should
be OK to use %half for ADDIU and LW, and (b), the *valP check above probably
needs to check a signed range rather than an unsigned one, or at least
allow both.

Richard

Re: MIPS %half in gas

[Maciej W. Rozycki]

On Fri, 12 Oct 2007, Daniel Jacobowitz wrote:

> >  Though the test case may be confusing -- given the relocation operates on 
> > an unsigned value, the use of "lw" in the test case is misleading.  I 
> > think "ori", "andi" or "xori" would be better.  Although of these only 
> > "ori" looks remotely useful and I am not sure if using this relocation on 
> > code rather than data (".hword %half(foo)") makes sense at all.
> 
> This is where my lack of knowing what things are for comes into play.

 Me too, just trying to apply some common sense which may not always be 
justified. ;-)

> That expands to something like %half(__GOTT_INDEX__)($gp) which is a
> magic symbol defined by the runtime to a small value, so the
> signedness does not much matter.

 Well, such an abuse may work and may even be necessary for someone, but 
it does not mean we should actively promote it, does it? ;-)

  Maciej

Re: MIPS %half in gas

[Thiemo Seufer]

Daniel Jacobowitz wrote:
[snip]
> 2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* config/tc-mips.c (append_insn): Handle BFD_RELOC_16.
> 	(md_apply_fix): Likewise.
> 
> 2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* gas/mips/half.d, gas/mips/half.s: New.
> 	* gas/mips/mips.exp: Run %half test.
> 
> Index: config/tc-mips.c
> ===================================================================
> RCS file: /cvs/src/src/gas/config/tc-mips.c,v
> retrieving revision 1.376
> diff -u -p -r1.376 tc-mips.c
> --- config/tc-mips.c	8 Oct 2007 16:09:34 -0000	1.376
> +++ config/tc-mips.c	12 Oct 2007 15:41:19 -0000
> @@ -2742,6 +2742,15 @@ append_insn (struct mips_cl_insn *ip, ex
>  	      ip->insn_opcode |= (address_expr->X_add_number >> 16) & 0xffff;
>  	      break;
>  
> +	    case BFD_RELOC_16:
> +	      if (address_expr->X_add_number & ~0xffffull)
> +		{
> +		  char value [32];
> +
> +		  sprintf_vma (value, offset_expr.X_add_number);
> +		  as_bad (_("Number (0x%s) larger than 16 bits"), value);
> +		}
> +	      /* FALLTHROUGH */
>  	    case BFD_RELOC_UNUSED:
>  	    case BFD_RELOC_LO16:
>  	    case BFD_RELOC_MIPS_GOT_DISP:
> @@ -11925,7 +11934,6 @@ md_apply_fix (fixS *fixP, valueT *valP, 
>  
>      case BFD_RELOC_RVA:
>      case BFD_RELOC_32:
> -    case BFD_RELOC_16:
>        /* If we are deleting this reloc entry, we must fill in the
>  	 value now.  This can happen if we have a .word which is not
>  	 resolved when it appears but is later defined.  */
> @@ -11933,6 +11941,27 @@ md_apply_fix (fixS *fixP, valueT *valP, 
>  	md_number_to_chars ((char *) buf, *valP, fixP->fx_size);
>        break;
>  
> +    case BFD_RELOC_16:
> +      /* If we are deleting this reloc entry, we must fill in the
> +	 value now.  This can happen if we have a .word which is not

That's .hword or .short then, I think. Otherwise ok, thanks for
fixing this. :-)


Thiemo

Re: MIPS %half in gas

[Thiemo Seufer]

Richard Sandiford wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> > +    case BFD_RELOC_16:
> > +      /* If we are deleting this reloc entry, we must fill in the
> > +	 value now.  This can happen if we have a .word which is not
> > +	 resolved when it appears but is later defined.  */
> 
> Minor nit, but this comment is for BFD_RELOC_16 in isolation, which can
> never come from a .word directive.  I think you should just generalise
> ".word" to "instruction", or something like that.
> 
> > +      if (fixP->fx_done)
> > +	{
> > +	  if (*valP & ~0xffffull)
> > +	    {
> > +	      char value [32];
> > +
> > +	      sprintf_vma (value, *valP);
> > +	      as_bad_where (fixP->fx_file, fixP->fx_line,
> > +			    _("Number (0x%s) larger than 16 bits"), value);
> > +	    }
> > +
> > +	  if (target_big_endian)
> > +	    buf += 2;
> > +	  md_number_to_chars ((char *) buf, *valP, 2);
> > +	}
> > +      break;
> > +
> 
> Regarding the discussion downthread, the ABI specifically says that an
> R_MIPS_16 addend is signed rather than an unsigned, so (a) it really should
> be OK to use %half for ADDIU and LW, and (b), the *valP check above probably
> needs to check a signed range rather than an unsigned one, or at least
> allow both.

Indeed, I missed the signedness here.


Thiemo

Re: MIPS %half in gas

[Maciej W. Rozycki]

On Fri, 12 Oct 2007, Richard Sandiford wrote:

> > +      if (fixP->fx_done)
> > +	{
> > +	  if (*valP & ~0xffffull)
> > +	    {
> > +	      char value [32];
> > +
> > +	      sprintf_vma (value, *valP);
> > +	      as_bad_where (fixP->fx_file, fixP->fx_line,
> > +			    _("Number (0x%s) larger than 16 bits"), value);
> > +	    }
> > +
> > +	  if (target_big_endian)
> > +	    buf += 2;
> > +	  md_number_to_chars ((char *) buf, *valP, 2);
> > +	}
> > +      break;
> > +
> 
> Regarding the discussion downthread, the ABI specifically says that an
> R_MIPS_16 addend is signed rather than an unsigned, so (a) it really should
> be OK to use %half for ADDIU and LW, and (b), the *valP check above probably
> needs to check a signed range rather than an unsigned one, or at least
> allow both.

 Fair enough.  I only complained about the way of use in the test case, 
because the code above clearly treats the addend as unsigned.  I have had 
a look at the ABI document now and it documents a half16 relocatable field 
together with its associated R_MIPS_16 relocation like you say.  Although 
no indication there as to what it would be useful for.

  Maciej