Differential Testing: Different output message involving Math functions

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
major
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: h4writer)

Tracking

(Blocks 2 bugs, {testcase})

Trunk
mozilla52
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
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.
Flags: needinfo?(j_schulte)
Flags: needinfo?(hv1989)
Reporter

Comment 1

3 years ago
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.
Summary: Differential Testing: Different output message involving Math.pow → Differential Testing: Different output message involving Math functions
Assignee

Comment 2

3 years ago
Posted 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)
Assignee

Updated

3 years ago
Blocks: 1176230
No longer blocks: 1311801
Assignee

Updated

3 years ago
Flags: needinfo?(j_schulte)
Thanks for looking into this!
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.
Attachment #8806822 - Flags: review?(nicolas.b.pierron)
Assignee

Comment 5

3 years ago
Posted 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)
Assignee

Comment 6

3 years ago
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!
Attachment #8807070 - Flags: review?(nicolas.b.pierron) → review+

Comment 7

3 years ago
Pushed by hv1989@gmail.com:
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

3 years ago
Pushed by hv1989@gmail.com:
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
Assignee

Comment 11

3 years ago
Posted 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
Assignee

Comment 12

3 years ago
Comment on attachment 8808117 [details] [diff] [review]
Patch

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+
Assignee

Comment 13

3 years ago
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.
Assignee

Comment 14

3 years ago
Opened bug 1316557 for the failures I was seeing.

Comment 15

3 years ago
Pushed by hv1989@gmail.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c0d7c338607
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1317675
You need to log in before you can comment on or make changes to this bug.