118117 - Improper single precision constants in nsUnitConversion.h

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[tenthumbs]

Look at the definitions of CEIL_CONST_FLOAT and
ROUND_EXCLUSIVE_CONST_FLOAT in mozilla/xpcom/ds/nsUnitConversion.h. The
constants are too precise for single precision IEEE floating point. On
Linux, gcc quietly converts them to the nearest single precision value
which are 1.0 and 0.5 respectively. This breaks the functions that use
these constants.

The comments in the code indicate the author knew something could go 
wrong and it has. On Linux, it should really be something like
  #define CEIL_CONST_FLOAT (1.0f - FLT_EPSILON)
  #define ROUND_EXCLUSIVE_CONST_FLOAT (0.5f*CEIL_CONST_FLOAT)

That should probably work for ANSI compilers but I'm sure there's some 
exception out there.

Because this affects all sorts of things, I'm setting the severity to 
critical.

Comment 1

[tenthumbs]

We're subtracting, not adding, so it should be
  #define CEIL_CONST_FLOAT (1.0f - 0.5f*FLT_EPSILON)

Comment 2

[Doug Turner (:dougt)]

blames to peterl.  not sure if he actually added these constants.

Comment 3

[Peter Lubczynski]

These belong to the peterl before me, --->back to XPCOM

Note: I'm "peterlubczynski" in CVS.

Comment 4

[Doug Turner (:dougt)]

Over to LAYOUT.  This header does not belong in XPCOM.

Comment 5

[Håkan Waara]

tenthumbs, if you have a proposed for this bug, can you attach a patch?  Thanks.

Comment 6

[tenthumbs]

SInce I can't test all compilers I can't provide a patch. However, for
gcc on Linux (and presumably other gcc's) you change the two defines
taht are marked as suspect to something like this:

  #define CEIL_CONST_FLOAT (1.0f - 0.5*FLT_EPSILON)
  #define ROUND_EXCLUSIVE_CONST_FLOAT (0.5f*CEIL_CONST_FLOAT)

This should work on most reasonable compilers but that's not saying 
much. You should also check that the compiler pre-computes these. 
Calculating these at run time is a performance loss.

Comment 7

[Christopher Hoess (gone)]

->Misc Code

Comment 8

[Christopher Hoess (gone)]

->Misc Code

Comment 9

[Vedran Miletic]

retargeting

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135586 [details] [diff] [review]
Patch

Thoughts?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 135586 [details] [diff] [review]
Patch

Looks reasonable.

I think any need for these constants indicates a design flaw...

Comment 13

[Boris Zbarsky [:bzbarsky]]

.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 15

[tenthumbs]

Reality has kept me away from mozilla for the last few weeks so I'm way behind
in bugs. Sorry for the late response.

Unfortunately, the patch is not correct. Presumably CEIL_CONST_FLOAT is supposed
to be less than but as close to 1.0 as possible. FLT_EPSILON is the smallest
number that can be _added_ to 1.0. Subtraction is different, which I said in
comment 2. I have a demo if anyone doesn't believe me (and they shouldn't).

Reopening with a patch upcoming.

Comment 16

[tenthumbs]

Attachment: simple fix patch

Comment 17

[Boris Zbarsky [:bzbarsky]]

.

Comment 18

[tenthumbs]

Attachment: the right fix

Assigning bugs to me is a sure way top make them disappear, but ....

This should do it. Use a real value for FLT_EPSILON not a made-up one jsut in
case. I could be convinced that it might be better to error out if FLT_EPSILON
isn't defined.

Comment 19

[tenthumbs]

Oh yeah, someone will have to check this in for me.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137751 [details] [diff] [review]
the right fix

sr=bzbarsky (I also can't check this in, so someone else would need to do it).

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 137751 [details] [diff] [review]
the right fix

I'll check in

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Assigning to myself for checkin

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fix checked in.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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