Closed Bug 507993 Opened 10 years ago Closed 10 years ago

parseInt of negative double rounds down instead of towards zero (jit)

Categories

(Core :: JavaScript Engine, defect, P2, major)

1.9.1 Branch
defect

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- .4-fixed

People

(Reporter: maksverver, Assigned: dvander)

References

()

Details

(Keywords: verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-GB; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-GB; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

When evaluating the expression parseInt(-1501014982/88) Firefox incorrectly computes a result of -17056989. Since -1501014982/88 = -17056988.431818184 and parseInt() should parse all the digits up to the decimal point and ignore the rest (effectively, rounding towards zero) the result should be -17056988 instead.

This behaviour started in Firefox 3.5. If the JIT is disabled the function behaves correctly.

The bug occurs with Firefox 3.5.1 (Windows and Linux) and Firefox 3.5.2 (candidate build 1). This particular test case seems to work correctly in the current development version, but you may want to verify that the underlying problem has actually been solved.

Reproducible: Always

Steps to Reproduce:
Go to the URL above and press "Test" a few times.
Actual Results:  
The first one or two times the result will be -17056988 (correct) but after that the JIT kicks in and the result is -17056989 (incorrect) instead.

Expected Results:  
The result should be -17056988 every time.
window for when this was fixed on mozilla-central:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1b2f9a366ca&tochange=d54a92a63aa7
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → 1.9.1 Branch
good: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080909032504 Minefield/3.1b1pre
bad: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080910043000 Minefield/3.1b1pre

Maybe caused by bug 454037?
This was fixed in mozilla-central by the checkin for bug 500580. Looks like jit is being disabled when it shouldn't be, so the underlying cause probably hasn't been fixed.

Now to find the original regressing changeset...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → major
Summary: parseInt after division rounds off incorrectly → parseInt of negative double rounds down instead of towards zero (jit)
Attached file reduced testcase
Expected: 0
Actual: -1
Blocks: 454037
Looks like ParseIntDouble is all wrong, floor(x) isn't the same as js_strtointeger. Taking, since I broke it.
Assignee: general → dvander
If I understand this correctly, the JIT emits code equivalent to floor(x) for parseInt(x) if x is a floating-point value. Would simply using trunc(x) instead fix it, or does this break other stuff?
Attached patch replicates num_parseInt better (obsolete) — Splinter Review
(In reply to comment #5)
Thanks for the reduced test case!

It seems the behaviour is a little more quirky than expected.
Attachment #393675 - Flags: review?(brendan)
Comment on attachment 393675 [details] [diff] [review]
replicates num_parseInt better

David, could you please add

[defaults]
diff = -U 8 -p

to your .hgrc (you may need other settings, but I'm ignorant of the details -- mrbkap or jorendorff can help if necessary).

>diff -r 5041273d214f js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp	Mon Aug 10 16:19:55 2009 -0700
>+++ b/js/src/jsnum.cpp	Mon Aug 10 17:17:15 2009 -0700
>@@ -208,7 +208,14 @@
> {
>     if (!JSDOUBLE_IS_FINITE(d))
>         return js_NaN;
>-    return floor(d);
>+    /* Don't preserve -0, because js_strtointeger doesn't */

Is that right? Reading js_strtointeger it seems *dp = negative ? -value : value; will be reached with 0 in value and true in negative. Indeed testing parseInt shows the comment is incorrect:

js> d = parseInt("-0")
0
js> Math.atan2(d, d)
-3.141592653589793
js> Math.atan2(d, 0)
0
js> Math.atan2(0, 0) 
0
js> Math.atan2(0, d) 
3.141592653589793

/be
If you parseInt("-0") you get -0, but if you parseInt(-0) you get 0.

I'm not sure what that means for this function though :)
From ECMA spec, 15.1.2.2, step 1 of parseInt says to call ToString() on the input. 9.8.1, ToString(num) says:

> If m is +0 or −0, return the string "0".

So I guess the functionality of parseInt(-0) is acting correctly.
Comment on attachment 393675 [details] [diff] [review]
replicates num_parseInt better

>diff -r 5041273d214f js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp	Mon Aug 10 16:19:55 2009 -0700
>+++ b/js/src/jsnum.cpp	Mon Aug 10 17:17:15 2009 -0700
>@@ -208,7 +208,14 @@
> {
>     if (!JSDOUBLE_IS_FINITE(d))
>         return js_NaN;
>-    return floor(d);
>+    /* Don't preserve -0, because js_strtointeger doesn't */
>+    if (d == 0)
>+        return 0;
>+    if (d > 0)
>+        return floor(d);
>+    d = ceil(d);
>+    /* ceil does not seem to return -0 if not given -0 */
>+    return (d == 0) ? -0.0 : d;

Ok, thanks -- I should've seen that -- but then why do you test again for -0 here? Control flow won't reach here in C or C++ if d is -0 IINM; you'll return 0 early above.

Nit: blank line before any comment taking one or more lines unless indented after a left brace.

Nit: periods at end of comment sentences.

/be
Comment on attachment 393675 [details] [diff] [review]
replicates num_parseInt better

r=me, I give up! This is correct, sorry for being dense.

Comment style nits stand.

/be
Attachment #393675 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/990b60c301f0

with nits picked (and .hgrc amended!)
Whiteboard: fixed-in-tracemonkey
Well, I suggested this last week:

{
    if (!JSDOUBLE_IS_FINITE(d))
        return js_NaN;
    return trunc(d);
}

This depends on trunc() (like floor() and ceil()) preserving the sign of its operand, which is definitely true on my system, although I can't say if it's universally true.

If ceil() and trunc() are as unreliable as you suggest, then you can still simplify the code to:

{
    if (!JSDOUBLE_IS_FINITE(d))
        return js_NaN;
    if (d > 0)
        return floor(d);
    if (d < 0)
        return -floor(-d);
    return 0;
}

This only assumes floor(d) == 0 if (0<d<1) which, I think, is undisputed.

(To be honest, I find it strange that the result of ceil(-0.1) would be undefined, as you claim. Do you have any proof for this?)
Hardware: x86 → All
I didn't say ceil() was undefined. It is weird on my system though (OS X). The compiler reduces constant ceil(-0.1) to -0. But if I pass a variable containing -0.1, it returns 0. I don't know what that's all about, but your version is a bit simpler and looks like it'll work. Feel free to attach a patch and I'll stamp it (or I can attach if it's a problem).
Really? Interesting. How are you testing? If I try this program:

#include <stdio.h>
#include <math.h>
int main(int argc, char *argv[])
{
        float d;
        if (argc > 1 && sscanf(argv[1], "%f", &d) == 1)
        {
                printf("floor(%f) => %08x\n", d, floorf(d));
                printf("ceil (%f) => %08x\n", d, ceilf(d));
                printf("trunc(%f) => %08x\n", d, truncf(d));
        }
        return 0;
}

And run it with -0.1 as an argument, I get the expected result of:

  floor(-0.100000) => b30c8950
  ceil (-0.100000) => b9538000
  trunc(-0.100000) => 80000000

But if the output of ceil() and trunc() is different on other supported platforms, then they are probably useless. (I print a hex string because not all printf() implementations show -0 with the minus sign.)

By the way, it occurs to me that trunc() is C99 function; MSVC doesn't support C99, so if that's a compiler you want to support, the short version with trunc() doesn't work anyway.
Well, here's what I get on my system with your program :)

keima:~ dvander$ ./test -0.1
floor(-0.100000) => 00000000
ceil (-0.100000) => 00000000
trunc(-0.100000) => 00000000
%f does not print -0 as "-0", does it?

I think we should take the simplest patch that's portable. Maks's patch looks like the winner. David, followup bug or fast followup patch.

/be
http://hg.mozilla.org/mozilla-central/rev/990b60c301f0
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 515380
tracking-fennec: --- → ?
Flags: blocking1.9.2?
Attachment #393675 - Flags: approval1.9.1.4?
This was reported again, so I am assuming this is biting people in the real world. We should fix it in 3.5 as well.
nit: There's a tab in the checked in patch. Convention is to use four spaces.

Also, I'm not convinced that this floor hackery is necessary. Can someone clarify what the exact problem is with trunc and how it differs from the output we want?
We seem to have landed a different simpler patch on TM:

http://hg.mozilla.org/tracemonkey/rev/6c02f6fd869d
Comment on attachment 393675 [details] [diff] [review]
replicates num_parseInt better

We landed the patch from Maks (comment 15). Please attach and nominate that.

/be
Attached patch patchSplinter Review
Comment on attachment 400319 [details] [diff] [review]
patch

Reviewed by brendan and dvander. Already landed on TM.
Attachment #400319 - Flags: approval1.9.1.4?
Attachment #393675 - Attachment is obsolete: true
Attachment #393675 - Flags: approval1.9.1.4?
Comment on attachment 400319 [details] [diff] [review]
patch

Let's get it on 1.9.2 before 1.9.1 -- sayrer will do the magicks in time, I have no doubt.
Attachment #400319 - Flags: approval1.9.2+
blocking 1.9.2 P2.  Not clear if this is on branch, so adding needs-1.9.2-patch in whiteboard.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey needs-1.9.2-patch
This landed on 1.9.2 as of this cset:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f58ad6150a83
Whiteboard: fixed-in-tracemonkey needs-1.9.2-patch → fixed-in-tracemonkey
Comment on attachment 400319 [details] [diff] [review]
patch

Approved for 1.9.1.4, a=dveditz
Attachment #400319 - Flags: approval1.9.1.4? → approval1.9.1.4+
(In reply to comment #31)
> This landed on 1.9.2 as of this cset:
> 
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f58ad6150a83

Marking the bug accordingly...
verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20091001 Shiretoko/3.5.4pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20090930 Minefield/3.7a1pre
Status: RESOLVED → VERIFIED
Keywords: verified1.9.1
http://hg.mozilla.org/tracemonkey/rev/990b60c301f0

js/src/trace-test.js
Flags: in-testsuite+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.