911369 - Differential Testing: Different output message involving valueOf

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

a1 = b1 = new ArrayBuffer();
for (x = 0; x < 9; ++x) {
    Array.prototype.unshift.call(a1, b1)
}
a1[0] = 7
Array.prototype.reverse.call(a1).valueOf = (function () {
    print("foo")
})
__proto__ = []
f0 = (function (a, a1) {
    var r = 6 - a1
});
Array, reduceRight.call(a1, f0, 2)

prints "foo" 8 times in a js opt 64-bit deterministic threadsafe shell on m-c changeset c7459bc8e449 without any CLI arguments but prints "foo" only 2 times with --ion-eager.

My configure flags are:

-target=x86_64-apple-darwin11.4.0 --enable-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-threadsafe --with-ccache <other NSPR flags>

This bug has probably existed for some time. Setting needinfo from jandem, as this is another bug that blocks differential testing.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Problem is that the "6 - a" expression is specialized as int32 based on baseline feedback. DCE then eliminates the ToInt32 and Sub instructions and we don't invoke the valueOf method.

The same issue came up a long time ago with the bitwise operators, fix then was to never specialize these if one operand may be an object. This patch does the same for the arithmetic operators.

I also noticed that we call HasOperationOverflowed and pass a "bool overflowed" around but no longer do anything with it, so the patch removes that as well.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 798487 [details] [diff] [review]
Patch

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

This looks OK, but it seems weird that we need to do it this way and I wonder if this patch is masking a deeper problem.  int32/double instructions require int32/double inputs, and it is the ToInt32 on the object where the side effect happens.  If DCE is eliminating that instruction then it is not being considered effectful and we could get correctness problems from the ToInt32 being consolidated or moved across other instructions.

Comment 4

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2977628a9d5

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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