927430 - Add an analysis step to detect is-NaN testing using |x != x| (or is-not-NaN using |x == x|)

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Jeff Walden [:Waldo]]

Various compilers historically have had problems with testing a floating point value for NaN using what you'd expect would work given IEEE-754.  We should break the build, warn, complain, whatever, and tell people to use mozilla::IsNaN instead (the only thing we're aware of that always, correctly, works).

Comment 1

[(no longer active)]

Can you please rephrase what the analysis here should check for more clearly?

Comment 2

[Joshua Cranmer [:jcranmer]]

I would imagine that the check should be for boolean expressions of the form x != x, where x is of type float, double, or long double.

Comment 3

[(no longer active)]

Oh, ok, that's super easy!  :-)

Comment 4

[(no longer active)]

So there is this code <http://dxr.mozilla.org/mozilla-central/source/gfx/skia/trunk/include/core/SkScalar.h#41> in skia which this analysis catches.  George, can we keep a local patch applied on top of skia to make this function return mozilla::IsNaN() instead?

Comment 5

[Jeff Walden [:Waldo]]

Comment 2 and comment 3 are exactly right.  :-)

Comment 6

[Jeff Walden [:Waldo]]

...actually, wait, comment 2 is not exactly right.  There's also the inverse case: x == x where x is float, double, or long double, which should be replaced with !SkScalarIsNaN(x).  SkScalarIsFinite several lines below SkScalarIsNaN's definition has such an instance.  (In the specific case of SkScalarIsFinite, the body there could be replaced with a call to mozilla::IsFinite, for what it's worth.)

Comment 7

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> ...actually, wait, comment 2 is not exactly right.  There's also the inverse
> case: x == x where x is float, double, or long double

Yes, my analysis catches it as well! :-)  Waiting for George before I attach my patch here.

Comment 8

[George Wright (:gw280) (needinfo me!)]

Not forgotten about this. Just wondering how the best way to deal with this is. I really don't like the idea of keeping local patches applied.

Is it possible to upstream our IsNan implementation?

Comment 9

[(no longer active)]

(In reply to George Wright (:gw280) from comment #8)
> Is it possible to upstream our IsNan implementation?

I'm not sure if the upstream project would be interested...  If you can please check with them, I would be happy to write something based on http://dxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h.

Another possible solution, assuming the upstream project is OK with it, is to check in some Mozilla specific code into skia, roughly like this:

static inline bool SkScalarIsNaN(float x) {
#ifdef MOZILLA_CLIENT
  return mozilla::IsNaN(x);
#else
  return x != x;
#endif
}

which option do you think would be more viable?

Comment 10

[George Wright (:gw280) (needinfo me!)]

I think in general they are opposed to having ifdef blocks specifically for various platforms if they can avoid it. If the new IsNaN code is demonstrably better than theirs then my experience has shown that they'll generally take it.

Comment 11

[(no longer active)]

At this point, I don't think that's worth spending time on.  We should just white-list SkScalar.h and move on with our lives.

Comment 12

[(no longer active)]

Attachment: Add a static analysis to detect is-NaN testing using |x == x| or |x != x|

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Waldo, do you have any details on the problems? It would be nice to document them in this change.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9efbbfa8e6

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bd9efbbfa8e6

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> Waldo, do you have any details on the problems? It would be nice to document
> them in this change.

The failures were either miscompiled code or compile errors, on MSVC and gcc both at the time.  Compiling with PGO enabled typically exacerbated these problems, or made them occur only in particular places, or other mayhem.

Generally there seems to be a very long trail of tears concerning how compilers have compiled floating point comparisons.  A brief survey of the field reveals bug 653056, bug 653777, <http://hg.mozilla.org/mozilla-central/rev/a1ab5a4ed1f3>, bug 654664, bug 640494 comment 39, bug 859892 comment 11, bug 977538, bug 435442 comment 58, and probably others as relevant instances of problems performing floating point comparisons -- most (but not all) of them against NaN.  Granted, most of those are older and in an era of somewhat older compilers -- but generally I'm not sure there's enough detail recorded to be much sure of when the affected compilers may have become unsupported, if indeed all of them even were.

Anyway.  That's not really the sort of braindump I'd want to put in a comment in code, unfortunately, with it being so fuzzy.  :-(  I'd leave it as something along the lines of there having been compiler bugs in these regards in the past, directing to this bug and perhaps this comment for the inchoate research.

Comment 17

[Jeff Walden [:Waldo]]

I see this analysis only works if the locations being compared are exactly the same lvalue.  I don't know how many more places it'd detect, but it might be worth a quick elaboration to detect structurally equivalent expressions -- that is, say, methodReturningFloat() != methodReturningFloat(), or obj->prop() != obj->prop() or so -- that are compared in this manner, but haven't been CSE'd by the human author.  I have memories of having seen such comparisons before and fixed them to store into a local, then do the comparison using that (or to use the mfbt methods), and it wouldn't surprise me if there were more such places in our tree.

Comment 18

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> I see this analysis only works if the locations being compared are exactly
> the same lvalue.  I don't know how many more places it'd detect, but it
> might be worth a quick elaboration to detect structurally equivalent
> expressions -- that is, say, methodReturningFloat() !=
> methodReturningFloat(), or obj->prop() != obj->prop() or so -- that are
> compared in this manner, but haven't been CSE'd by the human author.  I have
> memories of having seen such comparisons before and fixed them to store into
> a local, then do the comparison using that (or to use the mfbt methods), and
> it wouldn't surprise me if there were more such places in our tree.

Can you please file some bugs on these so that I don't forget to write the analysis?  I think they're relatively easy to detect.