Differential Testing: Different output message involving zero

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: jschulte)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
mozilla52
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
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)
Reporter

Comment 1

3 years ago
Setting this as a [fuzzblocker] because it is hard to ignore due to it seemingly involving zeroes.
Whiteboard: [fuzzblocker]
Reporter

Updated

3 years ago
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.
Assignee

Comment 4

3 years ago
Posted 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.
Assignee

Comment 9

3 years ago
Posted 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+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 13

3 years ago
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

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c1ef258c04f1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.