This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] MIPS/GAS: Accept negative MIPS16 constant %hi expansions
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: "Maciej W. Rozycki" <macro at codesourcery dot com>
- Cc: <binutils at sourceware dot org>
- Date: Sun, 23 Sep 2012 11:02:45 +0100
- Subject: Re: [PATCH] MIPS/GAS: Accept negative MIPS16 constant %hi expansions
- References: <alpine.DEB.1.10.1209211910020.28358@tp.orcam.me.uk>
"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);