Crash in debug build if I press submit button. Problem is with PR_dtoa function on ARM without FP unit.

NEW
Unassigned

Status

NSPR
NSPR
--
major
13 years ago
11 years ago

People

(Reporter: Tom Marn, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050110 Firefox/1.0 (Debian package 1.0+dfsg.1-2)
Build Identifier: CVS Source : Stable Mozilla 1.7.5 - Released December 17, 2004

Problem is with a PR_dtoa function in mozilla/nsprpub/pr/src/misc/prdtoa.c
Converting from double to string produce memory leak. Minimo crash will with
Assertion failure: lock != NULL, at ptsynch.c:206

 PR_dtoa is quite broken on ARM platform (PXA255-400Mhz version without FP
unit), Minimo (from mozilla source 1.7.5) is compiled with softfloat compiler
(gcc version 3.3.3/glibc 2.3.2, from www.kegel.org).

The reason is because ARM platform (little endian) use (mixed endian) when
manipulating with float numbers. Maybe the prdtoa.c functions are not compliant
with that arhitecture.

I also found the same problem in
http://lists.debian.org/debian-arm/2003/10/msg00028.html

Reproducible: Always

Steps to Reproduce:
1. build debug version of Minimo. (release version will skip assert checking and
"submit" will not be executed and not action forward to server :( ).
2. ./Minimo www.google.com
3. Enter something "test" into search textbox
4. Press "Google search"
5. Nothing happend after submit in release version.
   Crash in debug version: Assertion failure: lock != NULL, at ptsynch.c:206
Actual Results:  
Because pow5mult called from PR_dtoa is extremly slow you will got an error
after 1 minute:
Assertion failure: lock != NULL, at ptsynch.c:206



Expected Results:  
Function should convert double value to string, returns and than submit the
entered text forward (I guess).


Look into prdtoa.c: 
function static Bigint *mult(CONST Bigint *a, CONST Bigint *b) which is called
from pow5mult generates list with numbers of multiply of 5. 
Set breakpoint into line 678 of mult function and print return c variable ,
The table which generates here must have numbers ( 5 25 125 625 3125 15626 ... }

gdb: print *c

Next nubmer which is generated is 78125, which is OK  ( 5 * 15625)
OK $2 = {next = 0xffffffff, k = 1, maxwds = 2, sign = 0, wds = 1, x = {78125}}

Next nubmer is 390625, OK too. ( 5 * 78125)
$4 = {next = 0x0, k = 1, maxwds = 2, sign = 0, wds = 1, x = {390625}}

Than instead of 1953125 value of x is {2264035265}, IS THIS WRONG ????
$5 = {next = 0xffffffff, k = 1, maxwds = 2, sign = 0, wds = 2, x = {2264035265}}

gdb: bt

#0  mult (a=0x1f88c0, b=0x1f88c0) at prdtoa.c:678
#1  0x404933a4 in pow5mult (b=0x1f8900, k=77644143) at prdtoa.c:739
#2  0x40497294 in $a () at prdtoa.c:2341

Before the assertion happens in ptsynch.c:206 call stack is:

(gdb) bt
#0  PR_Unlock (lock=0x0) at ptsynch.c:206
#1  0x404926e4 in Balloc (k=16) at prdtoa.c:418
#2  0x40492ea0 in mult (a=0x424ab008, b=0x424ab008) at prdtoa.c:620
#3  0x404933a4 in pow5mult (b=0x424cc008, k=2369) at prdtoa.c:739
#4  0x40497294 in $a () at prdtoa.c:2341

Also, maybe something is wrong (Kmax) in Balloc function which generates
freelist[k] table with pointers on allocated memory where converted numbers will
be stored. (guess)

Comment 1

13 years ago
Tom, could you find out the CVS revision of
prdtoa.c?  You can find that out with the
following commands:

  cd mozilla/nsprpub/pr/src/misc
  cvs status prdtoa.c

In the current version of prdtoa.c, we have
the following code that tries to handle ARM:

#if defined(__arm) || defined(__arm__) || defined(__arm26__) \
    || defined(__arm32__)
#define IEEE_ARM
#elif defined(IS_LITTLE_ENDIAN)
#define IEEE_8087
#else
#define IEEE_MC68k
#endif

This code was added by Darin Fisher to fix bug 209814.
Does your prdtoa.c have this code?  Could you find
out if the above code causes IEEE_ARM to be defined
in your environment?
(Reporter)

Comment 2

13 years ago
(In reply to comment #1)
> Tom, could you find out the CVS revision of
> prdtoa.c?  You can find that out with the
> following commands:
> 
>   cd mozilla/nsprpub/pr/src/misc
>   cvs status prdtoa.c
> 
> In the current version of prdtoa.c, we have
> the following code that tries to handle ARM:
> 
> #if defined(__arm) || defined(__arm__) || defined(__arm26__) \
>     || defined(__arm32__)
> #define IEEE_ARM
> #elif defined(IS_LITTLE_ENDIAN)
> #define IEEE_8087
> #else
> #define IEEE_MC68k
> #endif
> 
> This code was added by Darin Fisher to fix bug 209814.
> Does your prdtoa.c have this code?  Could you find
> out if the above code causes IEEE_ARM to be defined
> in your environment?
> 

Hi, CVS Revision status of prdtoa.c that you ask for is:

File: prdtoa.c          Status: Up-to-date

   Working revision:    3.7.4.4
   Repository revision: 3.7.4.4 /cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v
   Sticky Tag:          MOZILLA_1_7_5_RELEASE (revision: 3.7.4.4)
   Sticky Date:         (none)
   Sticky Options:      (none)


#define's are there but nothing helps, i also try with the newest 1.8.X
(11.feb.2005), but problem still remains.

File: prdtoa.c          Status: Up-to-date

   Working revision:    3.7.4.7
   Repository revision: 3.7.4.7 /cvsroot/mozilla/nsprpub/pr/src/misc/prdtoa.c,v
   Sticky Tag:          NSPRPUB_PRE_4_2_CLIENT_BRANCH (branch: 3.7.4)
   Sticky Date:         (none)
   Sticky Options:      (none)

Just ask, I will check anything to solve this problem.

Comment 3

13 years ago
Tom: thanks for the cvs revision info.
Your file (rev. 3.7.4.4, MOZILLA_1_7_5_RELEASE)
has Darin's fix.

Could you confirm that prdtoa.c defines
IEEE_ARM?  (which means one of the following
macros: __arm, __arm__, __arm26__, __arm32__
is defined)

If so, this means Darin's fix does not address
your problem.  In that case I'll need to have
Doug Turner take on this bug.

Comment 4

13 years ago
Tom:

Specifically, please perform the following experiment:

Edit prdtoa.c and add the following line

  #error "IEEE_ARM is defined"

after the line

  #define IEEE_ARM

and then go to the nsprpub/pr/src/misc directory
in your build tree and do a "make".  See if the
compilation succeeds or fails.

Comment 5

13 years ago
alternatively, could you attach the file generated by:
make prdtoa.i (make from the directory that has prdtoa.o)
(Reporter)

Comment 6

13 years ago
(In reply to comment #4)
> Tom:
> 
> Specifically, please perform the following experiment:
> 
> Edit prdtoa.c and add the following line
> 
>   #error "IEEE_ARM is defined"
> 

Yes it fails after make, I confirming that IEEE_ARM define is use.

prdtoa.c:138:2: #error IEEE_ARM
make: *** [prdtoa.o] Error 1
(Reporter)

Comment 7

13 years ago
Created attachment 175284 [details]
gziped prdtoa.i file.
(Reporter)

Comment 8

13 years ago
Created attachment 175285 [details]
gziped prdtoa.i file.
(Reporter)

Comment 9

13 years ago
Created attachment 175286 [details]
source code to reproduce the same error.

This is the code (see attachment) for reproduce the same error. 
Just wait 30 seconds and you will get the same assertion as in the mozilla
minimo browser.
Assertion failure: lock != NULL, at ptsynch.c:206
Aborted

copy code into mozilla/nsprpub/pr/tests/testdtoa.c 
and change Makefile (see testdtoa.c for details).
(Reporter)

Comment 10

13 years ago
Created attachment 175935 [details] [diff] [review]
This is a patch for nspr detection workaround on ARM

I found the location where PR_dtoa crashes. It is in a whatnspr.c file 
set_whatnspr function. Instead of returning from PR_dtoa with -1 value function
dies. Patch skips fault version detection of nspr library and forces to setup
correct nspr version with r = 0. See attached patch.
tom
(Reporter)

Comment 11

13 years ago
Created attachment 175937 [details] [diff] [review]
Another patch to workaround a dtoa on ARM

Replacing dtoa with sprintf in nsStringObsolite.cpp:Modified_cnvtf(char *buf,
int bufsz, int prcsn, double fval) method. Note that this is just a workaround
until dtoa is not fixed.
tom

Comment 12

13 years ago
I am not seeing this problem on either Linux ARM or on WinCE ARM.  
(Reporter)

Comment 13

13 years ago
(In reply to comment #12)
> I am not seeing this problem on either Linux ARM or on WinCE ARM.  

The problem is with Linux ARM without FPU (tested on PXA255) and Minimo compiled
with softfloat compiler (for faster code) and with disabled kernel FastFPU
emulation of course.
I did workaround hacks, i can send you patches which work without crashing on my
platform.
The problem was with functions that call PR_dtoa.
(Reporter)

Comment 14

13 years ago
Created attachment 190515 [details] [diff] [review]
dtoa and jsdtoa workaround for ARM linux

PR_dtoa workaround (with debug printf's, just to see fixed locations.)

Updated

13 years ago
Assignee: dougt → nobody

Updated

13 years ago
Assignee: nobody → dougt
Component: General → NSPR
Product: Minimo → NSPR
Version: unspecified → 3.0

Updated

13 years ago
Attachment #190515 - Flags: review?(wtchang)

Comment 15

13 years ago
Comment on attachment 190515 [details] [diff] [review]
dtoa and jsdtoa workaround for ARM linux

Tom, thanks for the patch.

I will attach a better patch for
mozilla/security/nss/lib/base/whatnspr.c.
In short, the code that's causing problem for
ARM in that file is obsolete, so the better fix
is to simply delete that code.

Your changes for the other files in this patch
have two problems:
- You use the C++ comment delimiter //, which is
  not supported by all C compilers yet.
- You need to code these changes with ifdefs so
  that we continue to use the original code on
  other platforms.
Attachment #190515 - Flags: review?(wtchang) → review-

Comment 16

13 years ago
Tom, I decided to open bug 317052 to deal with
mozilla/security/nss/lib/base/whatnspr.c, so the
patch for that file will be in that bug report.
Depends on: 317052
(Reporter)

Comment 17

13 years ago
(In reply to comment #16)
> Tom, I decided to open bug 317052 to deal with
> mozilla/security/nss/lib/base/whatnspr.c, so the
> patch for that file will be in that bug report.
> 

Ok, I agree. I sent previous patches just for demonstration purpose where the bugs occured, and how to overcome the problems. I didn't pay attention on coding syntax. The patches were sent because of Doug Turner comment #12.

Comment 18

12 years ago
(In reply to comment #14)
> Created an attachment (id=190515) [edit]
> dtoa and jsdtoa workaround for ARM linux
> 
> PR_dtoa workaround (with debug printf's, just to see fixed locations.)

Tom, I have some question about the patch of hack4 in jsnum.c

+	double num = (jsdouble)d;
+	char temp[255];
+	printf("DOUBLE:%f\n",num);
+	sprintf(temp,"%f",(int)num);

num will be casted to integer, if num is not an integer, like 3.5, cast will create 0.0.
why not just use sprintf(temp,"%f",num) instead?

+	printf("CONVERTED:%s\n", temp);
+	numStr = temp;
eh? No, that would just truncate the value, yielding 3. but printing that as a %f is probably not such a good idea...

Comment 20

12 years ago
(In reply to comment #19)
> eh? No, that would just truncate the value, yielding 3. but printing that as a
> %f is probably not such a good idea...
> 

It's strange, when I use TestGtkEmbed to open http://maps.google.com , the message makes me confused:

== start of message ==

jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=3.500000)
DOUBLE:3.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=5.500000)
DOUBLE:5.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=5.500000)
DOUBLE:5.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=5.500000)
DOUBLE:5.500000
CONVERTED:0.000000
jsnum.c: Calling hack4 for JS_dtostr(buf, sizeof numBuf, DTOSTR_STANDARD, 0, d=0.000000)
DOUBLE:0.000000
CONVERTED:0.000000

== End of messages ==

This is the reason I remove the cast of num in hack 4. But even the convert look correct, my google map also can't show anything :-(
QA Contact: chofmann → nspr

Comment 21

11 years ago
mass reassigning to nobody.
Assignee: dougt → nobody
You need to log in before you can comment on or make changes to this bug.