Last Comment Bug 777494 - Avoid using == with float
: Avoid using == with float
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-25 13:49 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2012-07-26 11:13 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't use == with float (2.92 KB, patch)
2012-07-25 13:49 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
roc: review+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-07-25 13:49:25 PDT
Created attachment 645875 [details] [diff] [review]
don't use == with float

layout/generic/nsTextFrameThebes.cpp currently has some equality comparison with float point. While it looks like they should be valid in a really strict compiler, on x86 there can be some problems.

The x87 stack has higher precision than a float. If one side of the comparison is coming from a computation on the stack and the other one is loaded from memory, there can be a small difference that causes the comparison to fail.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-07-25 14:02:07 PDT
A try push is at https://tbpl.mozilla.org/?tree=Try&pusher=respindola@mozilla.com.
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-07-25 19:33:21 PDT
Comment on attachment 645875 [details] [diff] [review]
don't use == with float

Review of attachment 645875 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +6710,5 @@
>    }
>  }
>  
> +bool nsTextFrame::IsCurrentFontInflation(float aInflation) const {
> +  return fabsf(aInflation - GetFontSizeInflation()) < 1e-6;

If aInflation is greater than 1e6 * epsilon, which is roughly 1e+1, then this is no different than the == comparison it replaces.

So:
 - either you know something very specific here about the order of magnitude of aInflation?
 - or you should use a relative (multiplicative), not absolute, fuzzy comparison. Typically:
  abs(a - b) < precision * min(abs(a), abs(b))
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2012-07-25 19:52:46 PDT
From what I see in the code, the comparison is always used to check if a value is from a previous call to GetFontSizeInflation. The case I noticed it failing was because one side had been stored (narrowed to a float) and the other wasn't. The difference from the long double to the float was about 1e-8, so I just used 1e-6, but I agree it is not a general solution.

Do we have a routine for relative comparison somewhere in the codebase?
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-25 20:25:54 PDT
Font inflation values will generally be between 0.1 and 10 so this is fine.
Comment 5 Chris Peterson [:cpeterson] 2012-07-25 21:44:14 PDT
btw, gcc >= 3.0 has a -Wfloat-equal warning flag that will warn when a floating point value is compared for equality using the == or != operators.

http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_3.html#SEC11
Comment 6 Ed Morley [:emorley] 2012-07-26 05:08:07 PDT
https://hg.mozilla.org/mozilla-central/rev/05852ba29df2
Comment 7 :Ehsan Akhgari (out sick) 2012-07-26 11:13:41 PDT
(In reply to Chris Peterson (:cpeterson) from comment #5)
> btw, gcc >= 3.0 has a -Wfloat-equal warning flag that will warn when a
> floating point value is compared for equality using the == or != operators.
> 
> http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_3.html#SEC11

Filed bug 777803 to enable this warning.

Note You need to log in before you can comment on or make changes to this bug.