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)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox-esr45 --- fixed

People

(Reporter: gkw, Assigned: nbp)

Details

(Keywords: regression, testcase)

Attachments

(1 file)

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)
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)
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.
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)
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)
(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)
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)
Flags: needinfo?(nicolas.b.pierron)
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/3aa448374082
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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)
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?
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+
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
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.

Attachment

General

Created:
Updated:
Size: