320770 - ecma/Math/15.8.2.13 - Math.pow on MacOSX

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

recent 1.8/1.9 builds on MacOSX

Math.pow(1,NaN) = 1  FAILED! expected: NaN
Math.pow(1, Infinity) = 1  FAILED! expected: NaN
Math.pow(1, -Infinity) = 1  FAILED! expected: NaN
Math.pow(-1, Infinity) = 1  FAILED! expected: NaN
Math.pow(-1, -Infinity) = 1  FAILED! expected: NaN

Comment 1

[Brendan Eich [:brendan]]

Nothing's changed in SpiderMonkey -- has Mac OS X changed libm, or are we using fdlibm on Mac?  If the latter, what could have changed?  Apple's gcc?

/be

Comment 2

[Josh Aas]

Bob - what is your Mac OS X version?

Comment 3

[Josh Aas]

This fails on Mac OS X 10.4 (I tested) and Mac OS X 10.3 (Pinkerton tested). It fails using FF 1.0.7 and FF 1.5 for me on Mac OS X 10.4. Pinkerton only tested FF 1.5 on 10.3, but I assume FF 1.0.7 would also fail on 10.3.

This is probably a SpiderMonkey problem. I don't know where the Mac OS X code for this stuff is in SpiderMonkey, I can audit it looking for problems if someone wants to point me in the right direction.

Comment 4

[Josh Aas]

fyi, Mac OS X appears to us libm, not fdlibm.

Comment 5

[Josh Aas]

Attachment: fix v1.0

Mac OS X can actually use fdlibm.

Comment 6

[Brendan Eich [:brendan]]

We were hoping to use more OS libm implementations instead of old fdlibm, and benefit from whatever numerical optimizations the OS vendors purvey (instead of having to optimize fdlibm).  Are the results in comment 0 expected from OS X libm? There are differences between ECMA-262's Math.* methodso and POSIX, but those did not ring a bell.

/be

Comment 7

[Josh Aas]

I agree. I didn't understand much of anything about math libs or the way they are used, so my previous comments are pretty uninformed. All I knew was that my patch made the problem go away :)

I've done some research now, and I'm working on another solution.

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to comment #2)
> Bob - what is your Mac OS X version?
> 

10.4.3

Comment 9

[Josh Aas]

Thoughts, brainstorming...

Windows and Linux use fdlibm, and Mac OS X is the only tier-1 platform that uses libm. That is why Mac OS X has this bug - js is not correctly handling the return value from native libm calls. For example:

Math.pow(1,NaN) = 1 // libm (Linux, Mac OS X)
Math.pow(1,NaN) = NaN // fdlibm

You can't just substitute libm calls for fdlibm. So we have a couple of options here:

1. When a platform uses libm, use fdlibm's pow() function. I don't know how much these functions depend on each other's return values, so this might not work.
2. Wrap libm pow() to convert return values from libm to fdlibm form.
3. Fix fdlibm to be in line with libm, and fix the callers.
4. Use fdlibm for all platforms. Not a good direction.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)

> You can't just substitute libm calls for fdlibm. So we have a couple of options
> here:
> 
> 1. When a platform uses libm, use fdlibm's pow() function. I don't know how
> much these functions depend on each other's return values, so this might not
> work.

This keeps some of fdlibm around, but I'd hope we could ditch it if all OSes' libm implementations match POSIX or another such standard in all cases.

> 2. Wrap libm pow() to convert return values from libm to fdlibm form.

This seems better.

> 3. Fix fdlibm to be in line with libm, and fix the callers.

That violates ECMA-262 -- you can't just substitute libm for fdlibm, as you wrote, and the reason is that ECMA actually prescribes the current fdlibm results.

> 4. Use fdlibm for all platforms. Not a good direction.

I thought we did that since forever.  Did Mac change to use libm recently?

/be

Comment 11

[Brendan Eich [:brendan]]

See bug 291003.  Also, here are all the bugs with fdlibm somewhere in their summaries (or elsewhere, such that bugzilla.mozilla.org returns this list when I type just "fdlibm" into the front-page omni-query input):

https://bugzilla.mozilla.org/buglist.cgi?query_format=specific&order=relevance+desc&bug_status=__open__&content=fdlibm

If Mac OS X can blaze a trail away from fdlibm, then other platforms will gratefully follow.

/be

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Perhaps related: a fair number of people seem to think that C99's pow(1, NaN) -> 1 behaviour is broken, and I certainly find it surprising.

http://lists.debian.org/debian-glibc/2003/03/msg00202.html is one example.  Feh on C99, I say!

But yes, clearly we can't count on the system library to implement the (sane!) ECMA rule here, so we should always use that part of fdlibm on our system, or wrap the call with an explicit NaN test perhaps.

Comment 13

[Josh Aas]

Attachment: proof-of-concent for wrapping libm calls

This is a proof-of-concept patch for wrapping calls to libm for ECMA compliance. This patch fixes this bug for Mac OS X, but probably won't even compile on Windows, so it can't actually go in. However, if we think that this is a good direction to go in (naming scheme, function placement), then we can fix this up for Windows and Linux and be on our way.

Comments?

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'd use the name ecma_pow there.  Otherwise, that sort of wrapping seems a fine plan.  We can play with profilers or whatever to figure out in what order to do various tests (do we check for NaN arguments first and early/custom out, or...?)

(The ecma_pow routine itself doesn't really need to use strtod there, and I think we generally don't want to use == to compare floating point quantities, though with the special case of 1.0 might be acceptable.  But that's another pass, as you said.)

Comment 15

[Josh Aas]

Attachment: wrap libm |pow| for ecma compliance, v1.0

This patch takes shaver's comments into account and simplifies some code. It probably doesn't work on Windows for minor reasons, and I don't have a Windows box to work on. I made one attempt at Windows compat, since I happen to know that they use |_isnan| instead of |isnan|. It probably works on Linux.

If someone could please update this patch so that it compiles on Windows that would be great. Only Mac OS X will be using the function, but it should compile on all platforms before checkin.

Comment 16

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 206422 [details] [diff] [review]
wrap libm |pow| for ecma compliance, v1.0

We'll probably need a

#define NAN (nan(""))

for Linux, and that should work on Mac as well.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 206422 [details] [diff] [review]
wrap libm |pow| for ecma compliance, v1.0

How are the atan2 signed-zero quadrant-walking cases specified by ECMA-262 handled by Mac OS X's native libm?

Math.atan2( 0, 0) => 0
Math.atan2(-0, 0) => -0 (prove via 1/Math.atan2(-0,0), since (-0).toString()=="0")
Math.atan2(-0,-0) => -Math.PI
Math.atan2( 0,-0) => Math.PI

>+// Because the behavior or pow() differs from C99 to ECMA,
>+// we need to wrap the libm call.

No C++ comments in SpiderMonkey, it's C source and widely embedded.

Nit: "differs between C99 and ECMA" is better, it doesn't imply the wrong chronology.

>+static double
>+ecma_pow(double x, double y)
>+{
>+  if (isnan(y) || (fabs(x) == 1.0 && isinf(y)))
>+    return NAN;

Is this quiet NaN a well-specified bit pattern?  Just curious.

Thanks for working on this, it's a big help.

/be

Comment 18

[Josh Aas]

There does appear to be a well-defined bit pattern. The nan() function returns a quite NaN (QNaN) which conforms to ISO/IEC 9899:1999(E). From the IEEE:

The value NaN (Not a Number) is used to represent a value that does not represent a real number. NaN's are represented by a bit pattern with an exponent of all 1s and a non-zero fraction. There are two categories of NaN: QNaN (Quiet NaN) and SNaN (Signalling NaN).

A QNaN is a NaN with the most significant fraction bit set. QNaN's propagate freely through most arithmetic operations. These values pop out of an operation when the result is not mathematically defined.

An SNaN is a NaN with the most significant fraction bit clear. It is used to signal an exception when used in operations. SNaN's can be handy to assign to uninitialized variables to trap premature usage.

Semantically, QNaN's denote indeterminate operations, while SNaN's denote invalid operations.

Comment 19

[Brendan Eich [:brendan]]

Sure, ECMA-262 specifies only queit NaN usage/generation.  I was just curious if the obvious bit-pattern (there are 2^33 - 1 NaNs; we use as the default quient Nan the one with all 1s exponent and mantissa, 0 sign bit) was specified.

One more comment on the patch:

Instead of NAN (nan("NaN") or whatever), the patch should just use the NaN static jsdouble that's already initialized in jsnum.c by js_InitRuntimeNumberState.

/be

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> Sure, ECMA-262 specifies only queit NaN usage/generation.  I was just curious
> if the obvious bit-pattern (there are 2^33 - 1 NaNs; we use as the default
> quiet NaN the one with all 1s exponent and mantissa, 0 sign bit) was [fully]
> specified.

I meant "[fully] specified C99 or whatever is in charge of libm."

/be

Comment 21

[Josh Aas]

Using libm on Mac OS X, "javascript: 1/Math.atan2(-0,0)" generates "-Infinity". I think we're good with atan2.

Comment 22

[Josh Aas]

Attachment: 206422: wrap libm |pow| for ecma compliance, v1.1

address comments, add more Windows support

The only thing that worries me a bit is the integrity of this statement:
(fabs(x) == 1.0 && isinf(y))

I couldn't find a good way to use the static jsdouble NaN. I tried to use extern to access it, but that wouldn't link for some reason. Should I just move it into the header? Is there a better way?

Comment 23

[Josh Aas]

Attachment: fix v1.2

This is just an arguably cleaner way of organizing this change. Only organization has changed from v1.1.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 206446 [details] [diff] [review]
fix v1.2

This is not a complete alterna-patch, right?  What defines JS_USE_FDLIBM_MATH?

>+#define NAN (nan(""))
>+
>+#ifdef XP_WIN
>+#define isnan _isnan
>+/* how to create NaN on Windows, from MSDN */
>+static double
>+nan(const char *tagp)
>+{
>+  unsigned long nan[2] = {0xffffffff, 0x7fffffff};
>+  return *(double*)nan;
>+}
>+#endif
>+

There's no need for this given what follows.

> static JSConstDoubleSpec math_constants[] = {
>     {M_E,       "E",            0, {0,0,0}},
>     {M_LOG2E,   "LOG2E",        0, {0,0,0}},
>@@ -283,6 +296,14 @@
>         return JS_FALSE;
>     if (!js_ValueToNumber(cx, argv[1], &y))
>         return JS_FALSE;
>+#if !JS_USE_FDLIBM_MATH
>+   /*
>+    * Because C99 and ECMA specify different behavior for pow(),
>+    * we need to wrap the libm call to make it ECMA compliant.
>+    */
>+    if (isnan(y) || (fabs(x) == 1.0 && isinf(y)))
>+      return js_NewNumberValue(cx, NAN, rval);

Nit: no indentation by 2 spaces, or tab, or whatever happened here on the return line.

There's no need for NAN here now that you have cx, since you can use DOUBLE_TO_JSVAL(cx->runtime->jsNaN).

Also, you could use JSDOUBLE_IS_NaN(y) to avoid calling isnan (if it's not inlined/intrinsic), and I'd test (x == 1 || x == -1) instead of fabs(x) == 1.0.  Finally, note how the condition is testing (is-NaN or (is-Infinite and something)), which could be restated as (is-not-finite and (is-NaN or something)) where NaN is not finite by definition.

In SpiderMonkey terms:

    if (!JSDOUBLE_IS_FINITE(y) && (JSDOUBLE_IS_NaN(y) || x == 1 || x == -1))
        return DOUBLE_TO_JSVAL(cx->runtime->jsNaN);

But this makes me want to ask: does libm really return something other than NaN for 2^NaN?

/be

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

> But this makes me want to ask: does libm really return something other than NaN
> for 2^NaN?

Not on FC1:

[shaver@bitchcake tmp]$ cat nan.c
#include <math.h>
int main(void)
{ printf("%g\n", pow(2.0, nan(""))); return 1; }
[shaver@bitchcake tmp]$ ./nan
nan

or Tiger:

[shaver@Lickable shaver]$ ./nan
nan

Comment 26

[Josh Aas]

DOUBLE_TO_JSVAL(cx->runtime->jsNaN)| seems to generate undefined, not a jsval that is NaN. I can't just return that. When I returned |js_NewNumberValue(cx, NAN, rval)| (as in patch v1.2), it returned the correct NaN jsval.

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

> In SpiderMonkey terms:
> 
>     if (!JSDOUBLE_IS_FINITE(y) && (JSDOUBLE_IS_NaN(y) || x == 1 || x == -1))
>         return DOUBLE_TO_JSVAL(cx->runtime->jsNaN);

I think brendan means:

     if (!JSDOUBLE_IS_FINITE(y) && (JSDOUBLE_IS_NaN(y) || x == 1 || x == -1)) {
         *rval = DOUBLE_TO_JSVAL(cx->runtime->jsNaN);
         return JS_TRUE;
     }

Comment 28

[Josh Aas]

doh - that's what I get for trying to fix one more thing before bed

Comment 29

[Brendan Eich [:brendan]]

Yeah, I was sketching ;-).  Too bad C doesn't have nominal type equivalence, and a jsval is a JSBool....

Josh, new patch with that fix should be ready for review, unless there are other cases known where ECMA and libm standards differ.

/be

Comment 30

[Josh Aas]

Attachment: fix v1.3

comments taken into account

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 206531 [details] [diff] [review]
fix v1.3

sr=me to get things moving.

/be

Comment 32

[Mark Mentovai]

Comment on attachment 206531 [details] [diff] [review]
fix v1.3

All tests passed.  This works and looks good to me.

Comment 33

[Josh Aas]

Landed on trunk. I think we should take this for the branch because it is pretty safe and only affects Mac OS X right now (Mac OS X is the only tier-1 platform that uses libm instead of fdlibm).

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 206531 [details] [diff] [review]
fix v1.3

a=dveditz for drivers

Comment 35

[Marcia Knous [:marcia]]

Josh: can you please resolve this bug as fixed now that it has landed on the trunk?

Comment 36

[Josh Aas]

I have left some of my important bugs as unfixed since it has been difficult to keep track of what has made it to branches. Will close this bug when I have landed patches on branches.

Comment 37

[Josh Aas]

Fixed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.

Comment 38

[Bob Clary [:bc] (inactive)]

v 1.8.0.1, 1.8, trunk with 2005-01-11 builds.

Comment 39

[Bob Clary [:bc] (inactive)]

note this also occurs in firefox 1.0.7/1.0.8 on mac. let me know if we care.