Closed Bug 1319242 Opened 3 years ago Closed 3 years ago

Differential Testing: Different output message involving addition

Categories

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

x86_64
All
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase, Whiteboard: [fuzzblocker])

Attachments

(1 file)

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.
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(jdemooij)
Assignee: nobody → nicolas.b.pierron
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(jdemooij)
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)
Attachment #8813303 - Flags: review?(jdemooij)
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
Attachment #8813303 - Flags: review?(jdemooij) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5975fad5d1
ExtractLinearSum should remain within the same arithmetic space. r=jandem
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=39715343&repo=mozilla-inbound
Flags: needinfo?(nicolas.b.pierron)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31f8c4465e4
Backed out changeset fe5975fad5d1 for bustage
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9789423a63ab
ExtractLinearSum should remain within the same arithmetic space. r=jandem
(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
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/9789423a63ab
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Nicolas, can we please backport this fuzzblocker back to mozilla-aurora?
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(hv1989)
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]:
/
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(hv1989)
Attachment #8813303 - Flags: approval-mozilla-aurora?
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.
Attachment #8813303 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.