1314438 - Differential Testing: Different output message involving Math functions

Status: RESOLVED FIXED

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

Description

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

function f(x) {
    return Math.pow(x, x) && 0;
}
f(0);
print(uneval(f(-999)));


$ ./js-dbg-64-dm-clang-darwin-37ab1d54a08e --fuzzing-safe --no-threads --ion-eager testcase.js
0

$ ./js-dbg-64-dm-clang-darwin-37ab1d54a08e --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
-0

Tested this on m-c rev 37ab1d54a08e.

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 37ab1d54a08e

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f99da59c4383
user:        Johannes Schulte
date:        Thu Feb 04 16:53:41 2016 +0100
summary:     Bug 1311801 - Fold MTest to a Goto, if possible. r=h4writer

Johannes/Hannes, is bug 1311801 a likely regressor?

Marking this [fuzzblocker] because this seems to happen often and I don't know if Math.pow is the real issue.

Comment 1

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

function f(x) {
    return (-1 % x && Math.cos(8) >>> 0);
}
f(2);
print(uneval(f(-1)));


$ ./js-dbg-64-dm-clang-darwin-37ab1d54a08e --fuzzing-safe --no-threads --ion-eager testcase.js
0

$ ./js-dbg-64-dm-clang-darwin-37ab1d54a08e --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
-0

Here's another testcase - the issue might be with Math functions instead.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: patch

Fixing multiple issues here.
The actual bug is in bug 1176230. But bug 1311801 made it much easier to hit it. I had to put everything back in my head and dived through the code and find the possible other issues too.


1) MPhi::foldsTernary
In there we removed a use of "testDef" based on the type being MIRType::Int32. As a result we need to add "setGuardRangeBailouts()" to make sure this predicate is kept.

2) NeedNegativeZeroCheck
This needs to check "setGuardRangeBailouts" and not remove a possible -0 check in that case. Because the guard means that we added an optimization based on the type of that instruction.

3) Make sure we don't remove "setGuardRangeBailouts" during GVN

4) MCompare::tryFoldEqualOperands

Always add setGuardRangeBailouts. Different passes might reason about the uses and decide wrong things, since "setGuardRangeBailouts" wasn't added if it still had a use.

Comment 3

[Johannes Schulte [:jschulte]]

Thanks for looking into this!

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8806822 [details] [diff] [review]
patch

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

I am fine with the rest of the patch except for:
(what do you think?)

::: js/src/jit/MIR.cpp
@@ +2942,5 @@
>              continue;
>  
>          MDefinition* use_def = use->consumer()->toDefinition();
> +        if (use_def->isGuardRangeBailouts())
> +            return true;

Adding this inside the loop over the uses sounds too greedy.
I would suggest adding it after the isTruncated() calls for Add & Sub, as well as all other instruction, except for bitwise instructions, Abs and TurnateToInt32.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Patch

I removed the "isGuardRangeBailouts" on the uses. I was too eager there. Good observation.

Comment 6

[Hannes Verschore [:h4writer]]

I've created and updated https://wiki.mozilla.org/IonMonkey/Global_value_numbering#Common_expressions to document when to add "setGuardRangeBailouts". Feel free to improve the docs!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbd085908c8
IonMonkey - Guard we don't remove instructions where we optimized based on its type, r=nbp

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/969ad213c983
Follow-up fix to only guard if it isn't a guard already ON CLOSED TREE, r=bustage

Comment 9

[Iris Hsiao [:ihsiao]]

backed out for bustage, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=38756422&repo=mozilla-inbound#L19663, https://treeherder.mozilla.org/logviewer.html#?job_id=38756359&repo=mozilla-inbound

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5dfb4395051
Backed out changeset 969ad213c983 for bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d553507a16
Backed out changeset 3cbd085908c8

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Patch

Since we now always add "GuardRangeBailouts". The assert in tryRemovingGuards was incorrect. I changed it to stop propagating the GuardRangeBailouts if we find a "DeadIfUnused without GuardRangeBailouts". As a result we keep the GuardRangeBailouts, but we stop with hoisting as soon as it wouldn't make any sense anymore.

Comment 12

[Hannes Verschore [:h4writer]]

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

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

This fixes the issues I was seeing.

Comment 13

[Hannes Verschore [:h4writer]]

Haven't pushed, since my try-push was showing failures again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24c5ecf54fb2&selectedJob=30562285. Couldn't repro locally, but investigating before pushing.

Comment 14

[Hannes Verschore [:h4writer]]

Opened bug 1316557 for the failures I was seeing.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0d7c338607
IonMonkey - Guard we don't remove instructions where we optimized based on its type, r=nbp

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c0d7c338607