9967 - JSDOUBLE_IS_INT throws SIGFPE without integer range check

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[robinson]

Index: mozilla/js/src/jsnum.h
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsnum.h,v
retrieving revision 3.4
diff -u -r3.4 jsnum.h
--- jsnum.h     1998/10/14 10:22:11     3.4
+++ jsnum.h     1999/07/16 03:10:18
@@ -64,7 +64,7 @@
  * comparisons under MSVC.
  */
 #define JSDOUBLE_IS_INT(d, i) (JSDOUBLE_IS_FINITE(d) && !JSDOUBLE_IS_NEGZERO(d)
 \
-                              && ((d) == (i = (jsint)(d))))
+       && d <= 2147483647.0 && d >= -2147483648.0&& ((d) == (i = (jsint)(d))))

 /* Initialize the Number class, returning its prototype object. */
 extern JSObject *

Comment 1

[Mike McCabe]

This is not a new issue... here's a checkin note from more than a year ago:

revision 1.3
date: 1998/07/08 07:56:43;  author: mccabe;  state: Exp;  lines: +8 -8
Changed the definition of JSDOUBLE_IS_INT(d, i) to delay a (jsint)d
cast until after the double in question has been determined to be
finite, not NaN, etc.  This may make the code a little more XP for
platforms like BSD and Alpha Linux that don't like casting strange
values to int.  Thanks go to Uncle George <[e-mail]> and
hankin <[e-mail]> for their porting work.

(cc'ing them, in case they can cast light on the current situation)


It was associated with the following diff:

< #define JSDOUBLE_IS_INT_2(d, i)       (!JSDOUBLE_IS_NEGZERO(d) && (jsdouble)i
== d)
<
< #ifdef XP_PC
< /* XXX MSVC miscompiles NaN floating point comparisons for ==, !=, <, and <=
*/
< #define JSDOUBLE_IS_INT(d, i) (!JSDOUBLE_IS_NaN(d) && JSDOUBLE_IS_INT_2(d, i))
< #else
< #define JSDOUBLE_IS_INT(d, i) JSDOUBLE_IS_INT_2(d, i)
< #endif
---
> /*
>  * JSDOUBLE_IS_INT first checks that d is neither NaN nor infinite, to avoid
>  * raising SIGFPE on platforms such as Alpha Linux, then (only if the cast is
>  * safe) leaves i as (jsint)d.  This also avoid anomalous NaN floating point
>  * comparisons under MSVC.
>  */
> #define JSDOUBLE_IS_INT(d, i) (JSDOUBLE_IS_FINITE(d) &&
!JSDOUBLE_IS_NEGZERO(d) \
>                                && ((d) == (i = (jsint)(d))))



I think there was also some other changes surrounding this, mostly towards
adding compiler flags to ask the compiler to disable SIGFPE.

This operation is executed very frequently by the JS engine, so I'd prefer not
to take exactly the change you propose.  Is there something you can tweak
elsewhere in the build system?  Is an -mieee option missing?

Also cc'ing briano, brendan.

Comment 2

[brendan-obsolete]

Cc'ing the real IEEE f.p. experts.  I too don't want to see this change go in if
a platform-specific compiler misfeature that results in gratuitous SIGFPEs can
be disabled via a command line option.

At most, I'd say #ifdef an extra macro to check on the broken platform(s) and
expand to 1 on the non-broken ones, and && a call to that macro into
JSDOUBLE_IS_INT's body instead of foisting the cost on all platforms.

/be

Comment 3

[robinson]

About the performance concerns:

1. The JSDOUBLE_IS_FINITE check is a special case of the explicit range
   check, so it could be eliminated.
2. Any double which fails the explicit range check will also fail the
   conversion comparison, so this saves you some unknown number of
   unnecessary conversions (to int and then back to double).

I haven't counted the cycles, but I suspect it's close to being a wash,
performance-wise.  I guess the choice is between a clean, portable solution
on one hand or a .001% overall application performance improvement on the
other.

I don't really care, either way, as long as the final solution works on
FreeBSD.

Comment 4

[brendan-obsolete]

IS_FINITE is 32-bit load, AND with 32-bit constant, and compare.  That beats
64-bit-floating-point <= && <= on architectures I'm familiar with, because it
avoids yet the FPU and the extra branch.  But the main point isn't cycles, it's
why FreeBSD is special.

What compiler and options are you using?  Can you quote me the cc command line?
Thanks,

/be

Comment 5

[clayton]

On a few platforms, with excellent parallelism between the FPU and the integer
unit, the cost of floating-point comparison can be parallelized and relatively
cheap, but only if the branch unit is tightly integrated with the FPU (rare).
In general utilizing the integer unit and avoiding floating-point compare is the
more efficient choice.  So I agree that Brendan's on the right track: is there a
non-code workaround in FreeBSD that can be used to supress the signal?  (No
signal is the better thing to do, IEEE754 arithmetic specifies reporting the
error by setting the invalid flag in the FPU intending that it not disrupt, but
be visible to the executing program.)  Failing SIGFPE suppression, make a
compiler-specific code fix such as the macro suggestion, to retain efficiency on
most platforms.

Comment 6

[Mike McCabe]

Marking as assigned.

This issue came up before, and was resolved by adding a compiler flag, possibly
-mieee.  (I forget).  Did we lose that flag?

I'm accepting patches.

Comment 7

[Mike McCabe]

Any luck on this?

FWIW, I came across more info from the original (early last year) fix for this
problem on other platforms.

Adding -mieee is the fix.  Does this work for you?

Comment 8

[Mike McCabe]

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

Comment 9

[Pascal Hofstee]

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

Comment 10

[Brendan Eich [:brendan]]

Why is this bug still open?  Doesn't the gcc -mieee-fp option work on FreeBSD as
elsewhere?

/be

Comment 11

[Pascal Hofstee]

well .. on FreeBSD-4.0-CURRENT the -mieee-fp option seems not to cause gcc to
bail out so it might be that option is indeed normally supported. Should it be
my task however to keep manually telling the mozilla builds to use the
compiler-flag or is that something that mozilla's configure should setup to use
for me.

Comment 12

[Markus Holmberg]

Building with -mieee-fp on FreeBSD 3.4-STABLE + gcc 2.95.2 does not fix this.

Comment 13

[Pascal Hofstee]

check the gcc source-tree that is in my FreeBSD-4.0-CURRENT source-tree

and the #define TARGET_DEFAULT    already includes  MASK_IEEE_FP
So I assume FreeBSD's gcc compiles on default with the -mieee-fp flag enabled.

Comment 14

[Mike McCabe]

daeron -

Mozilla configure should probably add the needed flags.  If you can make any
needed changes, I'd be happy to check them in for you.

Mike

Comment 15

[Pascal Hofstee]

Like saska and i already tried to explain ... it looks like adding -mieee-fp
doesn't work on FreeBSD as the FreeBSD compilers Already seem to use that switch
internally anyway. And saska even tested a build which had the switch applied
... to no avail unfortunately.

I get the feeling adding -mieee-fp isn't gonna help a single bit in this case.
It looks like the problem will have to be fixed from within the mozilla
source-tree in this case.

As I mentioned in the 21914 bugreport I did notice a sequence occuring to the
js_NewNumberValue call before the SIGFPE got issued:

1st:  d=NaN(0xfffffffffffff)
2nd:  d=Inf
3rd:  d=-Inf
4th:  d=1.7976931348623157e+308  --==> SIGFPE

On several Different Tinderbox builds the 4th value is Always Exactly the same
on my system. Checked this over at least 4 different Tinderbox Builds.

Comment 16

[robinson]

Would it be possible to insert a call to fpsetmask(3) someplace?
That would almost certainly fix the problem, but only for FreeBSD.
Is there a proper place to put that kind of OS-specific code?

Comment 17

[robinson]

This (obviously bogus) patch fixes the problem for me.  Javascript on

FreeBSD needs a fpsetmask somewhere in the initialization process.  I

just don't know where, exactly, that should be.



Index: mozilla/js/src/jsapi.c

===================================================================

RCS file:

/cvsroot/mozilla/js/src/jsapi.c,v

retrieving revision 3.37

diff -u -r3.37

jsapi.c

--- jsapi.c     1999/12/22 00:03:47     3.37

+++ jsapi.c     2000/01/03

03:46:10

@@ -73,6 +73,8 @@

 #include "jsfile.h"

 #endif



+#include

<floatingpoint.h>

+

 #if defined(JS_PARANOID_REQUEST) && defined(JS_THREADSAFE)

 #define

CHECK_REQUEST(cx)      JS_ASSERT(cx->requestDepth)

 #else

@@ -633,6 +635,7 @@



  JS_INIT_CLIST(&rt->contextList);

     JS_INIT_CLIST(&rt->trapList);



JS_INIT_CLIST(&rt->watchPointList);

+       fpsetmask(0);

     return rt;



 bad:

Comment 18

[mreimer]

From Martin Cracauer <[e-mail]>, posted to [e-mail]:

Summary of following: The mozilla construction is a speed hack and it
would usually work when exceptions are disabled. However, it is slower
than the real thing would be and is hence useless and dangerous.

In <[e-mail]>, Markus Holmberg wrote:
> To my understanding, there isn't a flaw in the Mozilla code. What is
> happening is that a cast is made to test if a value inside a double
> actually is just an int! If it wasn't, no harm is done and the double
> will be continued to be treaten as a double.
>
> That's is how I've interpreted it. To see where the macro in question
> (that generates the SIGFPE's) is defined and used, check this link:
>
> http://lxr.mozilla.org/seamonkey/ident?i=JSDOUBLE_IS_INT
>
> Here's the definition:
>
> #define JSDOUBLE_IS_INT(d, i) (JSDOUBLE_IS_FINITE(d) && \
>       !JSDOUBLE_IS_NEGZERO(d) && ((d) == (i = (jsint)(d))))

Ah, OK, this macro is a conversion where the undefined result of the
(int)double_bigger_max_int is (intentionally) used as a speed hack of
a range check.

The only problems is that it isn't faster than correct code.

It is true that in practice it this macro will work as intended when
exceptions are disabled, because:

While the result of
  (int)double_bigger_max_int
is undefined, you will usually get an i that is filled with any
integer value. If d was > INT_MAX, the == with return nil for any
value of i. Hence the thing works if the programmer remembers only to
use i if the macro return true.

Beside the fact that I don't like this construction style-wise, I have
two problems with it:

1) In C, when an expression is undefined, *anything* may happen. You
   cannot depend on the fact that the code behaves as if i was filled
   with an integer at all. This is not ANSI C conformant. An
   conformant ANSI C compiler may make assumptions about this code
   that break it.

2) This speed hack is already weakend: If you look into the header
   file in Mozilla, you will find that the IS_FINITE and IS_NEGZERO
   was added to make MSVC++ happy, because it will run into
   exceptions otherwise.

The alternative to this hack is a normal range check. This will work
because behaviour is defined for comparisions on infinite and other
exceptional values.

You would get:
- ANSI C conformance.
- Make VC++ happy.
- Make the code ready to leave those FPU exceptions enabled that
  indicate real and serious problems.
- On my machine, it is actually faster than the current version.

Here's one version, implemented as an inline function. A complete
runnable example comparing the Mozilla construction (including
timekeeping) to this one is appended. On my machines, the conforming
version is 2-3 times faster than the Mozilla hack (and it doesn't
trigger exceptions).

static inline int cra_is_int(const double d, int *const i)
{
  if (d <= (double)INT_MAX && d >= (double)INT_MIN) {
    *i = (int)d;
    return 1;
  } else
    return 0;
}

I can only urge the Mozilla team to use clean constructions and leave
their hands off speed hacks that trigger undefined behaviour. The
project needs reliability first, and then speed. In this case, you
don't even get speed.

Martin

Comment 19

[Waldemar Horwat]

It's not as simple as you state.  The code you propose has three programming
bugs:

1.  It doesn't work for NaN under MSVC due to a bug in MSVC.
2.  It doesn't work for -0.0.
3.  It doesn't work for any non-integral number (such as 1.5) between INT_MIN and
INT_MAX.

The old C standard specified that a double-to-int conversion is undefined if the
source value can't fit in the destination value.  However, the new C9X standard
differs here:  if an implementation supports IEEE 754, then it should set the
invalid exception flag and return an unspecified result (paragraph F.4).
Trapping on flags is to be turned off by default.  (If an implementation doesn't
support IEEE 754, then the point is moot because the ECMAScript standard requires
IEEE 754.)


Note: The finiteness range check is there only to work around the MSVC NaN bug
and is not needed on correctly-behaving platforms such as the Mac.  On a
correctly implemented platform the following is sufficient:

#define JSDOUBLE_IS_INT(d, i) (((d) == (i = (jsint)(d))) &&
!JSDOUBLE_IS_NEGZERO(d))

    Waldemar

Comment 20

[Markus Holmberg]

But what's the point of the new standard setting the invalid exception flag if
flags are not checked and trapping is off by default?

The value of the conversion would still be unspecified (which to me is the same
as undefined..?) Which means the code is not safe in theory.

Comment 21

[Waldemar Horwat]

Returning an unspecified value is very different from undefined behavior.  A
program that exhibits undefined behavior may start playing NetHack.  On the other
hand, although the cast of an out-of-range double to a jsint may return an
unspecified value, that value still has to be a jsint.  Since the original double
was outside the range of jsint, the equality comparison must fail.

    Waldemar

Comment 22

[Markus Holmberg]

OK, point taken. Here's a patch to insert fpsetmask(0) in main1() for FreeBSD.
Better than nothing.

It works for me, but someone needs to check if it's been done the Right Way(tm).

Comment 23

[Markus Holmberg]

Attachment: Disable SIGFPE's on FreeBSD

Comment 24

[cbegle]

so unfair for me to hog all this mail.  rginda should get to feel the love as
well.

Comment 25

[leger]

Bulk add of "perf" to new keyword field.  This will replace the [PERF] we were
using in the Status Summary field.

Comment 26

[Mike McCabe]

Thanks to all for the debate and resolution, and to [e-mail] for the
patch.  I'm applying it and closing the bug.


Appending an interesting conversation that went by on email on 1-03-2000.  In
case this issue comes up again :)


From: "Ronald F. Guilmette" <[e-mail]> 01/03/00 19:48
Subject: Re: Should -mieee-fp equal fpsetmask(0) to avoid SIGFPE on FreeBSD?
To: [e-mail]

In message <[e-mail]>,
Markus Holmberg <[e-mail]> wrote:

>IEEE Std 754-1985 (Section 7, paragraph 1) is:
>
>        ``The default response to an exception shall be to proceed
>          without a trap.''
>
>(end quote)
>
>When checking with a simple C program and fpgetmask() I can confirm that
>a couple of bits are set by default, which mean they will trap some
>floating point operations. This is consistent with the older postings that
>say that default FreeBSD behaviour is *not* conforming with IEEE floating
>point standards.
>
>But when porting software from other platforms this becomes an issue. To
>my understanding, doing a fpsetmask(0) makes FreeBSD behave conforming
>to IEEE (i.e. not trapping the exceptions).
>
>Question 1: What is the reason for FreeBSD to differ from other platforms
>and not follow the IEEE standard by default?
>(Please forgive me if this is an ignorant question.)

No, its a Good Question, and I'd like to know the answer also.

>To avoid cluttering the code with fpsetmask(0)'s, using a compiler
>option to force IEEE conforming fp behaviour would be desirable. This is
>where the -mieee-fp parameter for gcc would be handy!

No.  This would be an entirely inappropriate use of that specific gcc
compiler flag.  And in fact, *no* special compiler flags should be
necessary in order for a compiled C program to conform to the specific
IEEE FP rule quoted above.

Please understand that gcc compiler flags, and specifically the -mieee-fp
option, are meant, in general, to control what type of code gcc emits.  That
is to say that the gcc compiler options (except those relating to linking,
specification of the output file, etc.)

For example, there are some machines (I think it is the x86 CPUs, and also
the Alphas) where gcc has a choice, and can either generate code for FP
operations that is fast or else code that is 100% IEEE conforming.  So for
those cases, gcc provides the -mieee-fp option... because gcc's default is
to just generate fast code, rather than fully IEEE conformant code.

But the problem you are talking about has nothing whatsoever to do what
what kind of code gcc generates.  It only has to do what what the ``pre-
main'' code that you programs get linked with (usually called crt0.o)
initialize the machine's IEEE trapping flags to.

If the existing crt0.o code... which is usually considered sort-of a part
of the C library... is failing to initialize all of the IEEE trapping bit
to `off', then this just means that someone needs to go in and fix the
crt0.o code so that it properly initializes all those bits to off... in
order to fully conform to the IEEE 754 FP standard, and specifically to
the rule quoted above.


>The direct reason for me asking about this is a FreeBSD-specific bug in
>Mozilla (that needs help from someone who knows this):
>
>http://bugzilla.mozilla.org/show_bug.cgi?id=9967

In that Mozilla bugreport, I see the comments:

]------- Additional Comments From [e-mail]  07/15/99 21:32 -------
]Cc'ing the real IEEE f.p. experts.  I too don't want to see this change go in
if
]a platform-specific compiler misfeature that results in gratuitous SIGFPEs can
]be disabled via a command line option.


My opinion is that brendan is correct, and that littering the Mozilla code
with a bunch of hacky kludges all over the place is *NOT* the Right Way to
solve such problems.  Not at all.

The Right Way to solve this problem is for the Mozilla guys to just slip
in the statement:

        fpsetmask(0);

right at the very beginning of their main() function.  This should be enough
to disable the unrequested FP traps on any and all platforms where you/they
might possibly get such traps.

As I say, the *ideal* way to solve this problem is for every person who
maintains a crt0.o file for any given flavor of UNIX to pay a bit more
attention to IEEE 754 and for them to arrange to have all FP traps dis-
abled already unpon entry into main().  However even if every maintainer
of a crt0.o file in the world reads these woards and goes forth and does
that TODAY, Mozilla should still be programmed so that it will avoid
crashing and burning even when it is compiled on some version of some
platform (e.g. FreeBSD) which is already out on the field today.

In short, program defensively.  You'll never be sorry.


-- rfg


P.S.  It's been a long time now, and I don't remember for sure any more...
too many brain cells cooked in my youth I guess... but I seem to recall
that I was the guy who talked Stallman into adding the -mieee-fp option.

I just mention it so that you will know that... once upon a time at least...
I actually *did* know what I was talking about when it came to IEEE FP
stuff.


From: [e-mail] (Charles M. Hannum) 01/03/00 20:19

"Ronald F. Guilmette" <[e-mail]> writes:

> >Question 1: What is the reason for FreeBSD to differ from other platforms
> >and not follow the IEEE standard by default?
> >(Please forgive me if this is an ignorant question.)
>
> No, its a Good Question, and I'd like to know the answer also.

That's actually a trick question.  When I researched this a few years
ago, I found that *no* system other than NetBSD starts up in an
IEEE-conformant mode.  To wit:

* iBCS2/SCO, SVR4 and FreeBSD leave exceptions unmasked.

* Linux uses the Intel default control word, which sets extended
  precision mode (which is not IEEE conformant).

> If the existing crt0.o code... which is usually considered sort-of a part
> of the C library... is failing to initialize all of the IEEE trapping bit
> to `off', then this just means that someone needs to go in and fix the
> crt0.o code so that it properly initializes all those bits to off...

Actually, the bits are *on* to mask the exceptions, and generally the
FPU is initialized on demand -- but we'll ignore the details.  ;-)

> >The direct reason for me asking about this is a FreeBSD-specific bug in
> >Mozilla (that needs help from someone who knows this):
> >
> >http://bugzilla.mozilla.org/show_bug.cgi?id=9967
>
> [...]
>
> My opinion is that brendan is correct, and that littering the Mozilla code
> with a bunch of hacky kludges all over the place is *NOT* the Right Way to
> solve such problems.  Not at all.
>
> The Right Way to solve this problem is for the Mozilla guys to just slip
> in the statement:
>
>       fpsetmask(0);
>
> right at the very beginning of their main() function.  This should be enough
> to disable the unrequested FP traps on any and all platforms where you/they
> might possibly get such traps.

Mozilla is far from the first application affected by this.  I've seen
this same hack in commercial code.



From: "Ronald F. Guilmette" <[e-mail]> 01/03/00 20:29

In message <[e-mail]>, you wrote:

>
>"Ronald F. Guilmette" <[e-mail]> writes:
>
>> >Question 1: What is the reason for FreeBSD to differ from other platforms
>> >and not follow the IEEE standard by default?
>> >(Please forgive me if this is an ignorant question.)
>>
>> No, its a Good Question, and I'd like to know the answer also.
>
>That's actually a trick question.  When I researched this a few years
>ago, I found that *no* system other than NetBSD starts up in an
>IEEE-conformant mode.  To wit:
>
>* iBCS2/SCO, SVR4 and FreeBSD leave exceptions unmasked.
>
>* Linux uses the Intel default control word, which sets extended
>  precision mode (which is not IEEE conformant).

Well, when *I* researched it a few years ago, I seem to recall that about
2/3rds of the systems I tested at that time did in fact properly disable
all of the FP traps prior to entry into main().

I think that the commercial/proprietary systems were generally pretty good
about doing this, e.g. Solaris, IRIX, HP/UX, DG/UX, DEC/OSF/1, etc.

But I think that you are correct that SCO was one of the ones that didn't
pass the test.

Part of the problem is that the major commercial C/C++ compiler test suites
make it a point to *only* test for ANSI/ISO conformance... IEEE 754 confor-
mance is outside of their scope as far as they are concerned.  The result is
that a lot of people never do any serious testing of their C/C++ compilers
for IEEE 754 conformance.



From: Tim Vanderhoek <[e-mail]> 01/03/00 23:00

On Mon, Jan 03, 2000 at 08:29:00PM -0800, Ronald F. Guilmette wrote:
>
> >> >Question 1: What is the reason for FreeBSD to differ from other platforms
> >> >and not follow the IEEE standard by default?
> >> >(Please forgive me if this is an ignorant question.)
[...]
> >That's actually a trick question.  When I researched this a few years
> >ago, I found that *no* system other than NetBSD starts up in an
> >IEEE-conformant mode.  To wit:
> >
> >* iBCS2/SCO, SVR4 and FreeBSD leave exceptions unmasked.

This should be a FAQ.

Over time, many people have expressed their preferrence that
exceptions be masked by default in FreeBSD.  They are not by
conscious decision.  The situation can be summed by the following
quote from bde: "I don't plan to change the exception handling
until the other [libm ANSI conformance bugs] are fixed."

The short answer is that porting is made easiest by simply calling
fpsetmask(0) on FreeBSD.

I couldn't find any really good references explaining all the exact
details, but...

References to two message by bde that are a little more explicit:

Message-ID:  <[e-mail]>
Message-ID:  <[e-mail]>

You can use the interface at http://www.FreeBSD.org/search to search
for messages by Message-ID.

Also, the following PRs are relevant:

URL: http://www.FreeBSD.org/cgi/query-pr.cgi?pr=105
URL: http://www.freebsd.org/cgi/query-pr.cgi?pr=229

There are many more references in the archives for the self-motivated.

Comment 27

[markush]

Just for the record: FreeBSD 4.0 now has floating point exceptions disabled by
default (as other platforms).

There was a (relatively) long discussion on this one of the FreeBSD mailing
lists when this issue was brought up, but there were no indications on a change.
But when reading the release notes for 4.0 I discovered this:

Excerpt from FreeBSD 4.0 Release Notes:

> Floating point exceptions for new processes (devide-by-zero,
> under/overflow, invalid range etc.) are now disabled by default. Use
> fpsetmask(3) to reenable those you need. Note that integer
> device-by-zero is not covered by the FPU and will still trap after
> this change. Also note that conversion of float/double to integer
> where the float variable is too big now doesn't trap as well (it can't
> be seperated from other operations we want masked).

The patch that's already in the (Mozilla) tree should be left intact for pre-4.0
users.