Closed Bug 1312620 Opened 4 years ago Closed 4 years ago

Differential Testing: Different output message involving zero

Categories

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

x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: gkw, Assigned: jschulte)

References

Details

(Keywords: testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file, 2 obsolete files)

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?
Flags: needinfo?(nicolas.b.pierron)
Setting this as a [fuzzblocker] because it is hard to ignore due to it seemingly involving zeroes.
Whiteboard: [fuzzblocker]
Priority: P1 → --
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.
Priority: -- → P1
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.
Attached patch fix.patch (obsolete) — Splinter Review
Assignee: nobody → j_schulte
Status: NEW → ASSIGNED
Attachment #8804223 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8804223 [details] [diff] [review]
fix.patch

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

Sounds good to me :)
Attachment #8804223 - Flags: review?(nicolas.b.pierron) → review+
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 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) {
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.
Attached patch fix_v2.patch (obsolete) — Splinter Review
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.
Attachment #8804223 - Attachment is obsolete: true
Attachment #8804782 - Flags: review?(jwalden+bmo)
Comment on attachment 8804782 [details] [diff] [review]
fix_v2.patch

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

r+ for the Jit part.
Attachment #8804782 - Flags: review+
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.  :-)
Attachment #8804782 - Flags: review?(jwalden+bmo) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c1ef258c04f1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Nicolas, is bug 1176230 a likely regressor?

Yes.
Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.