Closed Bug 53518 Opened 24 years ago Closed 23 years ago

Double maths not handling NaN +-Infinity correctly on some platforms

Categories

(Core :: XSLT, defect, P3)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: jjs, Assigned: sicking)

References

Details

(Keywords: testcase, Whiteboard: OS/2 0.9.2 branch)

Attachments

(1 file, 13 obsolete files)

11.55 KB, text/xml
Details
Well, Looks like the problem with the test cases to do with numbers comes down 
to the double maths handling.
On Linux alls well, but under Win2K (Dont have Win9x) the following maths fail.

if dbl = NaN;
(int)dbl == dbl  (This occurs in NumberResult::stringValue which causes a NaN 
to be printed as 0)

dbl == Double::POSITIVE_INFINITY
dbl == Double::NEGITIVE_INFINITY
dbl == Double::NaN

These three break the ieee rule stating that NaN is not even equal to itself, 
its the only case where x != x .

So, basically Double Maths is broken under Win2K..Ill keep looking for 
reasons/cause but Im thinking we need some help on this. It all looks fine to 
me, it just doesnt work :(
Blocks: 53335
No longer blocks: 53335
Sorry about the spam. Just getting the dependency the right way around.
Further checking shows that its just the handling of NaN that is stuffed. If a 
double is NaN it compares true to every thing when it should not to even its 
self..*cry*
Blocks: 53335
Sorry, Jus,
but this works as designed. The IEEE specs say that NaN compared to anything
is true. And every implementation should work that way.
There is a function isnan defined in math.h, at least here on the linux system.
Could someone check if this is standard C?
It should be, and that´s the spec´d way to test for NaN.

Axel
Status: UNCONFIRMED → NEW
Ever confirmed: true
Axel,
If this was the case then all would be well under windows but its not. 
Taken from the expresion handling of javascript1.1
<snip>
>If neither operand is object, function, or string-convertible, then both are 
>converted to number, and equality testing is performed in accordance with the 
>rules of the IEEE 754 standard:

>If either operand is NaN, the result of == is false but the result of != is 
>true. Indeed, the test x!=x is true if and only if the value of x is NaN.
<snip>

I agree that _isnan() should be used to check the doubles in functions where 
currently the == operator is relied apon to do the "correct" thing. If you have 
a windows 2000 build that does the correct thing with functions.xml as 
everything stands now Ill stop complaining and rebuild my box. But I have this 
exact same occurance under 2 different win2k pro and win2k advanced server 
configurations using VC6++. I dont have access win9x or NT4.

As a question why do we compare doubles with == to Double::POSITIVE_INFINITY 
and Double::NEGATIVE_INFINITY without checking for NaN when if the double is 
NaN we will get unexpected results in the operations?
Well regardless of the standard result for comparision operations on NaN's I 
have modified the problem areas in my tree to check for NaN correctly using 
Double::isNaN . Now all substring tests in functions.xml are being carried out 
correctly on Win2k. *Yay*.
Ill create a patch for the changes and test them on linux tomorrow.
I used to use the isnan function, but so many windows users were complaining 
about not having that functionality that I had to create my own function, 
Double::isNaN, and create my own representations of NaN, Positive Infinity and 
Negative Infinity. 

-Keith
Its not a problem. Im amused that linux and windows handling of NaN is 
different. However I have the patch to use isNaN and all will soon be cheery
Attached patch Patch for additonal NaNs (obsolete) — Splinter Review
accepting for now...I guess someone with a windows box should really accept 
this, but I can apply the patch and make sure it works on linux.

--Keith
Status: NEW → ASSIGNED
We have to do more than just fixing Windows. OS/2 has only very little support
for NaN or infinity, so we are in bad shape here.
I vote for doing the stuff like in js, pretty much copying the code from there.
That would be the div routine from
http://lxr.mozilla.org/seamonkey/source/js/src/jsinterp.c#2125
(see the MSVC comment there)
and the NaN, Infinity tests from
http://lxr.mozilla.org/seamonkey/source/js/src/jsnum.h#62

Looks like anything less would get us into platform troubles.
Opinions?

I'm adding mkaply for some input on that define, should this be XP_PC or 
XP_WIN?
OS: Windows 2000 → All
Summary: Double maths not handling NaN +-Infinity correctly on windows → Double maths not handling NaN +-Infinity correctly on some platforms
Sounds good to me. We could move all direct comparisons of the Infinity (etc) to
the Double object and handle the platform stuff from there, similar to the NaN
changes I made.
No longer blocks: 53335
*** Bug 53335 has been marked as a duplicate of this bug. ***
Maths seems to be the flavour of the night. So Is anyone working on this? 
Otherwise Im happy to pick up the torch and run with it.
Hi Jus,
I would like to postpone this math thing, until the functionlib issues have 
landed.
We should definitly make some plan here, as this is a rather broad change. We
can't rely on double to do the right math, so this needs to be changed all
over the place, to handle NaNs for platforms that don't really support it.
Hold your breath for a few days, please. Hopefully, we can do some triaging
next week or so.

Axel
After looking at the js files I couldnt agree more....
Pike asked me for some JS advice...

Math is hard.

Looks like the test primitives in the first part of jsnum.h are only
parameterized against IS_LITTLE_ENDIAN - different per-platform, obviously, but
not too hard to get right.  I expect that you should be able to get the #defines
you need from NSPR, or you can copy them from the JS standalone build system if
the NSPR ones aren't available.  (... what you get if you cd to js/src and gmake
-f Makefile.ref - if you weren't aready familiar with it).

Defining NaN (and possibly +- infinity) is tricky XP.  You probably want to
borrow code from jsnum.c:js_InitRuntimeNumberState.  This has been tested
over the years not to cause any unwanted processor floating point exceptions;
guaranteed to happen if you write it yourself.  IEEE math has a large number of
NaN values (all indistinguishable except by bitpattern), but I'd take the JS
approach and define a single one, using the union trickery from jsnum.c.

see jsinterp.c:RELATIONAL_OP and COMPARE_DOUBLES for a little bit about how to
handle arithmetic involving NaN xp.

Probably best to just rip off all the above code.

*if* you need IEEE-standard math library functions (cos, sin, atan, etc.) then
you probably want to consider linking against the fdlibm library in
js/src/fdlibm; it provides software implementations of these functions that are
guaranteed to have the right behavior for border cases (infinity, -0, NaN).  As
part of our build/link process, we use these functions whenever the
corresponding function is known to be wrong for a given platform.  Mostare. 
This is different than just numerics, though.

JS has been tested pretty carefully for IEEE performance.

Good luck!
Hmm, pray for me.
Assignee: kvisco → peterv
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla0.8
It would also be useful to have some function for checking if a negative or 
positive. The problem is detecting if a zero is a negative or positive zero. I 
have tried both |x<0| and |fabs(x)!=x| but both of theese are false for -0. I 
suppose

Double::isNeg(double x)
{
  return x<0 || x == DOUBLE::NEGATIVE_ZERO
}

should work if we somehow got DOUBLE::NEGATIVE_ZERO defined
Moving forward.
Target Milestone: mozilla0.8 → mozilla0.9
Target Milestone: mozilla0.9 → mozilla0.9.1
NaNs and other border cases won't keep us from shipping. The transformiix source
won't like linking against fdlibm, so pushing this to future.

Axel
Target Milestone: mozilla0.9.1 → Future
Patch coming up shortly... Basically I've copied the code from js, the 
constants to primitives.h and the macros to Double.cpp.

The code is rather straight forward except that the code for division seems a 
bit superfluous since it deals with almost all specialcases (1/0, 1/NaN, etc). 
The, very little, testing I've done on windows shows that at least windows does 
the right thing for division... But I bet that the js team has done more 
testing then I have :)

I didn't know how to get IS_LITTLE_ENDIAN for standalone, so I simply defined 
it always...
Another thing that I noticed is that on HPUX there is special code that does 
negation (in case the value is zero). But there is no similar code in 
multiplication, so the code assumes that on HPUX

 -1 * d  = -0
but
 -d      =  0

for d=0. Is this correct?
Keywords: review
Jonas: I believe HP computes negative zero multiply results correctly.  In JS,
we hacked workarounds based on specific testsuite test failures.  But I don't
think anyone has tested on HP systems recently -- cc'ing jdunn.

I think the Copyright notice in the license comments no longer applies, once
fragments of code lifted from the JS engine are dropped in, _mutatis mutandis_.
But IANAL.  Maybe it's "fair use"?  Cc'ing mitchell and hecker.

/be
I can try the patch on HP-UX, but is there a test for me
to try, so that I kow the patch works?

THanks
Brendan: uh? is the copyright issues with this?!? I though the js code was 
released under (M|N)PL (confirmed by a quick look), and transformiix certently 
is MPL.

Jim: I don't have any specific testcases right now, the best way to test this 
is using our testtool "buster". But we need to get another patch checked in 
before you can run that reliably. I'll let you know as soon as that's ready
Brendan, IANAL (and Mitchell DWTBAL :-) but if you take code out of an existing 
file licensed under the MPL (or NPL) and stick it into another file, then IMO 
that code would be considered still copyright by the original copyright holder, 
and still under the original license. Moreover, under the MPL/NPL the entire new 
source file into which the code had been dropped would have to be licensed under 
the MPL/NPL as well. (This follows from MPL/NPL Section 1.9(B), which defines a 
Modification as "Any new file that contains any part of the Original Code or 
previous Modifications", and MPL/NPL Section 1.3 which defines Covered Code as 
-- among other things -- "the Original Code or Modifications".)

So to summarize, the code snippets from the JS source files are "Original Code" 
licensed under the MPL or NPL (whatever happens to be the case), the new source 
files with JS code snippets added are "Modifications" (from 1.9(B)), and hence 
the new files are "Covered Code" (from 1.3). Therefore the new files must be 
licensed under the MPL or NPL as well (whatever the license was on the original 
file).

As a side note, if the original JS files were actually dual licensed (MPL + GPL 
or NPL + GPL) then IMO the new files should be as well, since in addition to 
being Modifications of the Original Code (as discussed above) they are derived 
works of GPLed code.

As for the copyright notice, IMO it should refer to Netscape (presuming Netscape 
was on the copyright notice for the JS code) and also to whomever is copyright 
holder for the new code (the code other than the JS-derived code).

As I said, IANAL and this is not legal advice, but this is what I personally 
would do were I personally involved in this.

P.S. And to clear up the issue about "fair use": That's a doctrine by which 
people claim to be able to use copyrighted material without having explicit 
permission from the copyright holder. But you already have explicit permission 
from the copyright holder to use the material -- that's what the license grants 
-- so IMO whether you have permission for "fair use" is irrelevant. Also, fair 
use doesn't change the fact that the material is copyrighted and that a specific 
person or entity owns the copyright; in other words, using copyrighted material 
under the guise of fair use doesn't make it "un-copyrighted".
If the NaN==0 is really a MSVC error, we shouldn't use XP_PC but XP_WIN

why not use DOUBLE_IS_... instead of Double::is..?

I'd prefer to have the lval and rval in the XP_WIN in RelationalExpr.cpp.

I don't like the define of IS_LITTLE_ENDIAN for standalone at all, alternatives,
please. We should at least hand out the chance to have that defined externally.

As to the use of the js code. That is GPL license, which we can't use. I see no
other way than to get the original auther to relicense that in MPL for us.
It's pretty simple code, and once looked at it, it might be hard to say we came
up with something that is not a modification :-(.
This might be easier, if we stick the code stolen from js in a separate header
file, so it could have a "original author" license plate. But we can't use the
current GPL one.

Axel

(this really doesn't block, so getting rid of that)
No longer blocks: 63906
Axel: yes, XP_WIN is right -- the JS code dates from 5 years ago, when XP_PC
meant XP_WIN, or was abused to mean the same thing.  I'll file a bug, thanks.

NSPR defines IS_BIG_ENDIAN or IS_LITTLE_ENDIAN appropriately, please use its
prtypes.h or (minimally) prcpucfg.h.

I don't understand your statement "that is GPL license, which we can't use." 
The JS license is a dual license, NPL or GPL, and the user of the code gets to
choose.  True, there could be a GPL-only fork (or an NPL-only, but netscape.com
won't do it).  That's not a real-world risk, the probability is too tiny.

My point is that you are not imposing the GPL on any transformiix consumers by
dual-licensing, but I am not urging you to dual-license just to get these fixes.
Hecker can provide better license advice.  And separate files should work ok, I
think.

/be
To add to what Brendan said: The stated goal of mozilla.org is to have the 
licensing on all Mozilla code distributed by mozilla.org be such that the code 
can be used within software otherwise licensed under the GPL, as well being 
usable within otherwise proprietary software (as is the case today). This is 
needed to support use of Mozilla by such projects as Galeon, etc.

The "dual license" scheme for the JavaScript code was put in place in support of 
this goal. Eventually a similar scheme, or some other scheme accomplishing the 
same purpose, will be put into place for all Mozilla code distributed by 
mozilla.org. So eventually the Transformiix code, and any other code which is 
intended for inclusion in the "official" Mozilla distribution, will have to be 
relicensed to take this into account.

In the short term the easiest thing to do IMO is probably to change the 
licensing on the files into which the JS-derived code is being included, and put 
them under the same dual-license scheme used for the original JS code. As 
Brendan says, this does _not_ mean that the code is now under the GPL (and hence 
cannot be used in non-GPL software); it only means that the code can be used and 
distributed as part of GPLed software.

If for some reason people don't want to use the dual license scheme, then IMO 
the only alternative is to find an alternative source for the code in question 
or to rewrite the code from scratch without using the JS code.

As brendan says the GLP part isn't blocking us, however I don't know if it's ok
to put NPL licenced code in an MPL licenced file?

Should all XP_PC be changed to XP_WIN? Someone (Pike?) mentioned that the
support in OS/2 for NaNs is pretty bad. How about BeOS? (I'm guessing that XP_PC
is defined on both OS/2 and BeOS, is that correct?)
I guess what it comes down to is do we know what OS's does the wrong thing and
which processors does the wrong thing (are there any differences between AMD,
Intel and Alpha?)

wrt DOUBLE_IS_ vs Double::is. The reason that I used Double::is is that IMHO we
shouldn't have two methods of checking for NaNd'ness and Inf'ness (both macro
and function) so I defined the macros local since using functions is more
transformiix style. (IMO I should even define the constants local..). What we
should do though is to define some of the Double functions as inline.
Of cource I would be totally ok with removing the Double functions and using
macros instead.
Aagain, IANAL, this is not legal advice, etc.: IMO if you take NPLed code and 
extract fragments they would remain under the NPL, ditto if you take MPLed code 
and extract fragments from it those fragments would remain under the MPL. 
Similarly, fragements of NPL/GPL code would remain under NPL/GPL, fragments of 
MPL/GPL code would remain under MPL/GPL. So you couldn't mix MPLed and NPLed 
code (or MPL/GPLed and NPL/GPLed code) in a single file unless a) you included 
separate license notices and identified the code to which they applied, or b) 
you got permission from the copyright holder(s) of one set of code to put it 
under the other license.

So in this case, where you have one set of code under the NPL/GPL (the JS stuff) 
and another under the MPL (MPL alone or MPL/GPL) I think you have the following 
options:

1. Put the code in separate files. Note that IMO you could actually use the 
preprocessor to #include one file in the other. This may be bending the 
spirit MPL and NPL a little, but I personally don't think it's out of bounds.

2. Put the JS code fragments into an existing file or files, and relicense those 
files under the NPL/GPL (whatever the license is on the JS code). This requires 
permission of copyright holders of the existing Transformiix files, but I 
presume that this permission can be obtained. Note that this does _not_ require 
relicensing of the other Transformiix files apart from the files into which the 
JS code will be inserted.

3. Get permission from Netscape (and whoever else holds copyright on the JS 
files) to relicense the JS code fragments under the MPL or MPL/GPL (whatever the 
existing Transformiix files are under).

4. Include the JS code in the existing files, and put two separate license 
notices in the files (e.g., as comments in the appropriate places).

I personally think (1) and (2) are probably the easiest options. (3) I suspect 
would be difficult, not so much because Netscape would be unwilling but more 
because IMO it might be difficult to get management's attention to look at this. 
(4) is doable but I think aesthetically unappetizing.
I'm willing to look into Frank's suggestion #3; asking Netscape to move from NPL
to MPL.  This could be a long-term project though, for the reason Frank
mentions.I don't like Frank's 4th suggestion at all.  The "file by file" license
distinction has been very clear to date and I would prefer not to see this
change.  Also, I'm not sure the MPL/NPL actually allow this.

mitchell
(snatching bug from peterv...)

as always, IANAL but:

Are we sure it's not *allowed* to copy NPL licenced code into an MPL licenced 
file? As far as I can see NPL is MPL plus two things:

1. Netscape may reintegrate thirdparty code without releaseing that sourcecode
   (IV)

2. Netscape may include NPL licenced code in other (non NPL/MPL) code and
   licence that under licences other then NPL/MPL (V.2 and V.3)

Nither of these should affect copying code from NPL to MPL since all that's 
allowed in MPL is also allowed in NPL so we're not loosening any restrictions.

However using this reasoning MPL code should be able to be released under GPL 
so I'm not really sure it holds...
Status: NEW → ASSIGNED
really snatching this time...
Assignee: peterv → sicking
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
As I think I mentioned earlier, I believe that incorporating NPLed code into a 
file otherwise governed by the MPL requires special handling: If the included 
code is placed under the MPL, then that would change the license terms for 
that code, since Netscape would no longer be able to relicense modifications to 
the included code under other terms as provided for by the NPL. Similarly, if 
the file into which the code were included were placed under the NPL, then 
Netscape would acquire special rights with regard to the code originally under 
the MPL (i.e., the code already in the file), in effect changing the license 
terms for that code.

Hence I believe that the best way to handle this problem is to get permission 
from the relevant copyright holder(s) to relicense either the included code or 
the file into which the new code is being included. If you want to relicense the 
included code under the MPL, then you need Netscape's permission. If you want to 
relicense the existing file under the NPL, then you need permission of whoever 
created the code already in the file.

The usual disclaimer: I am not a lawyer, and this is not legal advice. It is my 
personal opinion regarding what I would do myself if I were in this situation.
Yep I see your point :(

Oh well (I won't give my oppinion on the current situation, but I'm not happy 
about it). So mitchell, would you mind asking the appropriate people at 
netscape if copying this code from the js library to transformiix would be ok?

Targetting mozilla0.9.1, lets hope we've gotten somewhere with this by then...
Target Milestone: Future → mozilla0.9.1
Blocks: 72141
Has any thought been given to turning fdlibm into something that is more usable 
by other parts of Mozilla? probably not a component because of the JS linkage, 
but maybe a shared lib similar to NSPR or JS?

It would sure go along way toward solving issues like this.
Making fdlibm directly useable by the rest of Mozilla would be great where we 
need correct IEEE math operations.  A lot of work has gone into finding and 
addressing the math deficiencies on the various platforms, and the code is well 
tested.  I don't know how much the system math routines are used outside of JS, 
but if those uses require correct IEEE semantics on the edge cases, it would 
certainly be less work to make fdlibm a "first class" library than to 
reimplement it.

It would be nice if we could make fdlibm a shared lib like NSPR while still 
keeping it possible to build JS standalone without the client build 
infrastructure.
The code in question came from the JS engine sources under js/src, not from
fdlibm (js/src/fdlibm).

/be
Yes, xslt does not use any advanced math, we only need to borrow +-*/ and mod 
from the JS engine
should I push this or do we still aim to get transformiix in on OS/2 for 0.9.1?
lets push this to 0.9.2.  
Target Milestone: mozilla0.9.1 → mozilla0.9.2
mitchell: any progress on this? I still need an OK from netscape to copy this 
code. This is currently blocking us from building on OS2 and makes NaN handling 
suck on Win platforms
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Ok, the patch is now up-to-date with tip. I've also moved all macros to 
Double.cpp so now all exceptionhandling is done through the Double class.

I've tested this with all possible strange NaN and Inf expressions (test-xsl 
coming up) so i'm pretty sure the code is correct. However you need the new 
lexer/parser for the testcase to work 100%. Also NumberResult::stringValue is 
broken wrt NaNs on windows, don't know about other platforms.
cleaned up some stuff peterv commented on, nothing codewise. Credited the js-
code in all modified files.

To clear out the licensing issue: I mailed mitchell and got an ok from Netscape 
to copy the NPLed code into the MPLed files. Thanks Mitchell!
enhance the comment to the reused code. State the file (jsnum.h?),
and something like
License change by courtesy of Netscape Communications Corporation.
I don't dig the macros TX_DOUBLE_IS_INFINITE and TX_DOUBLE_IS_NaN. I'd prefer
the code inside the methods.
Check if 
    MBool isInfinite();
    MBool isNaN();
are used anyway.

oh, and it doesn't work. The endianess isn't set. At least for module, we should
include jsautocfg.h. Or we need our own configure test, along the lines of 
mozilla/js/src/jscpucfg.c

Axel
nsBranch ok, limited to XSLT only. If you can get r & sr and land on the trunk
first, of course.
took the code from jscpucfg.c to whack a configure test for endianess. It's
ifdef TX_EXE, so the normal mozilla build isn't affected.
Chris, could you review that code?
I tested both on sparc solaris, and intel linux, so the actual tests do what
they should.

Axel
Actually, isn't there already an autoconf macro that does this? AC_C_BIGENDIAN,
I believe.  Or is there something specific about the js test that you need?

When using this patch, I get a
configure.in:3923: warning: AC_TRY_RUN called without default to allow cross
compiling
while running autoconf. This is rather documented in the info to autoconf, and
in my autoconf, it's in acspecific.m4:1874.
}], ac_cv_c_bigendian=no, ac_cv_c_bigendian=yes)
should be 
}], ac_cv_c_bigendian=no, ac_cv_c_bigendian=yes, ac_cv_c_bigendian=unknown)
;-).

Maybe add a 
dnl This triggers a cross-compile warning. Ignore it, it's a autoconf badness
?

I tested the patch on linux in the meantime, too, works like charm.

Axel
Whiteboard: have r=peterv, need sr=
(1) does copying this code create any licensing issues?  i.e., is it copied
between two files that already have different licenses?  If so, then you need to
make sure the license in the destination file meets the criteria of the license
in the source file.  (2) how have you tested this code?  Math is hard(tm), do
you have a collection of edge cases to test it on?
OK, I hear from sicking on IRC that there is a big test case, and this code
passes.  I'm no lawyer, so assuming the used-with-permission language is
acceptable to the actual lawyers we do have and they don't have any further
licensing constraints (and sicking tells me this is so), then sr=scc.
Keywords: review
Whiteboard: have r=peterv, need sr= → have r=peterv, sr=scc
Whiteboard: have r=peterv, sr=scc → have r=peterv,r=axel, sr=scc
r=me, checked for sicking.
Leaving open for branch. vtrunk'ing.

Thanx to all at mozilla.org and Netscape for their effort to get this in.

Axel
Keywords: vtrunk
OS/2 passes this testcase with flying colors now.
hihi, IE fails every single one :) (though I havn't updated to MSXML3)
pushing
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Closing this one down.

Mike, I made up a whiteboard text for you to track stuff you want on your branch

Axel
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Keywords: nsBranch, vtrunk
Resolution: --- → FIXED
Whiteboard: have r=peterv,r=axel, sr=scc → OS/2 0.9.2 branch
This works on hpux (someone asked me to try it out!)
bitching buttons, verfication spam
Status: RESOLVED → VERIFIED
Attached file Updated testcase
This is the same stylesheet as in attachment 40482 [details] [diff] [review] but updated to conform to
the xslt-spec now that transformiix is more strict.
Attachment #15308 - Attachment is obsolete: true
Attachment #29797 - Attachment is obsolete: true
Attachment #29799 - Attachment is obsolete: true
Attachment #40480 - Attachment is obsolete: true
Attachment #40482 - Attachment is obsolete: true
Attachment #40493 - Attachment is obsolete: true
Attachment #40512 - Attachment is obsolete: true
Attachment #40616 - Attachment is obsolete: true
Attachment #41056 - Attachment is obsolete: true
Attachment #41402 - Attachment is obsolete: true
Attachment #41680 - Attachment is obsolete: true
Attachment #41833 - Attachment is obsolete: true
Attachment #42576 - Attachment is obsolete: true
Flags: in-testsuite?
Keywords: testcase
Alias: nan
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: