This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] MIPS/GAS: Accept negative MIPS16 constant %hi expansions


"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> Hi,
>
>  The change allows negative constant %hi expansions in the MIPS16 mode 
> letting code like:
>
> 	.set	mips16
> 	.set	foo, -32769
> blah:
> 	li	$2, %hi(foo)
> 	sll	$2, 16
> 	addiu	$2, %lo(foo)
>
> build successfully -- without the change below an "operand value out of 
> range for instruction" error is produced.
>
>  This is in spirit actually consistent with how we handle %hi in the 
> MIPS16 mode in general.  Consider the piece above.  If foo is an external 
> symbol instead and is only resolved by the linker and the whole program 
> uses 32-bit kernel addresses, then the final value of foo is in fact 
> negative and the linker will let it through and use the signed upper half 
> regardless.  So why not to allow it in expressions that turn out to 
> evaluate to a constant at the assembly time as well?

Agreed.

>  Now as to the change itself -- the 'U' operand/reloc code is used for LI, 
> but it is also used for the CMPI instruction.  The likelihood of someone 
> using %hi with CMPI (or any macros that use it in expansion) is low, but I 
> decided to play safe after all and replaced it with an extra operand code, 
> newly added for use exclusively by LI.  The rest is then mechanical.

But the linker scenario you describe above also applies to constants
that are only known at the end of assembly, rather than when the
instruction is parsed.  So doing it this way keeps the current
inconsistency where:

	.eqv	bar,-40000
	cmp	$4,%hi(bar)

isn't allowed but:

	cmp	$4,%hi(bar)
	.eqv	bar,-40000

is OK.  In contrast:

	cmp	$4,bar
	.eqv	bar,-400

and:

	.eqv	bar,-400
	cmp	$4,bar

are both (rightly) rejected.

Only MIPS16 treats %hi, %lo, etc., as inherently signed.  Other ISA modes
allow things like:

	.eqv	bar,-40000
	ori	$4,$4,%hi(bar)

without complaint.  So not only is the current MIPS16 handling
inconsistent wrt the definition site, it's also inconsistent wrt
the other ISA modes.  I think we should just trust that users of
the relocation operators know what they're doing.

I went for the patch below instead.  Tested on the usual targets.

As you say, the behaviour is covered by your new hi/lo tests,
so I've not added a separate test here.

Richard


gas/
	* config/tc-mips.c (SEXT_16BIT): New macro.
	(mips16_immed): Take the reloc type as a parameter.  Do not impose
	a signed vs. unsigned distinction on the value when a relocation
	operator was used.
	(mips16_macro_build, mips16_ip, md_convert_frag): Pass the reloc
	type to mips16_immed.
	(macro): Use SEXT_16BIT.

Index: gas/config/tc-mips.c
===================================================================
--- gas/config/tc-mips.c	2012-09-23 08:42:18.364466482 +0100
+++ gas/config/tc-mips.c	2012-09-23 08:58:32.755438149 +0100
@@ -1176,6 +1176,9 @@ #define RELAX_MICROMIPS_TOOFAR32(i) (((i
 #define RELAX_MICROMIPS_MARK_TOOFAR32(i) ((i) | 0x40000)
 #define RELAX_MICROMIPS_CLEAR_TOOFAR32(i) ((i) & ~0x40000)
 
+/* Sign-extend 16-bit value X.  */
+#define SEXT_16BIT(X) ((((X) + 0x8000) & 0xffff) - 0x8000)
+
 /* Is the given value a sign-extended 32-bit value?  */
 #define IS_SEXT_32BIT_NUM(x)						\
   (((x) &~ (offsetT) 0x7fffffff) == 0					\
@@ -1321,7 +1324,8 @@ static void mips16_macro (struct mips_cl
 static void mips_ip (char *str, struct mips_cl_insn * ip);
 static void mips16_ip (char *str, struct mips_cl_insn * ip);
 static void mips16_immed
-  (char *, unsigned int, int, offsetT, unsigned int, unsigned long *);
+  (char *, unsigned int, int, bfd_reloc_code_real_type, offsetT,
+   unsigned int, unsigned long *);
 static size_t my_getSmallExpression
   (expressionS *, bfd_reloc_code_real_type *, char *);
 static void my_getExpression (expressionS *, char *);
@@ -5222,7 +5226,7 @@ mips16_macro_build (expressionS *ep, con
 	      *r = (int) BFD_RELOC_UNUSED + c;
 	    else
 	      {
-		mips16_immed (NULL, 0, c, ep->X_add_number,
+		mips16_immed (NULL, 0, c, *r, ep->X_add_number,
 			      0, &insn.insn_opcode);
 		ep = NULL;
 		*r = BFD_RELOC_UNUSED;
@@ -7277,7 +7281,7 @@ macro (struct mips_cl_insn *ip)
 	    {
 	      expr1.X_add_number = offset_expr.X_add_number;
 	      offset_expr.X_add_number =
-		((offset_expr.X_add_number + 0x8000) & 0xffff) - 0x8000;
+		SEXT_16BIT (offset_expr.X_add_number);
 	      load_got_offset (tempreg, &offset_expr);
 	      offset_expr.X_add_number = expr1.X_add_number;
 	      /* If we are going to add in a base register, and the
@@ -7505,8 +7509,7 @@ macro (struct mips_cl_insn *ip)
 
 	      used_at = 1;
 	    }
-	  offset_expr.X_add_number =
-	    ((expr1.X_add_number + 0x8000) & 0xffff) - 0x8000;
+	  offset_expr.X_add_number = SEXT_16BIT (expr1.X_add_number);
 	  relax_switch ();
 
 	  if (gpdelay)
@@ -13397,12 +13400,13 @@ mips16_ip (char *str, struct mips_cl_ins
 			  default:
 			    internalError ();
 			}
-		      *offset_reloc = BFD_RELOC_UNUSED;
 
 		      mips16_immed (NULL, 0, *imm_reloc - BFD_RELOC_UNUSED,
-				    tmp, forced_insn_length, &ip->insn_opcode);
+				    *offset_reloc, tmp, forced_insn_length,
+				    &ip->insn_opcode);
 		      imm_expr.X_op = O_absent;
 		      *imm_reloc = BFD_RELOC_UNUSED;
+		      *offset_reloc = BFD_RELOC_UNUSED;
 		    }
 
 		  return;
@@ -13988,11 +13992,16 @@ #define MIPS16_NUM_IMMED \
    extending it if necessary.  The instruction in *INSN may
    already be extended.
 
-   TYPE is the type of the immediate field.  USER_INSN_LENGTH is the
-   length that the user requested, or 0 if none.  */
+   RELOC is the relocation that produced VAL, or BFD_RELOC_UNUSED
+   if none.  In the former case, VAL is a 16-bit number with no
+   defined signedness.
+
+   TYPE is the type of the immediate field.  USER_INSN_LENGTH
+   is the length that the user requested, or 0 if none.  */
 
 static void
-mips16_immed (char *file, unsigned int line, int type, offsetT val,
+mips16_immed (char *file, unsigned int line, int type,
+	      bfd_reloc_code_real_type reloc, offsetT val,
 	      unsigned int user_insn_length, unsigned long *insn)
 {
   const struct mips16_immed_operand *op;
@@ -14017,11 +14026,15 @@ mips16_immed (char *file, unsigned int l
 	  mintiny = 0;
 	  maxtiny = (1 << op->nbits) - 1;
 	}
+      if (reloc != BFD_RELOC_UNUSED)
+	val &= 0xffff;
     }
   else
     {
       mintiny = - (1 << (op->nbits - 1));
       maxtiny = (1 << (op->nbits - 1)) - 1;
+      if (reloc != BFD_RELOC_UNUSED)
+	val = SEXT_16BIT (val);
     }
 
   /* Branch offsets have an implicit 0 in the lowest bit.  */
@@ -14060,19 +14073,22 @@ mips16_immed (char *file, unsigned int l
       long minext, maxext;
       int extval;
 
-      if (op->extu)
-	{
-	  minext = 0;
-	  maxext = (1 << op->extbits) - 1;
-	}
-      else
+      if (reloc == BFD_RELOC_UNUSED)
 	{
-	  minext = - (1 << (op->extbits - 1));
-	  maxext = (1 << (op->extbits - 1)) - 1;
+	  if (op->extu)
+	    {
+	      minext = 0;
+	      maxext = (1 << op->extbits) - 1;
+	    }
+	  else
+	    {
+	      minext = - (1 << (op->extbits - 1));
+	      maxext = (1 << (op->extbits - 1)) - 1;
+	    }
+	  if (val < minext || val > maxext)
+	    as_bad_where (file, line,
+			  _("operand value out of range for instruction"));
 	}
-      if (val < minext || val > maxext)
-	as_bad_where (file, line,
-		      _("operand value out of range for instruction"));
 
       if (op->extbits == 16)
 	{
@@ -18263,8 +18279,8 @@ md_convert_frag (bfd *abfd ATTRIBUTE_UNU
       else
 	user_length = 0;
 
-      mips16_immed (fragp->fr_file, fragp->fr_line, type, val,
-		    user_length, &insn);
+      mips16_immed (fragp->fr_file, fragp->fr_line, type,
+		    BFD_RELOC_UNUSED, val, user_length, &insn);
 
       length = (ext ? 4 : 2);
       gas_assert (mips16_opcode_length (insn) == length);


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]