291003 - may want to use native libm instead of mozilla-provided fdlibm on Linux

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mosberger]

User-Agent:       Mozilla/5.0 (X11; U; Linux ia64; en-US; rv:1.7.6) Gecko/20050406 Firefox/1.0.2 (Debian package 1.0.2-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux ia64; en-US; rv:1.7.6) Gecko/20050406 Firefox/1.0.2 (Debian package 1.0.2-1)

I'm filing this bug report based on a suggestion by Bob Clary.

The fdlibm included as part of the javascript sources appear to be a repeated
source of trouble due to its endian-dependency (e.g., see 289326).  The GNU C
libm should be 100% compatible to fdlibm (since it's based on it) so it may make
more sense to use the native libm on Linux.  Not only would this avoid bloating
the mozilla-source code with #ifdef's, but it should also give access to much
better performing versions of transcendental etc. functions.

I'll attach a patch that makes this change for Linux.  I have built mozilla on
ia64 linux with that patch applied and tested in on maps.google.com and it
worked fine.  Is there a more formal test-suite for the javascript math functions?

If this approach looks reasonable, I could build for x86 and x86-64 as well.


Reproducible: Always

Comment 1

[David Mosberger]

Attachment: patch to use native libm on linux

Comment 2

[Bob Clary [:bc] (inactive)]

There is the spidermonkey test library. Read more at
<http://www.mozilla.org/js/tests/library.html>. 

1. Build the standalone js engine. 
   You will need to checkout js/src/config and maybe js/src/editline then build 
   in js/src

   make -f Makefile.ref # debug
   make -f Makefile.ref BUILD_OPT=1 # release

2. Checkout js/tests

3. Run the suite, e.g. for the release build

   perl jsDriver.pl -e smopt -L e4x lc2 lc3 spidermonkey-n.tests -s /pathtojs/js

   See perl jsDriver.pl --help for more info.

   for more information on running the test suite.

Comment 3

[David Mosberger]

Attachment: revised patch (fixes fd_pow() regression)

OK, running the test-suite uncovered a regression:

	  Failure messages were:
	  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

It turns out that pow() as specified by the JavaScript standard (Ecma 262)
behaves differently for two corner-cases than specified by the Single Unix Spec
v3 (which is what GNU libc adheres to).  This revised patch fixes the
regression by introducing a helper sus3_pow() function.  I went through the
specs and verified that the two corner-cases handled by sus3_pow() are the only
ones that are incompatible between Ecma 262 and SUS3.

Comment 4

[Bob Clary [:bc] (inactive)]

This certainly seems like something that should go in a developer preview.
Shaver? Brendan?

Comment 5

[Bastiaan Jacques]

Would be good to include module owner ;)

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 181226 [details] [diff] [review]
revised patch (fixes fd_pow() regression)

The patch looks fine, but it sounds from the comments like it hasn't been
tested on x86 or x86-64 (or PPC/ARM, I guess, which we also have people using
regularly).

Should we condition the use of this new path on IA64 until we get test results
for the other platforms?

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

I have this patch for testing in my seamonkey RPMs and didn't find any problems
with it up to now.
I wasn't sure if the patch works correctly because there is almost no difference
in the generated binaries. All fdlibm stuff is still there but it's also linked
against libm. So if we speak about testing on ix86, then it seems to work for me
since almost three weeks.

Comment 8

[Mike Shaver (:shaver emeritus)]

OK, cool.  We should figure out why we're still building and linking fdlibm,
though, before we check in.

Comment 9

[Wolfgang Rosenauer [:wolfiR]]

building and linking of fdlibm seems to be absolutely independent from
JS_USE_FDLIBM_MATH or any other settings.
The jslibmath.h just redefines all fdlibm functions to libm ones.
At first glance I see no reason why fdlibm should still be linked in.

Comment 10

[Mike Shaver (:shaver emeritus)]

Without any changes to Makefile.in (and Makefile.ref for the standalone
lib/shell), it will indeed still be built and linked.

We'll need to change the Makefiles if we want to avoid linking it in.  Can you
do that?

Comment 11

[Wolfgang Rosenauer [:wolfiR]]

hmm, I've played around with it but I found no nice solution up to now:
- we have to modify js/Makefile.in since there is defined that js/src/fdlibm
  should be built
- we have to modify js/src/Makefile.in to link libfdm conditionally

For both conditions we have the define in js/src/jslibmath.h which we can't use
in Makefiles. While having not so much experience in Makefile magic, I see that
we need 4 places where we need to define for the corresponding platforms.
- js/Makefile.in
- js/src/Makefile.in
- js/src/Makefile.ref
- js/src/jslibmath.h

What I could imagine is to add a js/fdlibm.mk (or alike), move all platform
checks there, include it in the other Makefile.ins and define with -D....

Other recommendations?

Comment 12

[Bob Clary [:bc] (inactive)]

*** Bug 303982 has been marked as a duplicate of this bug. ***

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #8)
> OK, cool.  We should figure out why we're still building and linking fdlibm,
> though, before we check in.

Besides (or before) the issues brought up in comment 3, I dimly recall bugs in
native libm implementations, including Windows' one, to do with NaNs and
possibly transcendentals (signed zero handling in atan2?  I forget).  The
testsuite should cover all these cases; does it?

/be

Comment 14

[Igor Bukanov]

*** Bug 322858 has been marked as a duplicate of this bug. ***

Comment 15

[Igor Bukanov]

Attachment: JS_USE_FDLIBM_MATH -> 0 with GLIBC >= 2.3

This a patch just to switch JS_USE_FDLIBM_MATH off when GLIBC >= 2.3. The patch adds explicit checks for GLIBC version instead of altering #elif defined(linux) as there are alternative C libraries for linux that are not tested.

As Math.pow issue was already addressed in bug 320770, the patch just touches jslibmath.h. I also think that it is better to address the issue of not compiling fdlibm when JS_USE_FDLIBM_MATH is 0 in a separated bug as on Linux this would be just build-time optimization. On my FC4 box the patch removes 7K from optimized build of js shell whether fdlibm is compiled or not.

Comment 16

[Mikhail T.]

In my opinion, the use of bundled fdlibm should be on only for systems, which prove to have inferiour -lm implementations.

Not just Linux and not just GLIBC ;-) The FreeBSD port, for example, patches out the build of fdlibm.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> In my opinion, the use of bundled fdlibm should be on only for systems, which
> prove to have inferiour -lm implementations.

Or just C99-compliant ones?  C99 specifies pow(1, NaN) == 1, not NaN as ECMA-262 requires.  See bug 320770. 

/be

Comment 18

[Mikhail T.]

=C99 specifies pow(1, NaN) == 1, not NaN as ECMA-262 requires.

If the discrepancy is in a finate number of cases, they should be enumerated as exceptions and the straightforward -lm used for everything else.

The less Mozilla-invented bicycles are built, the better :-)

Comment 19

[Igor Bukanov]

(In reply to comment #17)
> Or just C99-compliant ones?  C99 specifies pow(1, NaN) == 1, not NaN as
> ECMA-262 requires.  See bug 320770. 

The are 2 issues here:

1. Differences between C and ECMA script standards. So far this is only happens for pow()  (bug 320770), but even if there are more, I would expect that simple checks for NaN/Infinity would address that. Such checks are much smaller then fdlibm code and their additional overhead would be compensated by faster native implementation.

2. Buggy C libraries that are broken beyond repair. This is clearly require fdlibm but then a reasonable strategy would be to assume "innocent unless proved guilty". That is, JS_USE_FDLIBM_MATH should be set to 0 by default and only for specific cases of compiler/C-library it should be set to 1.

Comment 20

[Igor Bukanov]

Attachment: Making Linux default trusted case.

This version of the patch removes if defined(linux) checks all together under assumption that C libraries should work there. If specific version would not, then checks for that version should be added by interested parties in future.

Plus patch puts autodetection of JS_USE_FDLIBM_MATH inside "#ifndef JS_USE_FDLIBM_MATH" so it can be altered via make/config files.

Comment 21

[Brendan Eich [:brendan]]

Mikhail: If you read bug 320770 you'll see that I am all in favor of ditching fdlibm on as many platforms as possible.  As far as we know, that's all the top Unix-like OSes.  I'm not sure about Windows, but it just needs testing -- we can wrap any deviant functions and fix up results as Igor notes.

Igor: what OSes can you test on?

Bob Clary: do we have ECMA-262 Math tests including regressions hosted on some server that people can run in their browser builds?  Or at least easily run from js/tests?

/be

Comment 22

[Brendan Eich [:brendan]]

Forgot about comment 2, where Bob says how to build the js shell and run the testsuite.  Still hoping for a webified interface outside the firewall, but that may be hoping for too much.

/be

Comment 23

[Bob Clary [:bc] (inactive)]

(In reply to comment #22)

does <http://test.bclary.com/tests/mozilla.org/js/menu.html> do it for you?

Comment 24

[Igor Bukanov]

(In reply to comment #21)
> Igor: what OSes can you test on?

I can access only linux in the form of FC4, recent Debian and after some efforts with cross compiling a Linux distro on my wireless router with uclibc instead of glibc. 

Comment 25

[Mike Shaver (:shaver emeritus)]

Comment on attachment 208038 [details] [diff] [review]
Making Linux default trusted case.

I'm concerned that this will cause problems for people running our builds on older Linux machines, with buggy (pre-glibc 2.3?) C libraries.

We should test with the oldest glibc that we support with official Linux Firefox builds, at least.

Comment 26

[Mikhail T.]

> I'm concerned that this will cause problems for people running our builds on
> older Linux machines, with buggy (pre-glibc 2.3?) C libraries.

It will cause problems and that's how the #ifdef-s will be fine-tuned.

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> > I'm concerned that this will cause problems for people running our builds on
> > older Linux machines, with buggy (pre-glibc 2.3?) C libraries.
> 
> It will cause problems and that's how the #ifdef-s will be fine-tuned.
> 

Or older glibc can be patched with a fix as well - this is possible on Linux. This is exactly the reason I changed my mind about the initial GLIBC >= 2.3 only patch and suggest not to worry about buggy C libraries on this paltform and just wait for bug reports if any.

Comment 28

[Mike Shaver (:shaver emeritus)]

Comment on attachment 208038 [details] [diff] [review]
Making Linux default trusted case.

Bold, my friend, bold.  Let's go for it.

Comment 29

[Igor Bukanov]

I committed the last patch:

Checking in jslibmath.h;
/cvsroot/mozilla/js/src/jslibmath.h,v  <--  jslibmath.h
new revision: 3.18; previous revision: 3.17