Closed
Bug 120083
Opened 23 years ago
Closed 23 years ago
JavaScript toInt32 conversion doesn't match ECMAScript definition
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: schapel, Assigned: khanson)
Details
Attachments
(3 files)
1.36 KB,
text/html
|
Details | |
746 bytes,
patch
|
brendan
:
superreview+
|
Details | Diff | Splinter Review |
497 bytes,
patch
|
khanson
:
review+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Comment 2•23 years ago
|
||
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
Assignee: rogerl → khanson
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Hardware: PC → All
Summary: JavaScript toInt32 conversion does match ECMA Script definition → JavaScript toInt32 conversion doesn't match ECMAScript definition
Comment 3•23 years ago
|
||
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•23 years ago
|
||
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•23 years ago
|
||
Reviewers have at it. Kenton, I lack cvs authority; you (or someone else) will have to check in after reviewing is complete.
Comment 6•23 years ago
|
||
cc'ing reviewers for this patch -
Comment 7•23 years ago
|
||
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•23 years ago
|
||
Comment on attachment 65092 [details] [diff] [review] Fix for fractional numbers sr=brendan@mozilla.org
Attachment #65092 -
Flags: superreview+
Comment 9•23 years ago
|
||
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.
Assignee | ||
Comment 10•23 years ago
|
||
Comment on attachment 65093 [details] [diff] [review] diff -wu version of prior patch r=khanson Should we be testing for these types of errors?
Attachment #65093 -
Flags: review+
Comment 11•23 years ago
|
||
Kenton: we are testing for them now; see Comment #9
Comment 12•23 years ago
|
||
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•23 years ago
|
||
> 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•23 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 15•23 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Comment 16•23 years ago
|
||
> 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.
Updated•19 years ago
|
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•