1140890 - Differential Testing: Different output message involving Math.imul

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

function f(y) {
    return -(0 != 1 / y) - -Math.imul(1, !y)
}
x = [-0, Infinity]
for (var k = 0; k < 2; ++k) {
    print(uneval(f(x[k])))
}

$ ./js-dbg-64-dm-nsprBuild-darwin-fecf1afb0830 --fuzzing-safe --no-threads --ion-eager testcase.js
0
-0

$ ./js-dbg-64-dm-nsprBuild-darwin-fecf1afb0830 --fuzzing-safe --no-threads --baseline-eager testcase.js
0
0

Tested this on m-c rev fecf1afb0830.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r fecf1afb0830

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/65aea1c559fd
user:        Jan de Mooij
date:        Wed Jan 21 23:06:42 2015 +0100
summary:     Bug 1124002 - Remove unnecessary object/symbol checks in MBinaryArithInstruction::infer. r=h4writer

Jan, is bug 1124002 a likely regressor?

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jan, is bug 1124002 a likely regressor?

No. It's a pre-existing bug in NeedNegativeZeroCheck. The code there assumes that if the second operand to an MSub is 0, it doesn't matter if it's -0 or +0, but that's an invalid assumption:

-0 - -0 => +0
-0 -  0 => -0

NI Hannes but feel free to forward if you didn't add this.

Comment 2

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> > Jan, is bug 1124002 a likely regressor?
> 
> No. It's a pre-existing bug in NeedNegativeZeroCheck. The code there assumes
> that if the second operand to an MSub is 0, it doesn't matter if it's -0 or
> +0, but that's an invalid assumption:
> 
> -0 - -0 => +0
> -0 -  0 => -0

Actually it is a valid assumption. NeedNegativeZeroCheck actually only works on integer instructions. (I agree that just by looking at the code it might be hard to deduce).

In that case:
INT32 - -0 == INT32 - 0

The issue is something that already has been fixed in MAdd, but not in MSub.

MMul x y
MToInt32 z
MSub mtoint32 mmul

Here MMul indeed deduces that the second operand might remove the -0 check. But the MToInt32 bails, since it sees -0. In the resumepoint the value -0 is already changed to 0. And we see -0 - -0.

The quick fix for this is to make sure that the second operand is only evaluated after the first before allowing this optimization. (Like MAdd does).
=> I'll do this fix here.

A bigger fix would be to rewrite negative zero check removal using 'recover instructions'. In that case we would redo the MMul (with negative zero check) when we bailout.
=> I'll open a follow-up bug for this.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 4

[Hannes Verschore [:h4writer]]

followup: bug 1141634

Comment 5

[Nicolas B. Pierron [:nbp]]

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

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

::: js/src/jit/MIR.cpp
@@ -1983,5 @@
>              // Figure out the order in which the addition's operands will
>              // execute. EdgeCaseAnalysis::analyzeLate has renumbered the MIR
>              // definitions for us so that this just requires comparing ids.
> -            MDefinition *first = use_def->toAdd()->getOperand(0);
> -            MDefinition *second = use_def->toAdd()->getOperand(1);

Any reason to remove toAdd()?
Doing so will prevent the compiler from inlining the implementation of getOperand from the final class MAdd.

@@ +2044,5 @@
> +            // can bailout and change type. This should be fine if the first
> +            // operand is executed first. However if the second operand is
> +            // executed first, the first operand can bail and change type and
> +            // become -0.0 while the second operand has already been optimized
> +            // to not make a difference between zero and negative zero.

Use lhs instead of "first operand", and "rhs" instead of "second operand", this would clarify this comment.
Maybe do the same for the code.

@@ +2045,5 @@
> +            // operand is executed first. However if the second operand is
> +            // executed first, the first operand can bail and change type and
> +            // become -0.0 while the second operand has already been optimized
> +            // to not make a difference between zero and negative zero.
> +            if (use_def->getOperand(0)->id() >= use_def->getOperand(1)->id() &&

nit: Inverse this inequality, such that the instruction which is executed first comes first on the statement when the condition is true.

nit: use_def->toSub()->rhs() , and do the same above with toAdd().

note: if getOperand(0) == getOperand(1), then we do not need to check if it can produce a negative zero as 

  -0  -  -0  =  +0
  +0  -  +0  =  +0

Comment 6

[Hannes Verschore [:h4writer]]

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

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

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

Can we please backport this to 38? It is the next ESR and this will help fuzzing on that ESR branch if need be.

Comment 9

[Hannes Verschore [:h4writer]]

Definitely, popped a try build and I'll request uplift if that is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29953d4fd931

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: beta patch

Approval Request Comment

[Feature/regressing bug #]:
FF19? Already for quite some time, so didn't bother to find the exact date. But already for some time.

[User impact if declined]: 
Users won't stumble on this quite easy. But still possible. In that case they will observe -0 as a +0. In most cases quite benign

This is mostly for our fuzzer team. They hit it quite often when fuzzing. So having this fixed on a ESR release will make it easier for them to find other issues in ESR.


[Describe test coverage new/current, TreeHerder]:
This patch/fix is in FF39 and FF40 already. So it is extensively tested there. Though I did two try tests and Tomcat confirmed they don't contain anything related to these patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92fcd8cd88ac
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29953d4fd931


[Risks and why]: Low. Quite understandable code and patch. So the risk should be very contained.

[String/UUID change made/needed]: /

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8594655 [details] [diff] [review]
beta patch

The fix landed in m-c for a month and we are in the second half of the beta cycle.
So, next time, please act faster...
Taking it because 38 is an ESR, otherwise, it would have been rejected.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8594655 [details] [diff] [review]
beta patch

[Triage Comment]
38 is now in m-r

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d55fdde73ac8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d55fdde73ac8