Closed Bug 995673 Opened 7 years ago Closed 7 years ago

Differential Testing: Different output message involving Math.fround and valueOf

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: gkw, Assigned: h4writer)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file)

x = [ [] ]
m = [].__proto__.__proto__ = this
valueOf = print
function f(y) {
    y != Math.fround()
}
(function() {
    f()
    f(x[0])
    f(x[0])
})()

$ ./js-opt-64-dm-ts-linux-6149db60c6cb --fuzzing-safe --ion-parallel-compile=off test.js


$ ./js-opt-64-dm-ts-linux-6149db60c6cb --fuzzing-safe --ion-parallel-compile=off --ion-eager test.js 

$

(2 new lines with the first result, 1 new line with the second)

My configure flags are:

AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/eafe6b0acd33
user:        David Caabeiro
date:        Wed Oct 02 17:27:50 2013 +0200
summary:     Bug 896264 - Implement Math.hypot(). r=jorendorff.

Jason, is bug 896264 a possible regressor?
Flags: needinfo?(jorendorff)
Severity: critical → major
Hannes / Nicolas, you folks seem to be working on other recent differential testing bugs - is anyone likely to be able to take this on as well?
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(hv1989)
I think the regression range might be wrong again. (revision before eafe6b0acd33 also fails).
Doing bisect now!
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jorendorff)
Flags: needinfo?(hv1989)
The problem is again that we remove an unbox/ToXXX operation. This time because the comparison < is not used and DCE removes it. I have a solution for it, but it will require some 'bigger' changes to fix this once and for all.

I think we should mark all ToXXX/Unbox that might get in contact with objects as "guard". So we can't eliminate them...
So I made all MToXXX setGuard when input is possibly an object, since in that case we shouldn't remove the operation, since the instruction is effectfull. If an object has "valueOf" set, the MToXXX will make sure the "valueOf" on the object is called.
- MUnbox already adds setGuard for fallible MUnboxs. (I think we can even loosen this test and only set it when input can be a MIRType_Object. Since TI doesn't need the information anymore...).
- MToString also doesn't support MIRType_Object. So it wasn't affected.
Assignee: nobody → hv1989
Attachment #8407606 - Flags: review?(jdemooij)
Oh as for regression range we can say this was present one way or the other since IonMonkey landed. That particular testcase won't go that far, but there might be other paths that were "vulnerable". That means FF18+
Comment on attachment 8407606 [details] [diff] [review]
bug995673-unbox-setguard

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

Good catch.

::: js/src/jit/MIR.h
@@ +2823,5 @@
>      {
>          setResultType(MIRType_Double);
>          setMovable();
> +
> +        // An object might have "valueOf", which means it is effectfull.

Nit: effectful (one l), also 4 times below.
Attachment #8407606 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/6f8bee9b010f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.