Closed Bug 1314438 Opened 4 years ago Closed 4 years ago

Differential Testing: Different output message involving Math functions


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




Tracking Status
firefox52 --- fixed


(Reporter: gkw, Assigned: h4writer)



(Keywords: testcase, Whiteboard: [fuzzblocker])


(1 file, 2 obsolete files)

function f(x) {
    return Math.pow(x, x) && 0;

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

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

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/ -b "--enable-debug --enable-more-deterministic" -r 37ab1d54a08e

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
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.
Flags: needinfo?(j_schulte)
Flags: needinfo?(hv1989)
function f(x) {
    return (-1 % x && Math.cos(8) >>> 0);

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

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

Here's another testcase - the issue might be with Math functions instead.
Summary: Differential Testing: Different output message involving Math.pow → Differential Testing: Different output message involving Math functions
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8806822 - Flags: review?(nicolas.b.pierron)
Blocks: 1176230
No longer blocks: 1311801
Flags: needinfo?(j_schulte)
Thanks for looking into this!
Comment on attachment 8806822 [details] [diff] [review]

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.
Attachment #8806822 - Flags: review?(nicolas.b.pierron)
Attached patch Patch (obsolete) — Splinter Review
I removed the "isGuardRangeBailouts" on the uses. I was too eager there. Good observation.
Attachment #8806822 - Attachment is obsolete: true
Attachment #8807070 - Flags: review?(nicolas.b.pierron)
I've created and updated to document when to add "setGuardRangeBailouts". Feel free to improve the docs!
Attachment #8807070 - Flags: review?(nicolas.b.pierron) → review+
Pushed by
IonMonkey - Guard we don't remove instructions where we optimized based on its type, r=nbp
Pushed by
Follow-up fix to only guard if it isn't a guard already ON CLOSED TREE, r=bustage
Attached patch PatchSplinter Review
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.
Attachment #8807070 - Attachment is obsolete: true
Comment on attachment 8808117 [details] [diff] [review]

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

This fixes the issues I was seeing.
Attachment #8808117 - Flags: review?(nicolas.b.pierron)
Attachment #8808117 - Flags: review?(nicolas.b.pierron) → review+
Haven't pushed, since my try-push was showing failures again: Couldn't repro locally, but investigating before pushing.
Opened bug 1316557 for the failures I was seeing.
Pushed by
IonMonkey - Guard we don't remove instructions where we optimized based on its type, r=nbp
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.