Maks Verver Reporter
	Description • 15 years ago

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.

	Michael Ryan
	Comment 2 • 15 years ago

window for when this was fixed on mozilla-central:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1b2f9a366ca&tochange=d54a92a63aa7

	Michael Ryan
	Comment 3 • 15 years ago

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?

	Michael Ryan
	Comment 4 • 15 years ago

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...

	Michael Ryan
	Comment 5 • 15 years ago

Expected: 0
Actual: -1

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 15 years ago

Looks like ParseIntDouble is all wrong, floor(x) isn't the same as js_strtointeger. Taking, since I broke it.

	Maks Verver Reporter
	Comment 7 • 15 years ago

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?

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 8 • 15 years ago

(In reply to comment #5)
Thanks for the reduced test case!

It seems the behaviour is a little more quirky than expected.

	Brendan Eich [:brendan]
	Comment 9 • 15 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 10 • 15 years ago

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 :)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 15 years ago

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.

	Brendan Eich [:brendan]
	Comment 12 • 15 years ago

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

	Brendan Eich [:brendan]
	Comment 13 • 15 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/990b60c301f0

with nits picked (and .hgrc amended!)

	Maks Verver Reporter
	Comment 15 • 15 years ago

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?)

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 16 • 15 years ago

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).

	Maks Verver Reporter
	Comment 17 • 15 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 18 • 15 years ago

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

	Brendan Eich [:brendan]
	Comment 19 • 15 years ago

%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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 20 • 15 years ago

(In reply to comment #19)

http://hg.mozilla.org/tracemonkey/rev/6c02f6fd869d

	Robert Sayre
	Comment 21 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/990b60c301f0

	Andreas Gal :gal
	Comment 23 • 15 years ago

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.

	Michael Ryan
	Comment 24 • 15 years ago

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?

	Andreas Gal :gal
	Comment 25 • 15 years ago

We seem to have landed a different simpler patch on TM:

http://hg.mozilla.org/tracemonkey/rev/6c02f6fd869d

	Brendan Eich [:brendan]
	Comment 26 • 15 years ago

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

	Andreas Gal :gal
	Comment 28 • 15 years ago

Comment on attachment 400319 [details] [diff] [review]
patch

Reviewed by brendan and dvander. Already landed on TM.

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 29 • 15 years ago

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.

	Damon Sicore (:damons)
	Comment 30 • 15 years ago

blocking 1.9.2 P2.  Not clear if this is on branch, so adding needs-1.9.2-patch in whiteboard.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 31 • 15 years ago

This landed on 1.9.2 as of this cset:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f58ad6150a83

	Daniel Veditz [:dveditz]
	Comment 32 • 15 years ago

Comment on attachment 400319 [details] [diff] [review]
patch

Approved for 1.9.1.4, a=dveditz

	Jeff Walden [:Waldo]
	Comment 33 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/841360c52c94

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 34 • 15 years ago

(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...

	Tracy Walker [:tracy]
	Comment 35 • 15 years ago

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

	Bob Clary [:bc] (inactive)
	Comment 36 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/990b60c301f0

js/src/trace-test.js