620532 - TM: integer promotion/demotion doesn't distinguish signed vs unsigned sufficiently

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

billm spotted this in relation to Math.min and Math.max.  Here's a sample
program that is buggy under the tracejit:

    for (var i = 0; i < 20; i++) {
        m = Math.min(0x80000000, 0);
    }
    print(m);

    var buffer = new ArrayBuffer(4);
    var uint32View = new Uint32Array(buffer);
    uint32View[0] = -1;
    var m;
    for (var i = 0; i < 20; i++) {
        m = Math.min(uint32View[0], 0);
    }
    print(m);

Under the interpreter or the methodjit, this prints:

  0
  0

Under the tracejit it prints:

  -2147483648
  -1

It's because the min/max code in callNative() uses a signed comparison
regardless of whether the operands are signed or unsigned.  That one is easy to fix, but there may be others like it.  We need to audit all our IsPromote() calls to see if they should be IsPromoteInt()/IsPromoteUint().  Also, Demote() doesn't have signed vs. unsigned cases, perhaps it should.  And it would be nice to make all this stuff less error-prone.

I suspect this isn't a regression, but it still feels like a 2.0 blocker to me.

Comment 1

[Nicholas Nethercote [inactive]]

Jesse, do the fuzzers produce uint32 values much?  If not, it would be great if they did.  These kind of signed/unsigned mix-ups feel like the kind of thing fuzzers should be great at finding.

Comment 0 shows the two ways you can get unsigned integers, in the tracer at least:  integer immediates with values in the range 0x80000000..0xffffffff, and UInt typed arrays.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 58623:e10bd9b6cea0)

This patch:

- Fixes the bogus min/max case.  I looked through every call to
  IsPromote{,Int,Uint} and demote()/Demote() and this was the only one that
  looked wrong to me.  But signed vs. unsigned problems can be subtle and
  we'll need bug 620561 to be implemented before I'm confident we don't have
  further problems.

- Renames/replaces the following functions:

  - IsPromote       --> IsPromotedInt32OrUint32
  - IsPromoteInt    --> IsPromotedInt32
  - IsPromoteUint   --> IsPromotedUint32
  - Demote          --> DemoteToInt32, DemoteToUint32

  These changes make the signedness of things clearer, the sizes of things
  clearer, the meaning of promote/demote clearer (ie. which direction is
  demotion, something multiple people have found confusing) and the new
  names are more grammatically correct.  A few variables/fields in
  jstracer.cpp were renamed similarly.

- Removes a case from d2i().  I found this tricky to verify and it turns out
  not to be important -- at least, it's never hit in Sunspider.

- Removes the ins->isCall() case from DemoteTo{Int32,Uint32}().  I think
  it's a hangover from when softfloat was supported by the tracer.  It never
  triggers because IsPromoted{Int32,Uint32} don't succeed on a call and
  DemoteTo{Int32,Uint32} are only called if IsPromoted{Int32,Uint32}
  succeed.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 498926 [details] [diff] [review]
patch (against TM 58623:e10bd9b6cea0)

This looks good to me. It would be nice to have a explicit assertions in both Demotes that check the IsPromote condition. This might make it harder for them to fall out of sync.

Also, could you take a look at TraceRecorder::getCharCodeAt/getCharAt? It calls makeNumberInt32, which returns a signed integer. But then it immediately passes it to ui2p, which will fail to sign-extend. This might be okay, since it's doing a bounds check. However, the bound it's comparing with may be larger than 2**32 (I'm assuming this, since JSString::LENGTH_SHIFT is only 4). So for really huge strings, a negative index might just be treated as a really big index.

r+ with these changes/audits.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Might be good to check with gal why that i2d case is there...

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> 
> This looks good to me. It would be nice to have a explicit assertions in both
> Demotes that check the IsPromote condition. This might make it harder for them
> to fall out of sync.

Good idea.

> Also, could you take a look at TraceRecorder::getCharCodeAt/getCharAt? It calls
> makeNumberInt32, which returns a signed integer. But then it immediately passes
> it to ui2p, which will fail to sign-extend. This might be okay, since it's
> doing a bounds check. However, the bound it's comparing with may be larger than
> 2**32 (I'm assuming this, since JSString::LENGTH_SHIFT is only 4). So for
> really huge strings, a negative index might just be treated as a really big
> index.

JSString::MAX_LENGTH is 2**28 - 1.  So if you have a negative argument to charCode/charCodeAt, in the LIR_ltup that does the bounds check it'll be treated as unsigned and thus be larger than 2**31, and thus be out of bounds, which is just what we want, AFAICT.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 498926 [details] [diff] [review]
patch (against TM 58623:e10bd9b6cea0)

gal wants to keep the d2i() case, I propose changing it to use IsPromotedInt32/DemoteToInt32, because that's easier to think about and the unsigned case is rare.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Another question: is the d->isop(LIR_ui2d) case safe in d2i? It seems like it might not be.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch v2 (against TM 58637:0be4f01086ea)

(In reply to comment #7)
> Another question: is the d->isop(LIR_ui2d) case safe in d2i? It seems like it
> might not be.

Good question.  After some head-scratching and experimenting with the JS shell I concluded that it is safe.  I added a comment with an explanation and example, but I'm not 100% happy with it;  suggestions for more convincing explanations are welcome.

It is an important case, though.  In crypto-md5 we have this diff if it's removed:

   ------------------------------ # JSOP_URSH
       rshui64 = rshui ori369, immi126/*25*/
+      ui2d64 = ui2d rshui64
   ------------------------------ # JSOP_BITOR
-      ori368 = ori lshi336, rshui64
+      js_DoubleToInt32_64 = calli.none #js_DoubleToInt32 ( ui2d64 )
+      ori368 = ori lshi336, js_DoubleToInt32_64

crypto-sha1 is affected similarly.

I also reinstated the addi/subi case in d2i() on gal's request, but only for signed integers.

The patch also avoids a compiler warning about an unused var in opt builds in 
SetMaxCodeCacheBytes().

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 499424 [details] [diff] [review]
patch v2 (against TM 58637:0be4f01086ea)

I like the comment. I think the thing that I wasn't clear about is that d2i always succeeds. It never leaves with MISMATCH_EXIT or anything like that, even if the argument doesn't fit in an integer. Could you maybe add a comment to the top of it saying something like that?

Comment 10

[Jesse Ruderman]

The first bad revision is:
changeset:   ccf91ba2d62a
user:        Andreas Gal
date:        Wed Aug 19 15:31:10 2009 -0700
summary:     Specialize math functions to integer arithmetic where appropriate (bug 511307, r=dvander).

Comment 11

[Jesse Ruderman]

(That's for the first part of the testcase -- the part without the typed arrays.)

Comment 12

[Jesse Ruderman]

I'm guessing this is the same bug:

for (var j=0; j<9; ++j) { m = (Math.max(0x80000000, 0)); } print(m);

Assertion failure: hasInt32Repr(*vp), at jstracer.cpp:4028

Comment 13

[Nicholas Nethercote [inactive]]

(In reply to comment #12)
> I'm guessing this is the same bug:
> 
> for (var j=0; j<9; ++j) { m = (Math.max(0x80000000, 0)); } print(m);
> 
> Assertion failure: hasInt32Repr(*vp), at jstracer.cpp:4028

With the patch applied the assertion doesn't happen (and it prints 2147483648).  So, yes, it's the same bug :)

Comment 14

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/cd2898655d25

Comment 15

[Brendan Eich [:brendan]]

njn++ -- grammarians can sleep tonight!

/be

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/cd2898655d25

Comment 17

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.