This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Fix sleb128 encoding for difference expressions
On Thu, 8 Nov 2012 14:02:19 +0000
Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 08.11.12 at 14:05, Julian Brown <julian@codesourcery.com>
> >>> wrote:
> > We encountered a situation where .sleb128 directives would behave
> > in an unexpected way. For a source file such as:
> > the value of "-3" has been interpreted as a (64-bit) unsigned
> > quantity 0xfffffffffffffffd, and henceforth encoded as a sleb128
> > value, whereas what we wanted was simply the encoding "7d", i.e. -3.
> > My proposed fix is a change to the ad-hoc type system for
> > expressions. The basic idea is simply to see if the result of a
> > subtraction operation will fall below zero, and if so force the
> > "unsigned" flag for the result value to be false (normally all of
> > an expression's operands, and its result, are unsigned). No other
> > operators are altered.
>
> I don't think this will go without surprises here and there: Consider
> that expressions like
>
> a - b + c
>
> and
>
> a + c - b
>
> could now evaluate differently (for appropriately chosen values).
Here's a new version of the patch, which hopefully solves this issue.
Explanation below...
> I could see your change being correct for the <symbol> -
> <symbol> case, but certainly not for <constant> - <constant>
> (<symbol> - <constant> being questionable).
I'm not sure I understand this comment. Note that <constant> -
<constant> is just as broken at the moment for .sleb128...
> > This still works for expressions such as,
> >
> > .sleb128 .L2 - .L1 + (1 << 31)
> > (or .sleb128 .L2 - .L1 + (1 << 63))
> >
> > where L2 is greater than L1, i.e. where the sign bit "looks like"
> > it is set, since the result of these will still be regarded as
> > unsigned. There may be other corner cases which behave
> > unexpectedly, however.
> >
> > OK to apply, or any comments? Maybe suggestions for other ways of
> > tackling the problem? (Tested with cross to mips-linux.)
>
> I think that the .sleb128 handler itself might instead need to be
> changed to make the result explicitly signed (as that's what
> directive says) at least in some specific cases.
The trouble is there's no way of distinguishing between any different
cases -- by the time the expression is parsed, it just looks like a
constant (O_constant). So cases such as:
.sleb128 1<<63
which work at present (emitting an unsigned value) would be
sign-extended and thus be emitted as negative numbers. An ad-hoc
solution like interpreting negative numbers as negative only above some
arbitrary limit might be a possibility, but seems very much like it'd
be asking for trouble further down the line.
Instead, I've extended my previous patch as follows:
* The X_add_number field is now treated as a "word-size + 1-bit"
quantity, with the extra (high-order) bit being represented by the
new X_extrabit field of the expressionS structure.
* In each place that values are added to or subtracted from
an expression's X_add_number field, the operation is changed to
perform extended arithmetic (since there's only one extra bit to deal
with, this is quite quick/easy).
* The X_unsigned field is no longer used for determining when and how
to emit sleb128 constants: now, we can use X_extrabit instead, which
should be a much more reliable way of determining if the value is
negative. The behaviour of X_unsigned is left alone, since it's
apparently used for some other purposes in target-specific code, and
I don't want to upset those.
The extended representation can represent all the values of an unsigned
word, and also all the values of a signed word: this means that e.g.
negative differences between labels have a distinct representation from
word-size values which happen to have their high bit set (e.g.
bitmasks, or high addresses) -- and as long as only addition or
subtraction are used, both types of value can be freely intermixed.
I've added some more tests (including the case Alan Modra pointed out,
which now works).
OK to apply, or any further comments?
Thanks,
Julian
ChangeLog
gas/
* read.c (convert_to_bignum): Use X_extrabit instead of X_unsigned.
(emit_leb128_expr): Likewise.
(parse_bitfield_cons): Set X_extrabit.
* expr.c (make_expr_symbol, expr_build_uconstant, operand):
Initialise X_extrabit field as appropriate.
(add_to_result): New.
(subtract_from_result): New.
(expr): Use above.
* expr.h (expressionS): Add X_extrabit field.
gas/testsuite/
* gas/all/sleb128-2.s: New test.
* gas/all/sleb128-3.s: Likewise.
* gas/all/sleb128-4.s: Likewise.
* gas/all/sleb128-5.s: Likewise.
* gas/all/sleb128-6.s: Likewise.
* gas/all/sleb128-7.s: Likewise.
* gas/all/sleb128-2.d: New.
* gas/all/sleb128-3.d: New.
* gas/all/sleb123-4.d: New.
* gas/all/sleb123-5.d: New.
* gas/all/sleb123-6.d: New.
* gas/all/sleb123-7.d: New.
* gas/all/gas.exp (sleb128-2, sleb128-3, sleb128-4, sleb128-5)
(sleb128-6, sleb128-7): Run new tests.
Index: gas/read.c
===================================================================
--- gas/read.c (revision 394269)
+++ gas/read.c (working copy)
@@ -1312,7 +1312,7 @@ read_a_source_file (char *name)
}
/* Convert O_constant expression EXP into the equivalent O_big representation.
- Take the sign of the number from X_unsigned rather than X_add_number. */
+ Take the sign of the number from X_extrabit rather than X_add_number. */
static void
convert_to_bignum (expressionS *exp)
@@ -1328,8 +1328,8 @@ convert_to_bignum (expressionS *exp)
}
/* Add a sequence of sign bits if the top bit of X_add_number is not
the sign of the original value. */
- if ((exp->X_add_number < 0) != !exp->X_unsigned)
- generic_bignum[i++] = exp->X_unsigned ? 0 : LITTLENUM_MASK;
+ if ((exp->X_add_number < 0) == !exp->X_extrabit)
+ generic_bignum[i++] = exp->X_extrabit ? LITTLENUM_MASK : 0;
exp->X_op = O_big;
exp->X_add_number = i;
}
@@ -4684,6 +4684,7 @@ parse_bitfield_cons (exp, nbytes)
exp->X_add_number = value;
exp->X_op = O_constant;
exp->X_unsigned = 1;
+ exp->X_extrabit = 0;
}
}
@@ -5243,7 +5244,7 @@ emit_leb128_expr (expressionS *exp, int
}
else if (op == O_constant
&& sign
- && (exp->X_add_number < 0) != !exp->X_unsigned)
+ && (exp->X_add_number < 0) == !exp->X_extrabit)
{
/* We're outputting a signed leb128 and the sign of X_add_number
doesn't reflect the sign of the original value. Convert EXP
Index: gas/testsuite/gas/all/sleb128-3.d
===================================================================
--- gas/testsuite/gas/all/sleb128-3.d (revision 0)
+++ gas/testsuite/gas/all/sleb128-3.d (revision 0)
@@ -0,0 +1,7 @@
+#objdump : -s -j .data
+#name : .sleb128 tests (3)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 9c7f2a00 00000000 00000000 00000000 .*
Index: gas/testsuite/gas/all/sleb128-5.d
===================================================================
--- gas/testsuite/gas/all/sleb128-5.d (revision 0)
+++ gas/testsuite/gas/all/sleb128-5.d (revision 0)
@@ -0,0 +1,7 @@
+#objdump : -s -j .data
+#name : .sleb128 tests (5)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 012a0000 00000000 00000000 00000000 .*
Index: gas/testsuite/gas/all/gas.exp
===================================================================
--- gas/testsuite/gas/all/gas.exp (revision 394269)
+++ gas/testsuite/gas/all/gas.exp (working copy)
@@ -352,6 +352,12 @@ if { ![istarget "bfin-*-*"] } then {
run_dump_test assign
}
run_dump_test sleb128
+run_dump_test sleb128-2
+run_dump_test sleb128-3
+run_dump_test sleb128-4
+run_dump_test sleb128-5
+run_dump_test sleb128-6
+run_dump_test sleb128-7
# .byte is 32 bits on tic4x, and .p2align isn't supported on tic54x
# .space is different on hppa*-hpux.
Index: gas/testsuite/gas/all/sleb128-7.d
===================================================================
--- gas/testsuite/gas/all/sleb128-7.d (revision 0)
+++ gas/testsuite/gas/all/sleb128-7.d (revision 0)
@@ -0,0 +1,7 @@
+#objdump : -s -j .data
+#name : .sleb128 tests (7)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 cb012ac5 012acb01 2ac5012a 00000000 .*
Index: gas/testsuite/gas/all/sleb128-3.s
===================================================================
--- gas/testsuite/gas/all/sleb128-3.s (revision 0)
+++ gas/testsuite/gas/all/sleb128-3.s (revision 0)
@@ -0,0 +1,4 @@
+.data
+bar:
+.sleb128 100 - 200
+.byte 42
Index: gas/testsuite/gas/all/sleb128-5.s
===================================================================
--- gas/testsuite/gas/all/sleb128-5.s (revision 0)
+++ gas/testsuite/gas/all/sleb128-5.s (revision 0)
@@ -0,0 +1,13 @@
+.text
+.globl foo
+foo:
+.L1:
+.byte 0
+.byte 0
+.byte 0
+.L2:
+
+.data
+bar:
+.sleb128 .L1 - .L2 + 4
+.byte 42
Index: gas/testsuite/gas/all/sleb128-7.s
===================================================================
--- gas/testsuite/gas/all/sleb128-7.s (revision 0)
+++ gas/testsuite/gas/all/sleb128-7.s (revision 0)
@@ -0,0 +1,19 @@
+.text
+.globl foo
+foo:
+.L1:
+.byte 0
+.byte 0
+.byte 0
+.L2:
+
+.data
+bar:
+.sleb128 200+(.L2 - .L1)
+.byte 42
+.sleb128 200+(.L1 - .L2)
+.byte 42
+.sleb128 (.L2 - .L1)+200
+.byte 42
+.sleb128 (.L1 - .L2)+200
+.byte 42
Index: gas/testsuite/gas/all/sleb128-2.d
===================================================================
--- gas/testsuite/gas/all/sleb128-2.d (revision 0)
+++ gas/testsuite/gas/all/sleb128-2.d (revision 0)
@@ -0,0 +1,7 @@
+#objdump : -s -j .data
+#name : .sleb128 tests (2)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 7d2a0000 00000000 00000000 00000000 .*
Index: gas/testsuite/gas/all/sleb128-4.d
===================================================================
--- gas/testsuite/gas/all/sleb128-4.d (revision 0)
+++ gas/testsuite/gas/all/sleb128-4.d (revision 0)
@@ -0,0 +1,7 @@
+#objdump : -s -j .data
+#name : .sleb128 tests (4)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 83808080 082a0000 00000000 00000000 .*
Index: gas/testsuite/gas/all/sleb128-6.d
===================================================================
--- gas/testsuite/gas/all/sleb128-6.d (revision 0)
+++ gas/testsuite/gas/all/sleb128-6.d (revision 0)
@@ -0,0 +1,9 @@
+#objdump : -s -j .data
+#name : .sleb128 tests (6)
+
+.*: .*
+
+Contents of section \.data:
+ 0000 83808080 80808080 80012afd ffffffff .*
+ 0010 ffffffff 002a8380 80808080 80808001 .*
+ 0020 2afdffff ffffffff ffff002a 00000000 .*
Index: gas/testsuite/gas/all/sleb128-2.s
===================================================================
--- gas/testsuite/gas/all/sleb128-2.s (revision 0)
+++ gas/testsuite/gas/all/sleb128-2.s (revision 0)
@@ -0,0 +1,13 @@
+.text
+.globl foo
+foo:
+.L1:
+.byte 0
+.byte 0
+.byte 0
+.L2:
+
+.data
+bar:
+.sleb128 .L1 - .L2
+.byte 42
Index: gas/testsuite/gas/all/sleb128-4.s
===================================================================
--- gas/testsuite/gas/all/sleb128-4.s (revision 0)
+++ gas/testsuite/gas/all/sleb128-4.s (revision 0)
@@ -0,0 +1,13 @@
+.text
+.globl foo
+foo:
+.L1:
+.byte 0
+.byte 0
+.byte 0
+.L2:
+
+.data
+bar:
+.sleb128 .L2 - .L1 + (1 << 31)
+.byte 42
Index: gas/testsuite/gas/all/sleb128-6.s
===================================================================
--- gas/testsuite/gas/all/sleb128-6.s (revision 0)
+++ gas/testsuite/gas/all/sleb128-6.s (revision 0)
@@ -0,0 +1,19 @@
+.text
+.globl foo
+foo:
+.L1:
+.byte 0
+.byte 0
+.byte 0
+.L2:
+
+.data
+bar:
+.sleb128 1<<63+(.L2 - .L1)
+.byte 42
+.sleb128 1<<63+(.L1 - .L2)
+.byte 42
+.sleb128 (.L2 - .L1)+1<<63
+.byte 42
+.sleb128 (.L1 - .L2)+1<<63
+.byte 42
Index: gas/expr.c
===================================================================
--- gas/expr.c (revision 394269)
+++ gas/expr.c (working copy)
@@ -90,6 +90,7 @@ make_expr_symbol (expressionS *expressio
zero.X_op = O_constant;
zero.X_add_number = 0;
zero.X_unsigned = 0;
+ zero.X_extrabit = 0;
clean_up_expression (&zero);
expressionP = &zero;
}
@@ -161,6 +162,7 @@ expr_build_uconstant (offsetT value)
e.X_op = O_constant;
e.X_add_number = value;
e.X_unsigned = 1;
+ e.X_extrabit = 0;
return make_expr_symbol (&e);
}
@@ -732,6 +734,7 @@ operand (expressionS *expressionP, enum
something like ``.quad 0x80000000'' is not sign extended even
though it appears negative if valueT is 32 bits. */
expressionP->X_unsigned = 1;
+ expressionP->X_extrabit = 0;
/* Digits, assume it is a bignum. */
@@ -1026,6 +1029,8 @@ operand (expressionS *expressionP, enum
This is compatible with other people's
assemblers. Sigh. */
expressionP->X_unsigned = 0;
+ if (expressionP->X_add_number)
+ expressionP->X_extrabit ^= 1;
}
else if (c == '~' || c == '"')
expressionP->X_add_number = ~ expressionP->X_add_number;
@@ -1078,6 +1083,7 @@ operand (expressionS *expressionP, enum
expressionP->X_add_number = i >= expressionP->X_add_number;
expressionP->X_op = O_constant;
expressionP->X_unsigned = 1;
+ expressionP->X_extrabit = 0;
}
}
else if (expressionP->X_op != O_illegal
@@ -1717,6 +1723,42 @@ operatorf (int *num_chars)
/* NOTREACHED */
}
+/* Implement "word-size + 1 bit" addition for
+ {resultP->X_extrabit:resultP->X_add_number} + {rhs_highbit:amount}. This
+ is used so that the full range of unsigned word values and the full range of
+ signed word values can be represented in an O_constant expression, which is
+ useful e.g. for .sleb128 directives. */
+
+static void
+add_to_result (expressionS *resultP, offsetT amount, int rhs_highbit)
+{
+ valueT ures = resultP->X_add_number;
+ valueT uamount = amount;
+
+ resultP->X_add_number += amount;
+
+ resultP->X_extrabit ^= rhs_highbit;
+
+ if (ures + uamount < ures)
+ resultP->X_extrabit ^= 1;
+}
+
+/* Similarly, for subtraction. */
+
+static void
+subtract_from_result (expressionS *resultP, offsetT amount, int rhs_highbit)
+{
+ valueT ures = resultP->X_add_number;
+ valueT uamount = amount;
+
+ resultP->X_add_number -= amount;
+
+ resultP->X_extrabit ^= rhs_highbit;
+
+ if (ures < uamount)
+ resultP->X_extrabit ^= 1;
+}
+
/* Parse an expression. */
segT
@@ -1829,7 +1871,7 @@ expr (int rankarg, /* Larger # is highe
&& (md_register_arithmetic || resultP->X_op != O_register))
{
/* X + constant. */
- resultP->X_add_number += right.X_add_number;
+ add_to_result (resultP, right.X_add_number, right.X_extrabit);
}
/* This case comes up in PIC code. */
else if (op_left == O_subtract
@@ -1847,10 +1889,11 @@ expr (int rankarg, /* Larger # is highe
symbol_get_frag (right.X_add_symbol),
&frag_off))
{
- resultP->X_add_number -= right.X_add_number;
- resultP->X_add_number -= frag_off / OCTETS_PER_BYTE;
- resultP->X_add_number += (S_GET_VALUE (resultP->X_add_symbol)
- - S_GET_VALUE (right.X_add_symbol));
+ offsetT symval_diff = S_GET_VALUE (resultP->X_add_symbol)
+ - S_GET_VALUE (right.X_add_symbol);
+ subtract_from_result (resultP, right.X_add_number, right.X_extrabit);
+ subtract_from_result (resultP, frag_off / OCTETS_PER_BYTE, 0);
+ add_to_result (resultP, symval_diff, symval_diff < 0);
resultP->X_op = O_constant;
resultP->X_add_symbol = 0;
}
@@ -1858,7 +1901,7 @@ expr (int rankarg, /* Larger # is highe
&& (md_register_arithmetic || resultP->X_op != O_register))
{
/* X - constant. */
- resultP->X_add_number -= right.X_add_number;
+ subtract_from_result (resultP, right.X_add_number, right.X_extrabit);
}
else if (op_left == O_add && resultP->X_op == O_constant
&& (md_register_arithmetic || right.X_op != O_register))
@@ -1867,7 +1910,7 @@ expr (int rankarg, /* Larger # is highe
resultP->X_op = right.X_op;
resultP->X_add_symbol = right.X_add_symbol;
resultP->X_op_symbol = right.X_op_symbol;
- resultP->X_add_number += right.X_add_number;
+ add_to_result (resultP, right.X_add_number, right.X_extrabit);
retval = rightseg;
}
else if (resultP->X_op == O_constant && right.X_op == O_constant)
@@ -1907,7 +1950,9 @@ expr (int rankarg, /* Larger # is highe
/* Constant + constant (O_add) is handled by the
previous if statement for constant + X, so is omitted
here. */
- case O_subtract: resultP->X_add_number -= v; break;
+ case O_subtract:
+ subtract_from_result (resultP, v, 0);
+ break;
case O_eq:
resultP->X_add_number =
resultP->X_add_number == v ? ~ (offsetT) 0 : 0;
@@ -1951,10 +1996,11 @@ expr (int rankarg, /* Larger # is highe
resultP->X_op = op_left;
resultP->X_op_symbol = right.X_add_symbol;
if (op_left == O_add)
- resultP->X_add_number += right.X_add_number;
+ add_to_result (resultP, right.X_add_number, right.X_extrabit);
else if (op_left == O_subtract)
{
- resultP->X_add_number -= right.X_add_number;
+ subtract_from_result (resultP, right.X_add_number,
+ right.X_extrabit);
if (retval == rightseg
&& SEG_NORMAL (retval)
&& !S_FORCE_RELOC (resultP->X_add_symbol, 0)
@@ -1974,6 +2020,7 @@ expr (int rankarg, /* Larger # is highe
resultP->X_op = op_left;
resultP->X_add_number = 0;
resultP->X_unsigned = 1;
+ resultP->X_extrabit = 0;
}
if (retval != rightseg)
Index: gas/expr.h
===================================================================
--- gas/expr.h (revision 394269)
+++ gas/expr.h (working copy)
@@ -136,6 +136,11 @@ typedef struct expressionS {
when performing arithmetic on these values).
FIXME: This field is not set very reliably. */
unsigned int X_unsigned : 1;
+ /* This is used to implement "word size + 1 bit" arithmetic, so that e.g.
+ expressions used with .sleb128 directives can use the full range available
+ for an unsigned word, but can also properly represent all values of a
+ signed word. */
+ unsigned int X_extrabit : 1;
/* 7 additional bits can be defined if needed. */