Closed Bug 414531 Opened 12 years ago Closed 12 years ago

[Solaris]The return value of some of math method of javascript is not IEEE standard on solaris.

Categories

(Core :: JavaScript Engine, defect)

All
SunOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: leon.sha, Assigned: shajiayu)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

For example on solaris Math.acos(-3.0) return 0. While in javascript Reference, it should return NAN. This works OK on mozilla 1.7 for solaris.
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Math:acos
can you please find out when this regressed?
Flags: blocking1.9?
Keywords: regression
It is a regression of bug 326842.
There are two solutions.
1. Enable fdlibm again on solaris.
2. Use compiler option "-xlibmieee" of sunstudio on solaris.
But I don't find a similar compiler option for gcc.
Attached patch patch (obsolete) — Splinter Review
Add -xlibmieee for sunstudio.
Assignee: general → leon.sha
Status: NEW → ASSIGNED
Attachment #300311 - Flags: review?
for reference, this *is* a problem on solaris i86pc w/ sunstudio.

SunOS5.11_i86pc_DBG.OBJ% LD_LIBRARY_PATH=$LD_LIBRARY_PATH:. ./js
js> Math.acos(-3.0)
0

dbg-firefox-i686-pc-mingw32\dist\bin>xpcshell.exe
js> Math.acos(-3.0)
NaN

And your fix doesn't fix Makefile.ref which is not appreciated.

mozilla/js/src% gmake -s -f Makefile.ref JS_READLINE=1 CC="/opt/SUNWspro/bin/cc -xlibmieee -I/zfsroot/usr/include" AS=/opt/SUNWspro/prod/bin/as AR=/usr/ccs/bin/ar LDFLAGS="-L/usr/local/lib -lm"

is what I used to test, and with it, things do work:

SunOS5.11_i86pc_DBG.OBJ% LD_LIBRARY_PATH=$LD_LIBRARY_PATH:. ./js
js> Math.acos(-3.0)
NaN

(the -I/zfsroot/usr/include is because I don't have math.h installed on my / volume, and -L/usr/local/lib is where libreadline.so.5 is, although there's a symlink for it and libgcc_s.so.1 in SunOS5.11_i86pc_DBG.OBJ as /usr/local/lib is not in my library search path)

Note that gcc also suffers from this problem:
mozilla/js/src% gmake -s -f Makefile.ref JS_READLINE=1 CC="gcc -I/zfsroot/usr/include" AR=/usr/sfw/i386-pc-solaris2.11/bin/ar LDFLAGS="-L/usr/local/lib -lm"
/SunOS5.11_i86pc_DBG.OBJ% LD_LIBRARY_PATH=$LD_LIBRARY_PATH:. ./js
js> Math.acos(-3.0)
0

This isn't really sufficient, you should provide a patch which at least forces gcc to libfdm

for people who are curious, building w/ Makefile.ref on i86pc requires config/SunOS5.xx_i86pc.mk (just clone any of the others), and either zapping lock_SunOS.s or hacking config.mk to not treat i86pc as sparc.
Depends on: 326842
Hardware: PC → All
(In reply to comment #4)
> for reference, this *is* a problem on solaris i86pc w/ sunstudio.
> 
This is also a problem on solaris sparc.
> for people who are curious, building w/ Makefile.ref on i86pc requires
> config/SunOS5.xx_i86pc.mk (just clone any of the others), and either zapping
> lock_SunOS.s or hacking config.mk to not treat i86pc as sparc.
> 
Where is the other .mk files?
(In reply to comment #4)
> This isn't really sufficient, you should provide a patch which at least forces
> gcc to libfdm

I didn't find a easy way to do this. The only way I can see is to backout the whole patch of bug 326842 and only define JS_USE_FDLIBM_MATH to 1 when we build js on solaris with gcc.
Assignee: leon.sha → general
Status: ASSIGNED → NEW
Attachment #300311 - Flags: review?
I assumed that you were talking about at least sparc (it's the default when people talk about solaris) and was merely noting it also affects intel.

cvs co mozilla/js/src/config
http://mxr-test.landfill.bugzilla.org/js/source/js/src/config/

joshmoz@gmail.com: you didn't remove fdlibm entirely right, could you help fix this such that gcc gets fdlibm?
Is this affects only Math.acos/asin and only with Solaris on Sparc? If so perhaps it would be better to apply a targeted fix for jsmath.c and force the functions to return NaN on this platform?
timeless: "gcc" does not need fdlibm. OSes with broken libm's may, but as Igor suggested a targeted fix is better at this point. We should try to cvs remove fdlibm.

/be
This seems more like a wanted1.9 than a blocking1.9 bug to me.  We should fix it, but we shouldn't hold up a release waiting on it.
Agreed
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
(In reply to comment #8)
> Is this affects only Math.acos/asin and only with Solaris on Sparc? If so
> perhaps it would be better to apply a targeted fix for jsmath.c and force the
> functions to return NaN on this platform?
> 
You can see the difference here.
http://docs.sun.com/source/806-3568/ncg_compliance.html
Attached patch a targeted fixSplinter Review
Attachment #300311 - Attachment is obsolete: true
Attachment #314822 - Flags: review?(brendan)
Comment on attachment 314822 [details] [diff] [review]
a targeted fix

>--- configure	7 Apr 2008 21:21:03 -0000	1.2011

Need Benjamin or a delegate to review this build module stuff.

>+#if !JS_USE_FDLIBM_MATH && defined(SOLARIS) && defined(__GNUC__)
>+    if (x < -1 || x > 1) {

Is __GNUC__ the right macro to test?

Prefer number line order to such tests:

    if (x < -1 || 1 < x) {

Ditto below.

>+    if(x == 0 && JSDOUBLE_IS_NEGZERO(y)) {
>+        z = fd_copysign(M_PI, x);
>+        return js_NewDoubleInRootedValue(cx, z, vp);
>+    } else if (x == 0 && y == 0) {
>+        z = x;
>+        return js_NewDoubleInRootedValue(cx, z, vp);
>+    }

Nit: if ( not if( in the first line.

Don't test x == 0 twice, do make an outer if (x == 0) { ... } around the inner y testing if-else-if chain.

/be
Attachment #314822 - Flags: review?(benjamin)
(In reply to comment #14)

> Is __GNUC__ the right macro to test?
> 
For sun studio compiler on solaris we use compiler option -xlibmiee to do so. Do you think !defined(__SUNPRO_C) && !defined(__SUNPRO_CC) is better? Since we only have too compilers on solaris, I think test __GNUC__ is enough.
Comment on attachment 314822 [details] [diff] [review]
a targeted fix

the build-config looks fine
Attachment #314822 - Flags: review?(benjamin) → review+
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #316182 - Flags: review?(brendan)
Attachment #314822 - Flags: review?(brendan)
Comment on attachment 316182 [details] [diff] [review]
patch v4

+            return js_NewDoubleInRootedValue(cx, z, vp);
+        } else if (y == 0) {

"else after return".

remove |else| (and replace with a line break).
Attached patch patch v5Splinter Review
Attachment #316947 - Flags: review?(brendan)
Attachment #316182 - Attachment is obsolete: true
Attachment #316182 - Flags: review?(brendan)
Comment on attachment 316947 [details] [diff] [review]
patch v5

.c file changes look good, thanks -- if configure changes are good with Benjamin, r=me.

/be
Attachment #316947 - Flags: review?(brendan) → review+
Attachment #316947 - Flags: approval1.9?
Comment on attachment 316947 [details] [diff] [review]
patch v5

a1.9+=damons
Attachment #316947 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Assignee: general → leon.sha
mozilla/configure.in 	1.1991
mozilla/js/src/jsmath.c 	3.39 
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
/cvsroot/mozilla/js/tests/ecma/Math/15.8.2.2.js,v  <--  15.8.2.2.js
new revision: 1.6; previous revision: 1.5
Flags: in-testsuite+
Flags: in-litmus-
Leon, I don't have solaris available. Can you verify this bug is fixed?
Verified on solairis nightly build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.