184086 - number conversion must be LOCALE independent and not use atof (Double::toDouble)

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[caspar-mozilla]

User-Agent:       Mozilla/4.76 [en] (X11; U; Linux 2.2.18 i586)
Build Identifier: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.2.1) Gecko/20021130

Test and calculation of floats doesn't work correctly, they are handled as
integers, e.g. does 2.9 + 0.9 result in 2, which should be 3.8.

On the other hand, 7 div 4 yields 1.75, which is correct.

The problem exists not only for calculations but for relations within tests as
well, where indeed I saw this at first.

Reproducible: Always

Steps to Reproduce:
1. get the attachments from this report
2. load with mozilla
3. watch result
4. translate with sabcmd
5. watch result
Actual Results:  
in mozilla:
  2.9 is bigger than 1
  2.9 + 0.9 = 2
  7 div 4 = 1.75

with sabcmd:
  2.9 is bigger than 2
  2.9 is bigger than 1
  2.9 + 0.9 = 3.8
  7 div 4 = 1.75

Expected Results:  
Mozilla should calculate and test floats correctly. It even doesn't handle them
the same way (+ vs. div).

Following the spec at w3.org all numbers are floats. xpath, section 3.5 (which
is referenced in xslt, section 4).

Comment 1

[caspar-mozilla]

Attachment: demo stylesheet, 32 lines

demo stylesheet ( 2 tests and 2 calculations)

Comment 2

[caspar-mozilla]

Attachment: data file, 5 lines

the according data file

Comment 3

[Peter Van der Beken [:peterv]]

2.9 is bigger than 2

2.9 is bigger than 1

2.9 + 0.9 = 3.8

7 div 4 = 1.75

is what I see for that testcase.

Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.3a) Gecko/20021206

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: xml file

Reattaching data file linked to attachment 108600 [details].

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

wfm on a tip-build on windows. caspar, what platform are you on? Which processor
do you use?

Comment 6

[caspar-mozilla]

I am using Debian Linux (woody), AMD K6 II 500 Processor.

I downloaded 1.2.1 yesterday from this site. Peters "build identifier" shows up
a different version (1.3a).

So it seems that I pointed my finger at something fixed by now? Not sure.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

resolving per reporters comments

Comment 8

[Axel Hecht]

Madhur, could you please verify this on linux and 1.2.1? I'd hate if we have a 
platform problem here. Note that there were no checkins in months that appear
to change number behaviour.

Thanx

Comment 9

[caspar-mozilla]

Ok, I was wrong.

1. I installed Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.3a) Gecko/20021207
this morning on a newly created user account and got the same failure.

2. My Celeron i686 machine gets the same failure as well.

Both machines run Debian 3.0 (woody) standard install. So I suppose it to be a
compiler problem.

- I am doing a compile run of 1.21 at the moment but it's not finished yet.

- I give you some data from my machines:

$ /lib/libc.so.6
GNU C Library stable release version 2.2.5, by Roland McGrath et al.
Copyright (C) 1992-2001, 2002 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.
Compiled by GNU CC version 2.95.4 20011002 (Debian prerelease).
Compiled on a Linux 2.4.18 system on 2002-09-18.
Available extensions:
    GNU libio by Per Bothner
    crypt add-on version 2.1 by Michael Glad and others
    linuxthreads-0.9 by Xavier Leroy
    BIND-8.2.3-T5B
    libthread_db work sponsored by Alpha Processor Inc
    NIS(YP)/NIS+ NSS modules 0.19 by Thorsten Kukuk
Report bugs using the `glibcbug' script to <[e-mail]>.
$ ldd /usr/local/mozilla/mozilla-bin
    libgkgfx.so => not found
    libjsj.so => not found
    libmozjs.so => not found
    libxpcom.so => not found
    libplds4.so => not found
    libplc4.so => not found
    libnspr4.so => not found
    libpthread.so.0 => /lib/libpthread.so.0 (0x40020000)
    libdl.so.2 => /lib/libdl.so.2 (0x40034000)
    libgtk-1.2.so.0 => /usr/lib/libgtk-1.2.so.0 (0x40038000)
    libgdk-1.2.so.0 => /usr/lib/libgdk-1.2.so.0 (0x4015d000)
    libgmodule-1.2.so.0 => /usr/lib/libgmodule-1.2.so.0 (0x40191000)
    libglib-1.2.so.0 => /usr/lib/libglib-1.2.so.0 (0x40194000)
    libXi.so.6 => /usr/X11R6/lib/libXi.so.6 (0x401b7000)
    libXext.so.6 => /usr/X11R6/lib/libXext.so.6 (0x401bf000)
    libX11.so.6 => /usr/X11R6/lib/libX11.so.6 (0x401cc000)
    libm.so.6 => /lib/libm.so.6 (0x402a6000)
    libstdc++-libc6.1-1.so.2 => /usr/lib/libstdc++-libc6.1-1.so.2 (0x402c8000)
    libc.so.6 => /lib/libc.so.6 (0x4030a000)
    /lib/ld-linux.so.2 => /lib/ld-linux.so.2 (0x40000000)

I am not sure if that is enough.

Comment 10

[Axel Hecht]

Attachment: atof test file

Could you verify on your platform that the node value is the string "2.9"?
If it is, could you compile the attached atof testcase and verify that the
double
is 2.888888...9?
We either have a problem getting that node value straight, or we have an issue
in atof. I have the same libc, though compiled with 3.2 and not 2.... and it
seems to work for me. (Tested on standalone only, but I see no reason why it 
should matter.

Comment 11

[caspar-mozilla]

The node is printed as 2.9.

But I can't verify the double to be 2.888...9.

The difference from 2.9 is 0.000000, the value itself prints as 2.900000, a test
on d < 2.89 results in false.

I'll attach my code extension to show how I tested. If you know another way to
verify I would be glad to hear from you.

Comment 12

[caspar-mozilla]

Attachment: extended testfile

Comment 13

[caspar-mozilla]

Comment on attachment 108674 [details]
extended testfile

$ ./a.out
0.000000 2.900000
$

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I strongly suspect that this is a "parse" problem rather then a "rounding"
problem since the error is so big; 2 instead of 3.8

It seems like we only get the value up until the '.', 2.9 > 2 returns false and
2.9+0.9 is reported as 2.

hmmm.. could it be locale that is biting us? Is atof locale-sensitive? caspar,
do you happen to use some locale that uses another character then '.' as decimal
separator?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ah yes, atof is locale dependant:

http://lux.rm-rdf.com/man/man2html.cgi?atof+3
http://lux.rm-rdf.com/man/man2html.cgi?strtod+3

Comment 16

[caspar-mozilla]

First I want to resume on my ppor verifying:

ddd gives 2.8999...9 instead of 2.8888...9.


Yes, I set my locale to de_DE which uses "," as decimal seperator.

But 7 div 4 should have yielded 1,75 and I would have noticed that. And sabcmd
does transformation correctly with . and returns an error when using ,.

You are mislead by the big difference between 2 and 3.8. The term "2.9 + 0.9" is
read as "2 + 0".

BTW: I installed mozilla 1.0, 1.1, 1.2, 1.2.1 and 1.3a as binaries and none of
them worked correctly on my machine. I compiled 1.2.1 on my machine - lot's of
memory neccessary for that ;-)) - and it gave wrong results. All installs and
compilation were made on new user accounts, each one especially created for one
test.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: fix module

this should take care of module. I'm not sure what to do for standalone, but
i'm not sure if we should bother to find a solution or just wait until we have
nspr in standalone too?

Comment 18

[Axel Hecht]

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/misc/prdtoa.c#1259 checks
for '.', just to verify that PR_strtod is not locale dependent.
I'd like to have a better fix, though. PR_strtod gives us most parsing that we 
do before, so we should really remove all that format verification.
I'm tempted to leave this until we include xpcom/nspr in standalone.
(And I'm tempted to do that soon after tree opens.)

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

yep, the locale is the problem. The reason that 7 div 4 gives 1.75 is that we
use a locale-independant double->string function. But our string->double
function is locale-dependant. XSLT says that both should be locale-independant
which is why sabcmd does what it does and why our double->string fuction is
locale-independant. But we've missed doing the same thing for string->double,
hence this bug.

Axel: we still need the verification since PR_strtod can parse a wider range of
numbers then the XPath spec allows (such as "+5", "1e10" and "\f0")

Comment 20

[Axel Hecht]

looking at the string code, we should:

- create a charsink, checking for errors and converting PRUnichar to char.
- feed the incoming string with copy_string to that sink
- if the charsink has no error, call PR_strtod, return NaN otherwise

We should do the conversion and the syntax check in one pass. That way we get 
around alot of expensive stuff in mozilla's conversion routines.
Plus, NS_Lossy Foo inserts a '.' for chars too big, so we had to use toUTF8,
which goes over the string twice. For no reason, as far as this thing is 
concerned.
I really like to do this after nspr/xpcom.

Comment 21

[Axel Hecht]

Attachment: use mozilla string code for the string to double conversion

this is the right fix for this, IMHO.
I use one pass to check syntax and convert to a nsCString buffer. Then I call
PR_strtod. But only on the real number part of the string, the leading and 
trailing whitespace is not added to the buffer. It converts only up to the
first
error, even if we would get fragmented strings, too.

namedtemplate12 is fixed on standalone by this patch, didn't try module.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

-    // "."==NaN, ".0"=="0."==0

This doesn't seem to be enforced any more. math104 tests this. "-." should be
converted to NaN as well.

Comment 23

[Axel Hecht]

math104 succeeds. Didn't try "-.", will do next year.
("." is handled in getDouble, if Length()==1 and get()=='.'.)

Comment 24

[Axel Hecht]

Attachment: use PR_strtod, added mSign to catch -.

Comment 25

[Axel Hecht]

Comment on attachment 110599 [details] [diff] [review]
use PR_strtod, added mSign to catch -.

asking for reviews, jag, could you sr this to make sure this is good string fu?

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 110599 [details] [diff] [review]
use PR_strtod, added mSign to catch -.

>@@ -41,13 +41,8 @@
> #ifdef WIN32
> #include <float.h>
> #endif
>-//A trick to handle IEEE floating point exceptions on FreeBSD - E.D.
>-#ifdef __FreeBSD__
>-#include <ieeefp.h>
>-#endif
>-#ifndef TX_EXE

I'm not sure we dare remove this unless we know what it does and why it was
added.

>+private:
>+    nsCAutoString mBuffer;
>+    enum State {
>+        eWhitestart,
>+        eDecimal,
>+        eMantissa,
>+        eWhiteend,
>+        eIllegal};
>+    State mState;
>+    enum Sign {
>+        eNegative = -1,
>+        ePositive = 1};
>+    Sign mSign;
>+};

Please put the "};" on a separate line. You could also do:

enum State {
   ...
} mState;


With that, r=sicking

Comment 27

[jag (Peter Annema)]

Comment on attachment 110599 [details] [diff] [review]
use PR_strtod, added mSign to catch -.

>+    double
>+    getDouble()
>+    {
>+        if (mState == eIllegal || mBuffer.IsEmpty() ||
>+            (mBuffer.Length() == 1 && *mBuffer.get() == '.')) {

mBuffer[0] == '.'

sr=jag

Nice use of a sink, and this setup should allow you to add support for decimal
exponents later (e.g. -2.4E5).

Comment 28

[Axel Hecht]

fixed. thanx for the reviews

Comment 29

[Axel Hecht]

mass verifying