1492685 - Assertion failure: val->type() == MIRType::Int32, at js/src/jit/IonBuilder.cpp:12987

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision ac9f1219d11b (build with --enable-debug, run with --fuzzing-safe --no-threads --ion-eager):

function f(x, ...y) {
    x &= y;
}
f();

Backtrace:

#0  0x000055c975f37a56 in js::jit::IonBuilder::jsop_setarg (this=0x7f7965195220, arg=0) at js/src/jit/IonBuilder.cpp:12987
#1  0x000055c975f2fa66 in js::jit::IonBuilder::inspectOpcode (this=0x7f7965195220, op=JSOP_SETARG) at js/src/jit/IonBuilder.cpp:2040
#2  0x000055c975f2ec95 in js::jit::IonBuilder::visitBlock (this=0x7f7965195220, cfgblock=0x7f79651821c8, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1654
#3  0x000055c975f2b85e in js::jit::IonBuilder::traverseBytecode (this=0x7f7965195220) at js/src/jit/IonBuilder.cpp:1568
#4  0x000055c975f25714 in js::jit::IonBuilder::build (this=0x7f7965195220) at js/src/jit/IonBuilder.cpp:908
#5  0x000055c975f1e72c in js::jit::IonCompile (cx=<optimized out>, osrPc=<optimized out>, optimizationLevel=<optimized out>, script=<optimized out>, baselineFrame=<optimized out>, recompile=<optimized out>) at js/src/jit/Ion.cpp:2136
/snip

For detailed crash information, see attachment.

Setting s-s as a start because MIR is involved.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d5c22661c860
user:        Robin Templeton
date:        Tue Sep 18 04:04:53 2018 +0000
summary:     bug 1490387 - Part 3: Implement BigInt support for bitwise operators. r=jandem

Robin/Jan, is bug 1490387 a likely regressor?

Comment 3

[Jan de Mooij [:jandem]]

Robin, can you look at this?

It might be okay to just remove this whole optimization in IonBuilder::jsop_setarg, it was meant to optimize asm.js-like code.

Also, next time please don't make non-trivial changes to a patch without requesting re-review. For instance MBinaryBitwiseInstruction's constructor now has an unused |type| argument - I would have pointed that out in a review.

Comment 4

[Robin Templeton [:terpri]]

Attachment: Remove obsolete Ion optimization for JSOP_SETARG.

IonBuilder::jsop_setarg optimizes asm.js-style argument coercions
using JSOP_BITAND, JSOP_BITOR, or JSOP_POS. The optimization
assumes that bitwise operations always produce Int32 results, but
they may produce Value results due to BigInt-related changes in
bug 1490387.

Comment 5

[Robin Templeton [:terpri]]

Yes, this looks like a consequence of #1490387. Removing the special case for bitops/mul would be the easiest solution; the optimization won't necessarily be valid once BigInts are available, and I think asm.js coercions are still optimized without it.

I attached a patch that fixes this particular crash, but if it seems at all questionable we should just revert part 3 of #1490387 for now, which would restore the previous definitions for bitwise operations. (And in the future I'll be more careful about requesting re-reviews; clearly I was a bit too optimistic about a seemingly minor change being 'trivial' in the context of Ion :))

Comment 6

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

Comment on attachment 9011049 [details] [diff] [review]
Remove obsolete Ion optimization for JSOP_SETARG.

Jan is the right reviewer for this.

Also, it should be safe to land a test as well since this is nightly-only, so sec-approval should not be required.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9011049 [details] [diff] [review]
Remove obsolete Ion optimization for JSOP_SETARG.

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

Yeah this LGTM, thanks! We can see how it affects benchmarks on AWFY/Perfherder after it lands.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.