109286 - JS must set fpu control word on more platforms

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[tenthumbs]

On Windows the js engine sets the fpu control word, on Linux and other 
platforms it does not. Instead, it assumes that the initial state is 
what it desires. That's not a safe assumption. Any shared library in a 
process's address space can alter the fpu settings. It's quite easy to 
do. This should be fixed.

Now I know about only Linux x86 but I suspect that similar problems 
exist for any ELF platform that supports dynamic linking (and a 
configurable fpu, of course. Actually, I suspect that any platform that 
supports dynamic linking is a potential problem.

The big issue, of course, is that every platform is different so
figuring out what to do and how to do it is messy. One ray of hope is
that glibc appears to provide a /usr/include/fpu_control.h on each
platform it supports so those platforms with glibc may have a common
solution; or maybe not.

Clearly I can't provide a patch for all systems but, for Linux x86,
something like this works.

#include <fpu_control.h>
void set_extended_precision(void)
{
    fpu_control_t mode = _FPU_IEEE | _FPU_EXTENDED;
    _FPU_SETCW (mode);
}

with obvious changes for other modes.

Comment 1

[Brendan Eich [:brendan]]

tenthumbs, feel free to cc: me in the future.  Reassigning to khanson, cc'ing
rogerl, shaver, and me.

/be

Comment 2

[tenthumbs]

My example is too restrictive. One should probably do
  _FPU_GETCW(mode);
  mode &= stuff;
  mode |= otherstuff;
  _FPU_SETCW(mode);

It's probably nice to have the ability to turn math exceptions into 
errors.

There are also some higher-level C99 functions but they may not exist on 
every platform.

Comment 3

[Kenton Hanson (gone)]

targeting future.  I need to learn how to set default FPU state on Linux.

Comment 4

[Alban Browaeys]

i have the fpu precision problem on an athlon xp since end of january, using mozilla firefox, debian mozilla or firefox.
The lastest non installer build of mozilla works.

With the cpp code in this page i was able to check my fpu state:
http://www.stereopsis.com/FPU.html
(i have changed the Uint32 to unsigned int as under gcc int is 32 bit and does not use the SSE code using an athlon).
With it i set the fpu to default. and then assert its state in which case the FPU is on default.
If i just assert the FPU is not on default. It seems the settings is lost when the process exit.

The nspr code is also affected by this. Applications using it segfault too on my settup.

Cheers
Alban

PS: do you know what could change the FPU  settings , even booting in single user mode i have it on non default. Could it be the kernel ?

Comment 5

[Brendan Eich [:brendan]]

> Could it be the kernel ?

Of course.

What OS and version of that OS are you running?

/be

Comment 6

[Alban Browaeys]

Thanks i will check that KConfig did not build the kernel with improper optimisations for my athlon thunderbird (family 6 model 4).

If you need testing about those fpu precisions code, i have the broken system at hand ! 
debian libnspr4, mozilla and firefox segfault as does mozilla firefox . Only the latest non installer mozilla trunk build start (seems it was less optimized than other builds).

I had the problem in 2002 with debian too and mozilla taken from mozilla.org cvs.
By mistake i put the thunderbird instead of my athlon xp on this board , rebuild its kernel and the segfault are back at home.

It will take a few days to check the kernel side, my guesses are thunderbird is somewhat different march=athlon. Thanks you and biesi (from #mozilla channel) for your help in tracking the problem,  and also the debian gcc devels !

I am still wondering if some non root program can produce this fpu change. Before rebuilding my kernel, i used this cpu with the kernel configured for PIII and the precision was the default.
So it does not seems like the cpu itself is the culprit.

Comment 7

[Alban Browaeys]

adding me to the cc

Comment 8

[olecom]

More info:
<https://bugzilla.mozilla.org/show_bug.cgi?id=264912>

Based on <http://www.srware.com/linux_numerics.txt>
and tests on GNU+Linux/glibc and FreeBSD 5.3-RELEASE-p5.

Comment 9

[Vincent Lefevre]

Re comment #2:

> It's probably nice to have the ability to turn math exceptions into errors.

No, if you mean that they should be trapped, this would be incorrect (for
floating-point). According to the ECMAScript standard[*], NaN and infinities are
just special values (Section 8.5). Also, in Section 11.5.1: "If either operand
is NaN, the result is NaN." and so on.

[*] http://www.ecma-international.org/publications/standards/Ecma-262.htm

Comment 10

[olecom]

Attachment: common and (particular) linux x86, amd64 fpu setup

Protorype for common fpu setup i've put in jsmath.h, code for setup
is in jsapi.c (near to another platform depended windos dll setup).

In case some platform needs to setup fpu, it defines JS_NEED_FPU_SETUP
and implements "void setup_fpu(void)" as it should be, otherwise it will
be empty and optimized to nothing.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 185142 [details] [diff] [review]
common and (particular) linux x86, amd64 fpu setup

> # BeOS and HP-UX do not require the extra linking of "-lm"
>-ifeq (,$(filter BeOS HP-UX WINNT WINCE OpenVMS,$(OS_ARCH)))
>+ifeq (,$(filter BeOS HP-UX Linux OpenVMS WINNT WINCE,$(OS_ARCH)))
> LDFLAGS		+= -lm
> endif

Someone failed to update the comment.  Given how long this list is, perhaps it
is time to invert the test using filter-out of only the few OSes that want
fdlibm?  Which ones would be on that list?

>@@ -262,7 +262,10 @@ DASH_R		+= -n32
> endif
> endif
> ifeq ($(OS_ARCH),Linux)
>-LDFLAGS		+= -ldl
>+# fdlibm on Linux arch either, code has no :dlopen():

This comment is obscure, partly because of the colons around dlopen() -- and
the () is unnecessary.	The "either" makes it even less of a sentence.	What is
this comment trying to say?

>+LDFLAGS         := $(filter-out -lm -ldl, $(LDFLAGS))
>+NSPR_LIBS       := $(filter-out -lm -ldl, $(LDFLAGS))
>+OS_LIBS         := $(filter-out -lm -ldl, $(LDFLAGS))
> endif
> ifeq ($(OS_ARCH),OSF1)
> LDFLAGS		+= -lc_r
>+    setup_fpu();

If this is a static in the same file, it should be declared first.  It should
also be named SetupFPU or InitFPU to match local style.  It should *not* be
declared in jsmath.h (and the inline there does no good).

>-    if (!js_InitGC(rt, maxbytes))
>-        goto bad;
>+    do {
>+        if (!js_InitGC(rt, maxbytes))
>+            break;

Goto considered harmful aside, mixing a cleanup patch with the fix for this bug
is just going to slow me down.	At the least, attach a diff -w of jsapi.c to
show that nothing but the goto bad => break changes were done.

/be

Comment 12

[olecom]

(In reply to comment #11)
> (From update of attachment 185142 [details] [diff] [review] [edit])
> > # BeOS and HP-UX do not require the extra linking of "-lm"
> >-ifeq (,$(filter BeOS HP-UX WINNT WINCE OpenVMS,$(OS_ARCH)))
> >+ifeq (,$(filter BeOS HP-UX Linux OpenVMS WINNT WINCE,$(OS_ARCH)))
> > LDFLAGS		+= -lm
> > endif
> 
> Someone failed to update the comment.  Given how long this list is, perhaps it
> is time to invert the test using filter-out of only the few OSes that want
> fdlibm?  Which ones would be on that list?
> 
Must be generated from <jslibmath.h> or conversely.

> > ifeq ($(OS_ARCH),Linux)
> >-LDFLAGS		+= -ldl
> >+# fdlibm on Linux arch either, code has no :dlopen():
> 
> This comment is obscure, partly because of the colons around dlopen() -- and
> the () is unnecessary. The "either" makes it even less of a sentence.	What is
> this comment trying to say?
> 
this was addition for
'# BeOS and HP-UX do not require the extra linking of "-lm"'

> >+    setup_fpu();
> If this is a static in the same file, it should be declared first.  It should
> also be named SetupFPU or InitFPU to match local style.  It should *not* be
> declared in jsmath.h (and the inline there does no good).
> 
OK with bad style.
I couldn't find appropriate header file for declaration.
So, where to put that ? <jsapi> isn't place for that also.

> 
> Goto considered harmful aside, mixing a cleanup patch with the fix for this
> bug is just going to slow me down. At the least, attach a diff -w of jsapi.c
> to show that nothing but the goto bad => break changes were done.
> 
I know. It was my [stupid] attempt to start. It's not a bug itself, but now
i may ask you, Brendan, a permission to open new bug ID for things like this.
Btw, i read sometimes something like
<http://lxr.linux.no/source/Documentation/SubmittingPatches>.

> /be
> 

Thanks.

Comment 13

[olecom]

Attachment: Tiny setup, but with using GCC specials.

Without taining any headers and creating new, i decided
to use GCC's attributes. Hope it's OK.
My TODO in commets, maybe deleted. But making that, it
will be fix for this #109286. Patch fixes bug #264912.

Comment 14

[olecom]

(In reply to comment #13)
> Created an attachment (id=186102) [edit]
> Tiny setup, but with using GCC specials.
> 
There's summer or something wrong with patch again ?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 186102 [details] [diff] [review]
Tiny setup, but with using GCC specials.

I hope this is no longer necessary. If it is (sorry for the long-ignored review request), I'm gonna duck and let crowder catch it.

/be

Comment 16

[Brian Crowder]

Comment on attachment 186102 [details] [diff] [review]
Tiny setup, but with using GCC specials.

I don't think this is necessary any longer, if it is, please reopen.