1319242 - Differential Testing: Different output message involving addition

Status: RESOLVED FIXED

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

Description

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

function f(x) {
    return ((-4294967295 | 0) + (x | 0) | 0) + (x, -4);
}
f(Infinity);
print(f(-2147483649));


$ ./js-dbg-64-dm-clang-darwin-0534254e9a40 --fuzzing-safe --no-threads --ion-eager testcase.js
2147483644

$ ./js-dbg-64-dm-clang-darwin-0534254e9a40 --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
-2147483652

Tested this on m-c rev 0534254e9a40.

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 0534254e9a40

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/07358be0ec02
user:        Sander Mathijs van Veen
date:        Tue Oct 11 07:04:00 2016 +0200
summary:     Bug 1295130 - Fold AddI opcode with constant into other AddI with constant r=nbp

Jan/Nicolas, is bug 1295130 a likely regressor?

Note that this is quite difficult to ignore as this seems to involve addition, so setting [fuzzblocker] as it blocks differential testing.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> The first bad revision is:
> summary:     Bug 1295130 - Fold AddI opcode with constant into other AddI
> with constant r=nbp
> 
> Jan/Nicolas, is bug 1295130 a likely regressor?

This is plausible, the test case has the same layout as Bug 1316830.
I will look into it right now.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: ExtractLinearSum should remain within the same arithmetic space.

The problem here is that ExtractLinearSum, used by the FoldLinearArith phase
which was never meant to work with Truncated math, but only before Range
Analysis, where the only truncated operations are MMul (Math.imul) not
handled by ExtractLinearSum (not mentioning asm.js / wasm where all int32
are truncated).

This patch add a MathSpace argument to the ExtractLinearSum function which
is used to stop the recursion if the operands are operating in a different
space. (Modulo 2^32 / Infinite)

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8813303 [details] [diff] [review]
ExtractLinearSum should remain within the same arithmetic space.

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

Thanks for adding comments to this function.

::: js/src/jit/IonAnalysis.cpp
@@ +3148,5 @@
> +    MDefinition* rhs = ins->getOperand(1);
> +    if (lhs->type() != MIRType::Int32 || rhs->type() != MIRType::Int32)
> +        return SimpleLinearSum(ins, 0);
> +
> +    // Extract linear sums of each operands.

Nit: s/operands/operand/

@@ +3152,5 @@
> +    // Extract linear sums of each operands.
> +    SimpleLinearSum lsum = ExtractLinearSum(lhs, space);
> +    SimpleLinearSum rsum = ExtractLinearSum(rhs, space);
> +
> +    // LinearSum only consider a single term operand, if both sides have

Nit: considers

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5975fad5d1
ExtractLinearSum should remain within the same arithmetic space. r=jandem

Comment 5

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=39715343&repo=mozilla-inbound

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31f8c4465e4
Backed out changeset fe5975fad5d1 for bustage

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9789423a63ab
ExtractLinearSum should remain within the same arithmetic space. r=jandem

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Carsten Book [:Tomcat] from comment #5)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=39715343&repo=mozilla-
> inbound

I added a MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE macro, at the end of the switch case of ExtractMathSpace function, and this seems to work fine on try [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b56338af0ab10cc336298ed3afddb650da95199

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9789423a63ab

Comment 10

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

Nicolas, can we please backport this fuzzblocker back to mozilla-aurora?

Comment 11

[Hannes Verschore [:h4writer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0927c72618ccd19ab86d1a5a2c3e1fb5d243c5e8

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8813303 [details] [diff] [review]
ExtractLinearSum should remain within the same arithmetic space.

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1295130

[User impact if declined]:
- A fuzzers will have harder time to find issues in aurora, since they will hit this bug the whole time.
- Wrong results in very specific computations

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Yes, in nightly for a month already

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
/

[Is the change risky?]:
[Why is the change risky/not risky?]:
The fuzzers haven't complained for a month about this fix and it has been in nightly for a month. Therefore I assume quite safe.

[String changes made/needed]:
/

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8813303 [details] [diff] [review]
ExtractLinearSum should remain within the same arithmetic space.

Should help with fuzzing in aurora, let's uplift it now for 52.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a2a863fff37