Closed
Bug 414531
Opened 16 years ago
Closed 16 years ago
[Solaris]The return value of some of math method of javascript is not IEEE standard on solaris.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: leon.sha, Assigned: shajiayu)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
5.72 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
5.76 KB,
patch
|
brendan
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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.
Add -xlibmieee for sunstudio.
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.
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?
Comment 8•16 years ago
|
||
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?
Comment 9•16 years ago
|
||
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
Comment 10•16 years ago
|
||
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.
Reporter | ||
Comment 12•16 years ago
|
||
(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
Reporter | ||
Comment 13•16 years ago
|
||
Attachment #300311 -
Attachment is obsolete: true
Attachment #314822 -
Flags: review?(brendan)
Comment 14•16 years ago
|
||
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)
Reporter | ||
Comment 15•16 years ago
|
||
(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 16•16 years ago
|
||
Comment on attachment 314822 [details] [diff] [review] a targeted fix the build-config looks fine
Attachment #314822 -
Flags: review?(benjamin) → review+
Reporter | ||
Comment 17•16 years ago
|
||
Attachment #316182 -
Flags: review?(brendan)
Attachment #314822 -
Flags: review?(brendan)
Comment 18•16 years ago
|
||
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).
Assignee | ||
Comment 19•16 years ago
|
||
Attachment #316947 -
Flags: review?(brendan)
Attachment #316182 -
Attachment is obsolete: true
Attachment #316182 -
Flags: review?(brendan)
Comment 20•16 years ago
|
||
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 21•16 years ago
|
||
Comment on attachment 316947 [details] [diff] [review] patch v5 a1.9+=damons
Attachment #316947 -
Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Updated•16 years ago
|
Assignee: general → leon.sha
Comment 22•16 years ago
|
||
mozilla/configure.in 1.1991 mozilla/js/src/jsmath.c 3.39
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Comment 23•16 years ago
|
||
/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-
Comment 24•16 years ago
|
||
Leon, I don't have solaris available. Can you verify this bug is fixed?
You need to log in
before you can comment on or make changes to this bug.
Description
•