1312620 - Differential Testing: Different output message involving zero

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f(x) {
    return x || -0;
}
x = [0, 2147483648];
for (var j = 0; j < 3; ++j) {
    for (var k = 0; k < 2; ++k) {
        print(uneval(f(x[0], x[1])));
    }
}

$ ./js-dbg-64-dm-clang-darwin-215f96861176 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
-0
-0
-0
-0
-0
-0

$ ./js-dbg-64-dm-clang-darwin-215f96861176 --fuzzing-safe --no-threads --ion-eager testcase.js
-0
-0
-0
-0
0
0


Tested this on m-c rev 215f96861176.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 215f96861176

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/01d621c2dbe3
user:        Johannes Schulte
date:        Fri Jun 24 19:02:23 2016 +0200
summary:     Bug 1176230 - Try to fold ternary's with double-argument to NaNToZero. r=nbp

Nicolas, is bug 1176230 a likely regressor?

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting this as a [fuzzblocker] because it is hard to ignore due to it seemingly involving zeroes.

Comment 2

[Hannes Verschore [:h4writer]]

Adding P1 because of the fuzzblocker. Also this issue landed in 52, it would be nice to get it fixed in 52 too. That way we don't have to uplift.

Comment 3

[Nicolas B. Pierron [:nbp]]

Ok, this should be an easy fix, the problem is in MPhi::foldsTernary(), where we compare:
  c->numberToDouble() == 0

Which is true for both  +0.0  and  -0.0.
We should change this condition to only accept +0.0.

Comment 4

[Johannes Schulte [:jschulte]]

Attachment: fix.patch

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8804223 [details] [diff] [review]
fix.patch

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

Sounds good to me :)

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8804223 [details] [diff] [review]
fix.patch

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

(sorry for drive-by)

::: js/src/jit/MIR.cpp
@@ +2418,5 @@
>  
>      // If testArg is an double type we can:
>      // - fold testArg ? testArg : 0.0 to MNaNToZero(testArg)
> +    double constDouble = c->numberToDouble();
> +    if (testArg->type() == MIRType::Double && constDouble == 0 && !mozilla::IsNegativeZero(constDouble)  && c != trueDef) {

nit:
- two spaces before final &&, remove one
- condition is more than 100 chars, please break it into several lines

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8804223 [details] [diff] [review]
fix.patch

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

::: js/src/jit/MIR.cpp
@@ +2418,5 @@
>  
>      // If testArg is an double type we can:
>      // - fold testArg ? testArg : 0.0 to MNaNToZero(testArg)
> +    double constDouble = c->numberToDouble();
> +    if (testArg->type() == MIRType::Double && constDouble == 0 && !mozilla::IsNegativeZero(constDouble)  && c != trueDef) {

A slightly more concise (and IMO clearer) form might be

  if (testArg->type() == MIRType::Double && mozilla::NumbersAreIdentical(constDouble, +0.0)  && c != trueDef) {

Comment 8

[Jeff Walden [:Waldo]]

Or, for that matter, I think we could easily accept a mozilla::IsPositiveZero function added to the relevant mfbt header.  That's likely better, even.

Comment 9

[Johannes Schulte [:jschulte]]

Attachment: fix_v2.patch

Thanks for the quick reviews!
I guess, I have to ask you for review, as this touches mfbt?
If not, feel free to forward to nbp again.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8804782 [details] [diff] [review]
fix_v2.patch

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

r+ for the Jit part.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8804782 [details] [diff] [review]
fix_v2.patch

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

::: mfbt/FloatingPoint.h
@@ +188,5 @@
>  
> +/** Determines wether a float/double represents +0. */
> +template<typename T>
> +static MOZ_ALWAYS_INLINE bool
> +IsPositiveZero(T aValue)

Looks good, but: add |using mozilla::IsPositiveZero;| at the alphabetically-correct place in mfbt/tests/TestFloatingPoint.cpp, then copy the big block of IsNegativeZero tests a bit further down, do a s/IsNegative/IsPositive/g within that copied block, and flip the expected results for -0 and +0.  I assume what needs doing is clear enough we can skip another round of review.  :-)

Comment 12

[Johannes Schulte [:jschulte]]

Attachment: fix_v3.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa522d42bdd5&selectedJob=30016427

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ef258c04f1
Add IsPositiveZero function to mfbt and use it to replace MPhi by MNaNToZero iff c is +0.0. r=nbp, r=waldo

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Nicolas, is bug 1176230 a likely regressor?

Yes.