Closed
Bug 1247880
Opened 8 years ago
Closed 8 years ago
Differential Testing: Different output message involving Math.imul
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: gkw, Assigned: nbp)
Details
(Keywords: regression, testcase)
Attachments
(1 file)
1.35 KB,
patch
|
sunfish
:
review+
Sylvestre
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
function f(x) { return Math.imul(-4294967295, -(Number ? x | 0 : 0) >>> 0) % 2 >> 0 }; f(objectEmulatingUndefined()); print(f(new Boolean(true))); $ ./js-dbg-64-dm-clang-darwin-576a6dcde5b6 --fuzzing-safe --no-threads --baseline-eager testcase.js -1 $ ./js-dbg-64-dm-clang-darwin-576a6dcde5b6 --fuzzing-safe --no-threads --ion-eager testcase.js 1 Tested this on m-c rev 576a6dcde5b6. 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 576a6dcde5b6 This seems to go back prior to m-c rev 54be5416ae5d. Jan/Hannes, do you mind taking a look and moving this on as necessary?
Flags: needinfo?(jdemooij)
Flags: needinfo?(hv1989)
Comment 1•8 years ago
|
||
Ugh, this works on tip so I bisected it: The first good revision is: changeset: 284067:6cbce3cad8b7 user: Jan de Mooij <jdemooij@mozilla.com> date: Fri Feb 12 08:58:26 2016 +0100 summary: Bug 1246658 part 1 - Refactor MDefinition::constantValue and friends. r=bbouvier I'm not sure why that patch fixed this though. Let me do some digging.
Flags: needinfo?(hv1989)
Comment 2•8 years ago
|
||
OK, in MustBeUInt32 I changed this: return def->constantValue().isInt32() && def->constantValue().toInt32() >= 0; to: return defConst->value().isInt32(0); After fixing that, I can still reproduce the differential bug.
Comment 3•8 years ago
|
||
Below is a slightly simpler testcase. The "% 2" is compiled as LModPowTwoI, then in CodeGeneratorX86Shared::visitModPowTwoI we have: if (!ins->mir()->isUnsigned() && ins->mir()->canBeNegativeDividend()) { This is false (we think the % instruction is unsigned), but the LHS value we get at runtime is signed (-1). The LHS of the % is: Math.imul(1, -(Number ? (x|0) : 0) >>> 0) In MBinaryArithInstruction::foldsTo we replace that MMul with a new MTruncateToInt32 instruction: MTruncateToInt32(MUrsh(.., 0)) But because the MUrsh has type int32, we immediately eliminate the MTruncateToInt32 in MTruncateToInt32::foldsTo. So before the Truncate Doubles phase we have: MMod(MUrsh(MTruncateToInt32(..), 0), 2) The Truncate Doubles phase changes the MMod's LHS to refer to the MURsh's LHS: MMod(MTruncateToInt32(..), 2) So, at this point we have MMod with unsigned_ == true, but the actual operand is no longer MUrsh so it's not unsigned at all anymore. Not sure what the right fix is. Forwarding to nbp, as he's most familiar with the RA/truncation code. Hopefully the analysis above is useful. function f(x) { return (Math.imul(1, -(Number ? (x|0) : 0) >>> 0) % 2) >> 0; }; f(0); assertEq(f(1), -1);
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
Comment 4•8 years ago
|
||
nbp (and gkw), this doesn't repro on m-c tip because of the issue in comment 1 and comment 2. NI myself to land the one-line fix for that next week (the differential bug definitely reproduces at the original revision, though).
Flags: needinfo?(jdemooij)
Comment 5•8 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > NI myself to land the one-line fix for that next week OK, I pushed the fix for the regression I introduced: https://hg.mozilla.org/integration/mozilla-inbound/rev/1493196bb7a0 So this bug should reproduce again on top of that. Sorry for the confusion.
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 6•8 years ago
|
||
Thanks jandem for the analysis. (patch verified with --ion-check-range-analysis) The problem here is that MustBeUint32 was removing MUrsh instruction iff bailouts were enabled, but the MUrsh::computeRange function only set the flag when the range of its input is already prooven to be an unsigned value, after being wrapped to an int32. Thus, the condition was wrong, and this patch just invert the condition, such that we only remove infallible no-op MUrsh(x, 0) by x, only if the operand x is already unsigned. I would recommend to backport this small patch to all branches, even esr branches if possible. (caused by Bug 940864)
Attachment #8723576 -
Flags: review?(sunfish)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(nicolas.b.pierron)
Comment 7•8 years ago
|
||
Comment on attachment 8723576 [details] [diff] [review] Only remove MUrsh operands when the input of MUrsh is guaranteed to be unsigned. Review of attachment 8723576 [details] [diff] [review]: ----------------------------------------------------------------- The patch does actually fix the testcase in bug 940864 in the version that patch was originally applied to, even though the patch is ultimately mistaken. We should understand why this is so before backpatching it to older branches.
Attachment #8723576 -
Flags: review?(sunfish) → review+
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Dan Gohman [:sunfish] from comment #7) > The patch does actually fix the testcase in bug 940864 in the version that > patch was originally applied to, even though the patch is ultimately > mistaken. We should understand why this is so before backpatching it to > older branches. First, I do not see any reasons why the test case was no landed, and so I will do that with this patch. Second, Today, when running the test case with --ion-eager --no-threads, the body of the function is fully inlined under GVN. So, I changed the code to replace the -1 by "x" and give x as argument, in an attempt to make it fail under Ion, and then noticed that we have ((x >>>> 0) | 0) % 10000, and not (no longer) (x >>> 0) % 10000. Thus MMod never had any MUrsh as argument. At the time, the reason might have been that we eagerly removed the MBitOr causing this issue to appear. Thus, I still think that it is better to backport this patch.
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3aa448374082
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Comment 11•8 years ago
|
||
Nicolas, do you mind nominating this for approval‑mozilla‑esr45 as well? It would aid fuzzing on that branch.
Assignee: nobody → nicolas.b.pierron
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8723576 [details] [diff] [review] Only remove MUrsh operands when the input of MUrsh is guaranteed to be unsigned. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Help run differential fuzzing on the esr branch (comment 11). User impact if declined: low, unexpected JS behaviour. Fix Landed on Version: 48 Risk to taking this patch (and alternatives if risky): low. Checked-in & fuzzed since a month. String or UUID changes made by this patch: N/A
Flags: needinfo?(nicolas.b.pierron)
Attachment #8723576 -
Flags: approval-mozilla-esr45?
Updated•8 years ago
|
status-firefox-esr45:
--- → affected
Comment 13•8 years ago
|
||
Comment on attachment 8723576 [details] [diff] [review] Only remove MUrsh operands when the input of MUrsh is guaranteed to be unsigned. Improve the quality of esr, taking it. Should be in 45.1.0
Attachment #8723576 -
Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr45/rev/2fc98c31ee4a
And a quick followup because I messed up resolving a merge conflict when I uplifted this. https://hg.mozilla.org/releases/mozilla-esr45/rev/f65347183228
Comment 16•8 years ago
|
||
Nicolas tells me this doesn't need to uplift. WONTFIX 47.
You need to log in
before you can comment on or make changes to this bug.
Description
•