sign extension break sleb128 tests on m68k

sign extension break sleb128 tests on m68k

[Alan Modra]

When building with a 64-bit bfd, sleb128 and quad tests fail on m68k
targets, because something like

 .sleb128 0x80000000

has the constant sign extendend due to a peculiarity of m68k gas (grep
for TARGET_WORD_SIZE) as if you'd written

 .sleb128 0xffffffff80000000

Richard's recent change to .sleb128 made it treat both of these
constants as unsigned, so they are different values.  Does anyone know
why this patch was applied?

Thu Apr 13 14:34:36 1995  Torbjorn Granlund  <tege@adder.cygnus.com>

	* config/tc-m68k.c (m68k_init_after_args): Test for m68360.
	(md_parse_option): Likewise.
	(md_show_usage): Mention m68360.
	* config/tc-m68k.h (TARGET_WORD_SIZE): Define.
	(TARGET_ARCH): Define.

	* expr.c (integer_constant): If TARGET_WORD_SIZE is defined,
	sign-extend appropriately.

Presumably it too was concerned with building 32-bit m68k binutils with
a 64-bit bfd, but unfortunately no testcase was added.  So I don't know
whether the hack is still needed.  Failing any good reason being found,
I propose the following:

	* expr.c (integer_constant): Remove TARGET_WORD_SIZE hack.
	* config/tc-m68k.h (TARGET_WORD_SIZE): Delete.

Index: gas/expr.c
===================================================================
RCS file: /cvs/src/src/gas/expr.c,v
retrieving revision 1.53
diff -u -p -r1.53 expr.c
--- gas/expr.c	1 Mar 2005 02:00:14 -0000	1.53
+++ gas/expr.c	1 Mar 2005 03:03:00 -0000
@@ -609,10 +609,6 @@ integer_constant (int radix, expressionS
       else
 	{
 	  expressionP->X_op = O_constant;
-#ifdef TARGET_WORD_SIZE
-	  /* Sign extend NUMBER.  */
-	  number |= (-(number >> (TARGET_WORD_SIZE - 1))) << (TARGET_WORD_SIZE - 1);
-#endif
 	  expressionP->X_add_number = number;
 	  input_line_pointer--;	/* Restore following character.  */
 	}			/* Really just a number.  */
Index: gas/config/tc-m68k.h
===================================================================
RCS file: /cvs/src/src/gas/config/tc-m68k.h,v
retrieving revision 1.15
diff -u -p -r1.15 tc-m68k.h
--- gas/config/tc-m68k.h	26 Jan 2004 18:09:30 -0000	1.15
+++ gas/config/tc-m68k.h	1 Mar 2005 03:03:11 -0000
@@ -206,7 +206,6 @@ extern int m68k_parse_long_option PARAMS
 
 #define md_operand(x)
 
-#define TARGET_WORD_SIZE 32
 #define TARGET_ARCH bfd_arch_m68k
 
 extern struct relax_type md_relax_table[];


-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: sign extension break sleb128 tests on m68k

[Nick Clifton]

Hi Alan,

> Does anyone know
> why this patch was applied?
> 
> Thu Apr 13 14:34:36 1995  Torbjorn Granlund  <tege@adder.cygnus.com>
> 
> 	* config/tc-m68k.c (m68k_init_after_args): Test for m68360.
> 	(md_parse_option): Likewise.
> 	(md_show_usage): Mention m68360.
> 	* config/tc-m68k.h (TARGET_WORD_SIZE): Define.
> 	(TARGET_ARCH): Define.
> 
> 	* expr.c (integer_constant): If TARGET_WORD_SIZE is defined,
> 	sign-extend appropriately.
> 
> Presumably it too was concerned with building 32-bit m68k binutils with
> a 64-bit bfd, but unfortunately no testcase was added.  So I don't know
> whether the hack is still needed.  Failing any good reason being found,
> I propose the following:
> 
> 	* expr.c (integer_constant): Remove TARGET_WORD_SIZE hack.
> 	* config/tc-m68k.h (TARGET_WORD_SIZE): Delete.

Agreed.  I suspect that Torbjorn's patch was an attempt at handling 
sign-extension in a target specific way when in fact it should have been 
generic.  With the current code handling this sort of thing quite well I 
doubt very much if his patch is still needed.

Cheers
   Nick

Re: sign extension break sleb128 tests on m68k

[James E Wilson]

[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]

On Mon, 2005-02-28 at 20:25, Alan Modra wrote:
> When building with a 64-bit bfd, sleb128 and quad tests fail on m68k
> targets, because something like
>  .sleb128 0x80000000
> has the constant sign extendend due to a peculiarity of m68k gas (grep
> for TARGET_WORD_SIZE) as if you'd written
>  .sleb128 0xffffffff80000000

That reminds me of something I recently noticed in the h8300 port.  I
was investigating a compiler problem, and tried building the h8300 port
on a lark to see if it could reproduce it.  Unfortunately, I discovered
that the port does not work on a 64-bit host.

One of the problems is that the h8300 gas port uses internal masks like
0xffffff00 which is intended to be -256.  That works fine on a 32-bit
host, but not on a 64-bit host, where this is a large positive number
instead of a small negative one.

The other related problem is that the testsuite contains similar
numbers.  See for instance the file h8sx_mov_insn.s in the
gas/testsuite/gas/h8300 directory.  It contains
        mov.b   #foo,@0xffff8000
which is supposed to be assembled into a 16-bit immediate field, but on
a 64-bit host that is a large positive number which requires a 32-bit
immediate field.  This causes the test to fail.

Perhaps something similar happened with the m68k port.  The problem
isn't obvious, since the code will still assemble, it just gets
assembled into larger less efficient code.  You can't tell there is a
problem unless you look at the binary instruction encodings to see that
the wrong immediate form was used.

In fact, this does seem to be the case.  It was easy to generate an
example.  Follows is two typescripts, one generated from before updating
binutils, one generated from after updating binutils.  Note that the
first one assembles into a 32-bit instruction, but the second one
assembles into a 48-bit instruction.  This is on an x86-64 linux
machine.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


[-- Attachment #2: orig-typescript --]
[-- Type: text/plain, Size: 341 bytes --]

Script started on Mon 07 Mar 2005 10:49:54 AM PST
aretha$ more tmp.s
	move  %d0, 0xffffff00
aretha$ ./as-new tmp.s
aretha$ ../binutils/objdump -d a.out

a.out:     file format elf32-m68k

Disassembly of section .text:

00000000 <.text>:
   0:	31c0 ff00      	movew %d0,0xffffff00
aretha$ exit

Script done on Mon 07 Mar 2005 10:50:06 AM PST

[-- Attachment #3: typescript --]
[-- Type: text/plain, Size: 346 bytes --]

Script started on Mon 07 Mar 2005 10:53:40 AM PST
aretha$ more tmp.s
	move  %d0, 0xffffff00
aretha$ ./as-new tmp.s
aretha$ ../binutils/objdump -d a.out

a.out:     file format elf32-m68k

Disassembly of section .text:

00000000 <.text>:
   0:	33c0 ffff ff00 	movew %d0,0xffffff00
aretha$ exit
exit

Script done on Mon 07 Mar 2005 10:53:54 AM PST

Re: sign extension break sleb128 tests on m68k

[Alan Modra]

On Mon, Mar 07, 2005 at 11:01:07AM -0800, James E Wilson wrote:
> On Mon, 2005-02-28 at 20:25, Alan Modra wrote:
> > When building with a 64-bit bfd, sleb128 and quad tests fail on m68k
> > targets, because something like
> >  .sleb128 0x80000000
> > has the constant sign extendend due to a peculiarity of m68k gas (grep
> > for TARGET_WORD_SIZE) as if you'd written
> >  .sleb128 0xffffffff80000000

[snip]
> One of the problems is that the h8300 gas port uses internal masks like
> 0xffffff00 which is intended to be -256.  That works fine on a 32-bit
> host, but not on a 64-bit host, where this is a large positive number
> instead of a small negative one.
> 
> The other related problem is that the testsuite contains similar
> numbers.  See for instance the file h8sx_mov_insn.s in the
> gas/testsuite/gas/h8300 directory.  It contains
>         mov.b   #foo,@0xffff8000
> which is supposed to be assembled into a 16-bit immediate field, but on
> a 64-bit host that is a large positive number which requires a 32-bit
> immediate field.  This causes the test to fail.

Which indicates another problem in the h8300 gas code.  I think this
constant ought to be assembled the same regardless of the host.  Take a
look at what tc-i386.c does for similar situations:

	    /* If this operand is at most 16 bits, convert it
	       to a signed 16 bit number before trying to see
	       whether it will fit in an even smaller size.
	       This allows a 16-bit operand such as $0xffe0 to
	       be recognised as within Imm8S range.  */
	    if ((i.types[op] & Imm16)
		&& (i.op[op].imms->X_add_number & ~(offsetT) 0xffff) == 0)
	      {
		i.op[op].imms->X_add_number =
		  (((i.op[op].imms->X_add_number & 0xffff) ^ 0x8000) - 0x8000);
	      }
	    if ((i.types[op] & Imm32)
		&& ((i.op[op].imms->X_add_number & ~(((offsetT) 2 << 31) - 1))
		    == 0))
	      {
		i.op[op].imms->X_add_number = ((i.op[op].imms->X_add_number
						^ ((offsetT) 1 << 31))
					       - ((offsetT) 1 << 31));
	      }
	    i.types[op] |= smallest_imm_type (i.op[op].imms->X_add_number);


> Perhaps something similar happened with the m68k port.  The problem
[snip example]

Thanks for bringing this to light.  I've opened a bugzilla report.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: sign extension break sleb128 tests on m68k

[Alan Modra]

This fixes the regression caused by my removal of the TARGET_WORD_SIZE
hack.  Applying mainline and branch.

	PR gas/780
	* config/tc-m68k.c (TRUNC, SEXT): Define.
	(issbyte, isubyte, issword, isuword, isbyte, isword): Use the above.
	(m68k_ip): Truncate or sign extend expressions as appropriate.
	(get_num): Likewise.
	(md_apply_fix3): Use SEXT.

Index: gas/config/tc-m68k.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-m68k.c,v
retrieving revision 1.63
diff -u -p -r1.63 tc-m68k.c
--- gas/config/tc-m68k.c	18 Mar 2005 17:20:33 -0000	1.63
+++ gas/config/tc-m68k.c	21 Mar 2005 00:33:59 -0000
@@ -651,13 +651,18 @@ const pseudo_typeS mote_pseudo_table[] =
   {0, 0, 0}
 };
 
-#define issbyte(x)	((x) >= -128 && (x) <= 127)
-#define isubyte(x)	((x) >= 0 && (x) <= 255)
-#define issword(x)	((x) >= -32768 && (x) <= 32767)
-#define isuword(x)	((x) >= 0 && (x) <= 65535)
+/* Truncate and sign-extend at 32 bits, so that building on a 64-bit host
+   gives identical results to a 32-bit host.  */
+#define TRUNC(X)	((valueT) (X) & 0xffffffff)
+#define SEXT(X)		((TRUNC (X) ^ 0x80000000) - 0x80000000)
+
+#define issbyte(x)	((valueT) SEXT (x) + 0x80 < 0x100)
+#define isubyte(x)	((valueT) TRUNC (x) < 0x100)
+#define issword(x)	((valueT) SEXT (x) + 0x8000 < 0x10000)
+#define isuword(x)	((valueT) TRUNC (x) < 0x10000)
 
-#define isbyte(x)	((x) >= -255 && (x) <= 255)
-#define isword(x)	((x) >= -65536 && (x) <= 65535)
+#define isbyte(x)	((valueT) SEXT (x) + 0xff < 0x1ff)
+#define isword(x)	((valueT) SEXT (x) + 0xffff < 0x1ffff)
 #define islong(x)	(1)
 
 static char notend_table[256];
@@ -1762,8 +1767,7 @@ m68k_ip (char *instring)
 		  if (opP->mode != IMMED)
 		    losing++;
 		  else if (opP->disp.exp.X_op != O_constant
-			   || opP->disp.exp.X_add_number < 1
-			   || opP->disp.exp.X_add_number > 8)
+			   || TRUNC (opP->disp.exp.X_add_number) - 1 > 7)
 		    losing++;
 		  else if (! m68k_quick
 			   && (strncmp (instring, "add", 3) == 0
@@ -1809,8 +1813,7 @@ m68k_ip (char *instring)
 		  if (opP->mode != IMMED)
 		    losing++;
 		  else if (opP->disp.exp.X_op != O_constant
-			   || opP->disp.exp.X_add_number < 0
-			   || opP->disp.exp.X_add_number > 7)
+			   || TRUNC (opP->disp.exp.X_add_number) > 7)
 		    losing++;
 		  break;
 
@@ -1823,9 +1826,8 @@ m68k_ip (char *instring)
 		  if (opP->mode != IMMED)
 		    losing++;
 		  else if (opP->disp.exp.X_op != O_constant
-			   || opP->disp.exp.X_add_number < -1
-                           || opP->disp.exp.X_add_number > 7
-                           || opP->disp.exp.X_add_number == 0)
+			   || (TRUNC (opP->disp.exp.X_add_number) != 0xffffffff
+			       && TRUNC (opP->disp.exp.X_add_number) - 1 > 6))
 		    losing++;
 		  break;
 
@@ -2306,7 +2308,7 @@ m68k_ip (char *instring)
 			  else
 			    {
 			      add_frag (adds (&opP->disp),
-					offs (&opP->disp),
+					SEXT (offs (&opP->disp)),
 					TAB (PCREL1632, SZ_UNDEF));
 			      break;
 			    }
@@ -2478,7 +2480,8 @@ m68k_ip (char *instring)
 			  frag_grow (14);
 			  nextword += baseo & 0xff;
 			  addword (nextword);
-			  add_frag (adds (&opP->disp), offs (&opP->disp),
+			  add_frag (adds (&opP->disp),
+				    SEXT (offs (&opP->disp)),
 				    TAB (PCINDEX, SZ_UNDEF));
 
 			  break;
@@ -2621,7 +2624,7 @@ m68k_ip (char *instring)
 		    {
 		      tmpreg = 0x3A;	/* 7.2 */
 		      add_frag (adds (&opP->disp),
-				offs (&opP->disp),
+				SEXT (offs (&opP->disp)),
 				TAB (ABSTOPCREL, SZ_UNDEF));
 		      break;
 		    }
@@ -2804,20 +2807,24 @@ m68k_ip (char *instring)
 		 out which mode.  We try in this order of preference:
 		 long branch, absolute jump, byte/word branches only.  */
 	      if (HAVE_LONG_BRANCH (current_architecture))
-		add_frag (adds (&opP->disp), offs (&opP->disp),
+		add_frag (adds (&opP->disp),
+			  SEXT (offs (&opP->disp)),
 			  TAB (BRANCHBWL, SZ_UNDEF));
 	      else if (! flag_keep_pcrel)
 		{
 		  if ((the_ins.opcode[0] == 0x6000)
 		      || (the_ins.opcode[0] == 0x6100))
-		    add_frag (adds (&opP->disp), offs (&opP->disp),
+		    add_frag (adds (&opP->disp),
+			      SEXT (offs (&opP->disp)),
 			      TAB (BRABSJUNC, SZ_UNDEF));
 		  else
-		    add_frag (adds (&opP->disp), offs (&opP->disp),
+		    add_frag (adds (&opP->disp),
+			      SEXT (offs (&opP->disp)),
 			      TAB (BRABSJCOND, SZ_UNDEF));
 		}
 	      else
-		add_frag (adds (&opP->disp), offs (&opP->disp),
+		add_frag (adds (&opP->disp),
+			  SEXT (offs (&opP->disp)),
 			  TAB (BRANCHBW, SZ_UNDEF));
 	      break;
 	    case 'w':
@@ -2831,10 +2838,12 @@ m68k_ip (char *instring)
 			  || (! flag_keep_pcrel)))
 		    {
 		      if (HAVE_LONG_BRANCH (current_architecture))
-			add_frag (adds (&opP->disp), offs (&opP->disp),
+			add_frag (adds (&opP->disp),
+				  SEXT (offs (&opP->disp)),
 				  TAB (DBCCLBR, SZ_UNDEF));
 		      else
-			add_frag (adds (&opP->disp), offs (&opP->disp),
+			add_frag (adds (&opP->disp),
+				  SEXT (offs (&opP->disp)),
 				  TAB (DBCCABSJ, SZ_UNDEF));
 		      break;
 		    }
@@ -2856,7 +2865,8 @@ m68k_ip (char *instring)
 		  addword (0);
 		}
 	      else
-		add_frag (adds (&opP->disp), offs (&opP->disp),
+		add_frag (adds (&opP->disp),
+			  SEXT (offs (&opP->disp)),
 			  TAB (FBRANCH, SZ_UNDEF));
 	      break;
 	    default:
@@ -4607,7 +4617,7 @@ md_apply_fix3 (fixS *fixP, valueT *valP,
   buf += fixP->fx_where;
   /* End ibm compiler workaround.  */
 
-  val = ((val & 0xffffffff) ^ 0x80000000) - 0x80000000;
+  val = SEXT (val);
 
   if (fixP->fx_addsy == NULL && fixP->fx_pcrel == 0)
     fixP->fx_done = 1;
@@ -5206,38 +5216,38 @@ get_num (struct m68k_exp *exp, int ok)
       switch (ok)
 	{
 	case 10:
-	  if (offs (exp) < 1 || offs (exp) > 8)
+	  if ((valueT) TRUNC (offs (exp)) - 1 > 7)
 	    {
 	      as_warn (_("expression out of range: defaulting to 1"));
 	      offs (exp) = 1;
 	    }
 	  break;
 	case 20:
-	  if (offs (exp) < 0 || offs (exp) > 7)
+	  if ((valueT) TRUNC (offs (exp)) > 7)
 	    goto outrange;
 	  break;
 	case 30:
-	  if (offs (exp) < 0 || offs (exp) > 15)
+	  if ((valueT) TRUNC (offs (exp)) > 15)
 	    goto outrange;
 	  break;
 	case 40:
-	  if (offs (exp) < 0 || offs (exp) > 32)
+	  if ((valueT) TRUNC (offs (exp)) > 32)
 	    goto outrange;
 	  break;
 	case 50:
-	  if (offs (exp) < 0 || offs (exp) > 127)
+	  if ((valueT) TRUNC (offs (exp)) > 127)
 	    goto outrange;
 	  break;
 	case 55:
-	  if (offs (exp) < -64 || offs (exp) > 63)
+	  if ((valueT) SEXT (offs (exp)) + 64 > 127)
 	    goto outrange;
 	  break;
 	case 60:
-	  if (offs (exp) < -128 || offs (exp) > 127)
+	  if ((valueT) SEXT (offs (exp)) + 128 > 255)
 	    goto outrange;
 	  break;
 	case 70:
-	  if (offs (exp) < 0 || offs (exp) > 4095)
+	  if ((valueT) TRUNC (offs (exp)) > 4095)
 	    {
 	    outrange:
 	      as_warn (_("expression out of range: defaulting to 0"));
@@ -5245,9 +5255,8 @@ get_num (struct m68k_exp *exp, int ok)
 	    }
 	  break;
 	case 80:
-	  if (offs (exp) < -1
-              || offs (exp) > 7
-              || offs (exp) == 0)
+	  if ((valueT) TRUNC (offs (exp)) != 0xffffffff
+              && (valueT) TRUNC (offs (exp)) - 1 > 6)
 	    {
 	      as_warn (_("expression out of range: defaulting to 1"));
 	      offs (exp) = 1;

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre