1660405 - Replace mozilla::IsNaN with std::isnan, etc.

Status: RESOLVED FIXED

(Core :: MFBT, task)

Description

[Mike Hommey [:glandium]]

I just hit an error in third party code that the clang plugin emits when a float is compared to itself for NaN detection. That error invites to use mozilla::IsNaN. That function comes from the C++-ification of a C macro that used to live under content/ all the way back to bug 278981, back when C++11 was not a thing.

It seems we should be able to replace it with the now C++ std equivalent, and the same for IsInfinite and probably others.

Of course, the clang-plugin error message should be adjusted too.

Comment 1

[Steve Fink [:sfink] [:s:]]

This is also necessary for upgrading gcc to gcc 10 (or maybe 11?). mozilla::IsNaN will make gcc issue a warning that will fail a warnings-as-errors build.

Comment 2

[Mike Hommey [:glandium]]

We have builds on automation with gcc 11, with warnings-as-errors, and they're not failing. https://treeherder.mozilla.org/jobs?repo=mozilla-central&searchStr=linux%2Cbuild%2Cgcc

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

This is also necessary for upgrading gcc to gcc 10 (or maybe 11?). mozilla::IsNaN will make gcc issue a warning that will fail a warnings-as-errors build.

The warning could be #pragma'd off if it came to it.

The problem with the sanctioned C++ forms was miscompilation with various compilers. Wouldn't be hard to check if that's an issue still by just replacing the bodies of all the functions with calls to the standard functions, then let it sit awhile and see if regressions are reported.

Comment 4

[Steve Fink [:sfink] [:s:]]

Hm, I can't reproduce it locally right now. Perhaps it was the gcc that came with Fedora 33? I recently upgraded to 34, and now have gcc-11.1.1.

This came up because tcampbell ran into it a day or two ago when attempting to reproduce a jsapi-tests failure. (I think he's on Ubuntu, though, not Fedora.) Which is weird, come to think of it, because now that autospider.py runs via mach I would expect it to get the compiler from ~/.mozbuild/gcc.

Comment 5

[Mike Hommey [:glandium]]

We don't bootstrap gcc.

Comment 6

[Steve Fink [:sfink] [:s:]]

tcampbell says "./mach build + a mozconfig that said CC=gcc CXX=g++", and "my system gcc is 9.3".

I compiled the JS shell with gcc 9.3.0 (it's what get installed by mach hazards bootstrap). Still no warning.

Bleh. I will ignore this for now.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

This looks easy enough to be a good first bug. Please unset it if anyone disagrees.

Comment 8

[dsmurrow]

I'd like to work on this!

Comment 9

[Mike Hommey [:glandium]]

You're welcome to :)

Comment 10

[dsmurrow]

This is my first bug, I've replaced all instances of mozilla::IsNaN with std::isnan, mozilla::IsInfinite with std::isinf, and mozilla::IsFinite with std::isfinite. I am unsure which review group(s) I should submit this to, considering how the changes are in many different modules, but they are also relatively inconsequential within each module, could I get some guidance? :')

Comment 11

[dsmurrow]

Attachment: Bug 1660405 - Replace mozilla::IsNaN with std::isnan, etc. r?glandium

Replaced mozilla::IsNaN with std::isnan, mozilla::IsInfinite with
std::isinf, and mozilla::IsFinite with std::isfinite. Also removed their
declarations from mfbt/FloatingPoint.h and replaced them with std
equivalents in mfbt/tests/TestFloatingPoint.cpp (although it'd probably
be cleaner to just get rid of the tests).

Also changed message in build/clang-plugin/NaNExprChecker.cpp to
advocate using std::isnan.

Comment 12

[Kagami Rosylight [:saschanaz] (they/them)]

Late to the party, but I'd split the patches per the components for easier review. Including everything in a single patch often requires multiple reviews which can be frustrating. Splitting allows you to land one by one as the review proceeds.

Comment 13

[Ranjan]

Is this issue still active? I would love to contribute it to it as my first bug!!

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 15

[Kagami Rosylight [:saschanaz] (they/them)]

I missed comment #13, yes it is active.

Comment 16

[Andi [:andi]]

Attachment: Bug 1660405 - Move away from mozilla::IsNaN in favor of std::isnan.

Comment 17

[Andi [:andi]]

Attachment: Bug 1660405 - Move away from mozilla::IsFinite in favor of std::isfinite.

Depends on D173035

Comment 18

[Andi [:andi]]

Attachment: Bug 1660405 - Move away from mozilla::IsInfinite in favor of std::isinf.

Depends on D173036

Comment 19

[Andi [:andi]]

glandium what do you say, should we also remove mozilla::IsNegative()? My main concern is that it encapsulates a MOZ_ASSERT.

Comment 20

[Andi [:andi]]

Attachment: Bug 1660405 - remove unused mozilla::IsNaN.

Depends on D173037

Comment 21

[Andi [:andi]]

Attachment: Bug 1660405 - remove unused mozilla::IsInfinite.

Depends on D173048

Comment 22

[Andi [:andi]]

Attachment: Bug 1660405 - remove unused mozilla::IsFinite.

Depends on D173049

Comment 23

[Mike Hommey [:glandium]]

(In reply to Andi [:andi] from comment #19)

glandium what do you say, should we also remove mozilla::IsNegative()? My main concern is that it encapsulates a MOZ_ASSERT.

What would you replace it with?

Comment 24

[Andi [:andi]]

Attachment: Bug 1660405 - Transform mozilla::IsNegative to use std implementation. r=glandium

Since we cannot move away from mozilla::IsNegative to std::signbit because the
first one doesn't accept a NaN we should transform our function to use std implementation.

Depends on D173050

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/11b0f9cf8091
Move away from mozilla::IsNaN in favor of std::isnan. r=nbp,media-playback-reviewers,sergesanspaille,padenot
https://hg.mozilla.org/integration/autoland/rev/e27052da4927
Move away from mozilla::IsFinite in favor of std::isfinite. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/535cc12c8bed
Move away from mozilla::IsInfinite in favor of std::isinf. r=nbp,media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/8419b99aab60
remove unused mozilla::IsNaN. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/48629ee0d70d
remove unused mozilla::IsInfinite. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/a39e95f0aafe
remove unused mozilla::IsFinite. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/ad1a5f59214f
Transform mozilla::IsNegative to use std implementation. r=glandium

Comment 26

[Serban Stanca [:SerbanS]]

Backed out for causing wasi bustages on Linux x64 opt.

• Backout link
• Push with failures - wasi bustages
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/jsdate.cpp:544:8: error: use of undeclared identifier 'IsFinite'; did you mean 'isfinite'?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/458f942ecef8
Move away from mozilla::IsNaN in favor of std::isnan. r=nbp,media-playback-reviewers,sergesanspaille,padenot
https://hg.mozilla.org/integration/autoland/rev/e44ca4fc1195
Move away from mozilla::IsFinite in favor of std::isfinite. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/3735e6d16998
Move away from mozilla::IsInfinite in favor of std::isinf. r=nbp,media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/e0fbc2d0840b
remove unused mozilla::IsNaN. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/647666f69ada
remove unused mozilla::IsInfinite. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/0b66fb4d1382
remove unused mozilla::IsFinite. r=sergesanspaille
https://hg.mozilla.org/integration/autoland/rev/9385659b1013
Transform mozilla::IsNegative to use std implementation. r=glandium

Comment 28

[mac198442]

It seems there were two references t0 IsFinite that were bare with no mozilla:: so were missed by your search.

Comment 29

[mac198442]

Meant to say 2 references is jsdate.cpp to IsFinite

Comment 30

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/458f942ecef8
https://hg.mozilla.org/mozilla-central/rev/e44ca4fc1195
https://hg.mozilla.org/mozilla-central/rev/3735e6d16998
https://hg.mozilla.org/mozilla-central/rev/e0fbc2d0840b
https://hg.mozilla.org/mozilla-central/rev/647666f69ada
https://hg.mozilla.org/mozilla-central/rev/0b66fb4d1382
https://hg.mozilla.org/mozilla-central/rev/9385659b1013