208048 - ARM constant number definitions are not correct for infinities

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Justin Fletcher]

User-Agent:       Mozilla/4.72 [en] (Compatible; RISC OS 4.36; Oregano 1.10)
Build Identifier: JavaScript-C 1.5 pre-release 5 2003-01-10 [checked out of CVS trunk 2003-30-05]

The values declared for POSITIVE_INFINITY, NEGATIVE_INFINITY and NaN are not 
correct for the ARM platform. As noted in the jsnum.h source as a comment from 
Stefan Hanske, the words are in little endian byte order, but the two words are 
reversed. The code was changed to reflect this and macros declared in jsnum.h 
to access these values in a generic manner.

The define CPU_IS_ARM is used, together with IS_LITTLE_ENDIAN to select the 
correct macros to use at jsnum.h, line 104. This appears to all be correct.

jsnum.c, however, has its own declaration of 'union dpun' which is used in the 
js_InitRuntimeNumberState functions to initialise the number structures.

If clauses within the 'union dpun' need to be updated to cater for the ARM CPU 
with something like :

#if defined(IS_LITTLE_ENDIAN) && !defined(CPU_IS_ARM)

or the assignment of the values should be updated to use the macros declared
in jsnum.c.



Reproducible: Always

Steps to Reproduce:
1. Build the standard engine and shell application.
2. print(Number.POSITIVE_INFINITY);
3. print(Number.NEGATIVE_INFINITY);

Actual Results:  
Output:
2.  1.06047983e-314
3.  2.1214777256e-314


Expected Results:  
Should have produced:
2.  Infinity
3.  -Infinity

Making the conditional in 'union dpun' as suggested above generated the 
Infinity/-Infinity output as expected.

I believe that whilst the above change to the 'union dpun' will fix the 
problem, the correct solution would be to make the code use the macros from 
jsnum.h as they were intended. If you want a patch putting together for that, 
let me know and I'll do that.

This bug has apparently been unnoticed for quite some time which leads me to 
believe that I may be incorrect in my assertion.

Comment 1

[Phil Schwartau]

Reassigning; cc'ing Brendan and Waldemar -

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Thanks, I think this is a real bug.  Not sure why it wasn't discovered earlier.


I took the opportunity to consolidate the union type, preferring shorter names
(making for shorter ugly macros).

/be

Comment 3

[Brendan Eich [:brendan]]

Easy portability fix, candidate for the 1.4 branch as well as the 1.5 trunk.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 124841 [details] [diff] [review]
proposed fix

Roger, can you eyeball this?  Thanks,

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 124841 [details] [diff] [review]
proposed fix

rjesup pre-approved pending review.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed in trunk and branch, thanks to Justin for reporting this.

/be

Comment 7

[Phil Schwartau]

Marking Verified.

No regressions introduced on Linux or WinNT, as per the JS testsuite.
Also, checkin verified on 1.4 branch.