[PATCH] Fix sleb128 encoding for difference expressions

Julian Brown julian@codesourcery.com
Fri Nov 9 17:57:00 GMT 2012


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sleb128-fix-6.diff
Type: text/x-patch
Size: 13022 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20121109/3981a19c/attachment.bin>


More information about the Binutils mailing list