120083 - JavaScript toInt32 conversion doesn't match ECMAScript definition

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Chapel]

Reproducable: Always

Steps to Reproduce:

1. Run the following script:

// Define the sign operation: 1 for positive, -1 for negative, else 0
function sign(x) {
    if (x > 0)
        return 1;
    else if (x < 0)
        return -1;
    else
        return 0;
}

// Compute x modulo y, assuming y is positive
function modulo(x, y) {
    var r = x % y;
    return r < 0 ? r + y : r;
}

// Use the built-in toInt32 conversion by using a bitwise operator
function toInt32_builtin(x) {
    return x | 0;
}

// Use the ECMA Script definition to perform toInt32 conversion
function toInt32_ecma(x) {
    if (!isFinite(x) || x == 0)
        return 0;
    var r3 = sign(x) * Math.floor(Math.abs(x));
    var r4 = modulo(r3, 0x100000000);
    return (r4 >= 0x80000000) ? r4 - 0x100000000 : r4;
}

function check(x) {
    var b = toInt32_builtin(x);
    var e = toInt32_ecma(x);
    if (b != e) {
        document.writeln("Original number (dec):  " + x);
        document.writeln("Original number:        " + x.toString(16));
        document.writeln("Built-in conversion:    " + b.toString(16));
        document.writeln("ECMA Script conversion: " + e.toString(16));
        document.writeln();
    }
}

check(2147483648.25);
check(2147483648.5);
check(2147483648.75);
check(4294967295.25);
check(4294967295.5);
check(4294967295.75);


Expected Results:
No output because the built-in toInt32 conversion matches the ECMA Script
definition.


Actual Results:

Original number (dec):  2147483648.25
Original number:        80000000.4
Built-in conversion:    -7fffffff
ECMA Script conversion: -80000000

Original number (dec):  2147483648.5
Original number:        80000000.8
Built-in conversion:    -7fffffff
ECMA Script conversion: -80000000

Original number (dec):  2147483648.75
Original number:        80000000.c
Built-in conversion:    -7fffffff
ECMA Script conversion: -80000000

Original number (dec):  4294967295.25
Original number:        ffffffff.4
Built-in conversion:    0
ECMA Script conversion: -1

Original number (dec):  4294967295.5
Original number:        ffffffff.8
Built-in conversion:    0
ECMA Script conversion: -1

Original number (dec):  4294967295.75
Original number:        ffffffff.c
Built-in conversion:    0
ECMA Script conversion: -1


I first noticed this with Windows build 2002011103, and also see it in build
2002011403.

Comment 1

[Phil Schwartau]

Attachment: Reporter's HTML testcase

Comment 2

[Phil Schwartau]

Confirming behavior with Mozilla trunk binaries from 2001-12 and 2001-01
on WinNT, Linux. OS: ---> All. Reassigning to Kenton. 


In the JS testsuite, the existing testcase for toInt32 conversion is

          mozilla/js/tests/ecma/TypeConversion/9.5-2.js

For some reason (why?) it doesn't test numbers with any digits to 
the right of the decimal point. When I add numbers such as 2147483648.25
to the testcase, it fails similarly to Steve's test above:

            ToInt32(2147483648.25) =  -2147483647 
                    FAILED! expected: -2147483648


If we reduce the test number by 1, the test passes:

            ToInt32(2147483647.25) =  -2147483647


So this issue has to do with numbers near range boundaries, e.g. 0x80000000.
For convenience, here is Steve's output on this number, from above:

            Original number (dec):  2147483648.25
            Original number (hex):     80000000.4
            Built-in conversion:      -7fffffff
            ECMA Script conversion:   -80000000

Comment 3

[Steven Cole]

Actually, it's not edge cases at all.  Any number between 2^31 and 2^32 with a
fractional part is off by one.

check(3000000000.5);

results in

Original number (dec):  3000000000.5
Original number:        b2d05e00.8
Built-in conversion:    -4d2fa1ff
ECMA Script conversion: -4d2fa200

Comment 4

[Steven Cole]

Attachment: Fix for fractional numbers

The error's in the routine js_DoubleToECMAInt32; this patch fixes the problem
by eliminating the fractional portion of the input argument early in the
algorithm.

(Many lines changed due to tab expansion; a diff -wu patch (for reviewing
purposes) will be attached momentarily.)

Comment 5

[Steven Cole]

Attachment: diff -wu version of prior patch

Reviewers have at it.  

Kenton, I lack cvs authority; you (or someone else) will have to check in after
reviewing is complete.

Comment 6

[Phil Schwartau]

cc'ing reviewers for this patch -

Comment 7

[Brendan Eich [:brendan]]

Steven, thanks very much.  Kenton, can you r=?  I'll sr= now and get this in for
mozilla0.9.8.

I'm cc'ing mang for review, if he's around -- also to clarify fd_floor vs. floor
usage (both are called, fd_floor in only a few places, and jslibmath.h seems to
#define fd_floor as floor anyway).

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 65092 [details] [diff] [review]
Fix for fractional numbers

sr=[e-mail]

Comment 9

[Phil Schwartau]

Steve's examples added to existing test in JS testsuite:

        mozilla/js/tests/ecma/TypeConversion/9.5-2.js

This test now fails in the current JS shell in the manner described above:
the current toInt32(x), available to the user as x | 0 or x << 0,
is off by one for any number between 2^31 and 2^32 with a fractional part.

After this patch is checked in, the test will pass again.

As rogerl pointed out to me, the reason that x | 0 and x << 0 produce
toInt32(x) for the user is due to the ECMA algorithms for these operators,
in sections 11.10, 11.7.1 respectively of ECMA-262 3rd Edition Final.

Comment 10

[Kenton Hanson (gone)]

Comment on attachment 65093 [details] [diff] [review]
diff -wu version of prior patch

r=khanson
Should we be testing for these types of errors?

Comment 11

[Phil Schwartau]

Kenton: we are testing for them now; see Comment #9

Comment 12

[Steven Cole]

Phil,

Please add tests for numbers with fractional parts between -2^31 and -2^32 to
the 9.5-2.js testcase as well (this tests the "ceil" path of the patch -- and
breaks my first try at a patch that I never posted...).

Thanks...

Comment 13

[Phil Schwartau]

> Please add tests for numbers with fractional parts between -2^31 and -2^32
> to the 9.5-2.js testcase as well 

Done; thanks -

Comment 14

[Brendan Eich [:brendan]]

Fix is in (I took the liberty of parenthesizing d >= 0 in the ?: condition in
two places, for readability -- |d = d >= 0 ? ... : ...| is less clear to me than
|d = (d >= 0) ? ... : ...|).  Thanks again,

/be

Comment 15

[Phil Schwartau]

Verified FIXED. The above testcase now passes in the debug and optimized
JS shells on WinNT and Linux. Still wrestling with my Mac, but my problems
there are due to the amount of memory I'm allocating to the JS process,
I believe -

Have filed bug 120194 against Rhino for this same issue. The above 
test is failing in Rhino the same way it did in SpiderMonkey after the
new cases were added.

Comment 16

[Phil Schwartau]

> Still wrestling with my Mac...

UPDATE: the above testcase does pass on the Mac, and this bug is verified!
The problem turned out to be mozilla/js/tests/ecma/TypeConversion/9.4-2.js
(not 9.5-2.js). I have filed bug 120725 on the issue.