Closed Bug 1105574 Opened 5 years ago Closed 5 years ago

Differential Testing: Different output message involving Math.tan

Categories

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

x86_64
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- disabled
firefox37 --- disabled
firefox38 --- disabled
firefox39 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 3 open bugs)

Details

(Keywords: regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file)

function f1(f, inputs) {
    for (var j = 0; j < 2; ++j) {
        for (var k = 0; k < 2; ++k) {
            print(f(inputs[j], inputs[k]));
        }
    }
}
try {
    f2 = function() {}
    f1(f2[z(), []])
} catch (e) {}
f2 = function() {}
f3 = function(x, y) {
    return (Math.tan((Math.log2(Math.log10(Math.min(x, y))) >>> 0) | 0, f2()))
}
f1(f3, [, 3])


$ ./js-dbg-opt-64-dm-nsprBuild-darwin-daf6fc0b0195-1105187-c3-eff24bf5d366 --fuzzing-safe --no-threads --ion-eager 588.js
0
0
0
-0.5722513701805471

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-daf6fc0b0195-1105187-c3-eff24bf5d366 --fuzzing-safe --no-threads --baseline-eager 588.js
0
0
0
-1.557407724654902

Tested this on m-c rev daf6fc0b0195.

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-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9188c8b7962b
user:        Nicolas B. Pierron
date:        Mon Nov 24 16:11:32 2014 +0100
summary:     Bug 1093674 - IonMonkey: Add Sink for instruction which can be recovered on bailout. r=sunfish

Setting [fuzzblocker] because this comprises most of the current differential testing issues being constantly tripped up.

Nicolas, is bug 1093674 a likely regressor?
Flags: needinfo?(nicolas.b.pierron)
This was tested in a binary compiled with the patch in bug 1105187 comment 3, and also occurs in a binary without the patch.
The problem is that Sinking + Range Analysis cause the " | 0 " to disappear before giving the value as argument of Math.tan.

js> Math.tan((-1 >>> 0) | 0, undefined)
-1.5574077246549023
js> Math.tan((-1 >>> 0), undefined)
-0.5722513701805472
Flags: needinfo?(nicolas.b.pierron)
The problem is that Range Analysis removes the bitwise instructions, but the
bitwise instructions are still useful to recover the correct result in case
of a bailout.

The current test case had a different behaviour because the MUrsh
instruction got sank after the function dispatch, and once it failed, it had
to recover the execution of MUrsh, which failed in the first place because
it did not fit in the Int32 range.  As Range Analysis assumed the bailout of
MUrsh, it hypothesized that MUrsh can only return an Int32, and thus that it
is safe to remove the no-op bitwise operation which are after.

These bitwise operations are still needed for recovering the results of sunk
instructions, as the recovered MUrsh can overflow in the Double range.
Attachment #8529904 - Flags: review?(sunfish)
Attachment #8529904 - Flags: review?(sunfish) → review+
Summary: Differential Testing: Different output message involving a variety of Math functions → Differential Testing: Different output message involving Math.tan
Helped to push to fix a fuzzblocker:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b332be3b73f
Target Milestone: --- → mozilla37
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Nicolas, seems like this failed to stick. :(
Flags: needinfo?(nicolas.b.pierron)
Target Milestone: mozilla37 → ---
This failure sounds similar to what we have in Bug 1106171, we should re-try this patch with Bug 1106171 patches.
Flags: needinfo?(nicolas.b.pierron)
Pushing to try, with Bug 1106171 patches, which just landed on inbound & aurora,
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=61c7ebb3f3d7

This way we can see if we still have the previous issue reported in comment 5.
Nope, try shows failures.
Flags: needinfo?(nicolas.b.pierron)
Blocks: 1109195
Comment on attachment 8529904 [details] [diff] [review]
Range Analysis: Keep folded bitwise instructions alive for bailouts.

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

::: js/src/jit/MIR.cpp
@@ +580,5 @@
> +void
> +MDefinition::replaceAllLiveUsesWith(MDefinition *dom)
> +{
> +    for (MUseIterator i(usesBegin()), e(usesEnd()); i != e; ++i) {
> +        MUse *use = *i++;

fixed: the MUse iterator is incremented twice, which cause the assertion hasLiveDefUses throw in the AssertGraphCoherency.  I removed the increment which is in the for statement.
https://hg.mozilla.org/mozilla-central/rev/7529425ef21f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1143216
You need to log in before you can comment on or make changes to this bug.