1460436 - Assertion failure: ins->input()->type() == MIRType::Double, at js/src/jit/Lowering.cpp:1671 or Assertion failure: ins->type() == MIRType::Int32, at jit/Lowering.cpp:1670

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

for (var j = 0; j < 2; ++j) {
    for (var k = 0; k < 2; ++k) {
        print(Math.sign(Math.PI ? 2 : (function() {})() ? h : l))
    }
}

Backtrace:

#0  0x0000000000839320 in js::jit::LIRGenerator::visitSign (this=0x7ffffffeb880, ins=0x7ffffd4b0ca8) at js/src/jit/Lowering.cpp:1671
#1  0x000000000084e1c2 in js::jit::LIRGenerator::visitInstruction (this=this@entry=0x7ffffffeb880, ins=ins@entry=0x7ffffd4b0ca8) at js/src/jit/Lowering.cpp:5337
#2  0x000000000084e43e in js::jit::LIRGenerator::visitInstruction (ins=0x7ffffd4b0ca8, this=0x7ffffffeb880) at js/src/jit/Lowering.cpp:5335
#3  js::jit::LIRGenerator::visitBlock (this=0x7ffffffeb880, block=0x7ffffd4abe50) at js/src/jit/Lowering.cpp:5423
#4  0x000000000084e73b in js::jit::LIRGenerator::generate (this=this@entry=0x7ffffffeb880) at js/src/jit/Lowering.cpp:5491
/snip

For detailed crash information, see attachment.

Setting s-s as a start because this seems to involve MIR.

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/d37d926c33fa
user:        André Bargull
date:        Tue May 08 05:41:18 2018 -0700
summary:     Bug 1416289 - Part 2: Add Ion-inline support for Math.sign. r=jandem

Andre, is bug 1416289 a likely regressor?

Comment 3

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

$ ./js-64-dm-linux-9294f67b3f3b --fuzzing-safe --no-threads --ion-eager testcase.js
1
0
1
1

$ ./js-64-dm-linux-9294f67b3f3b/js-64-dm-linux-9294f67b3f3b --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js
1
1
1
1

This also causes a differential testing error as found by compare_jit involving 64-bit opt deterministic builds off m-c rev 9294f67b3f3b.

Comment 4

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

function f(y) {
    Math.sign(y) ? n : undefined
}
f();
function g() {};
(function() {
    for (var j = 0; j < 1100; ++j)
        f(!1 ^ 1 ? undefined | 0 : +w())
})();


$ ./js-dbg-64-dm-linux-9294f67b3f3b --fuzzing-safe --no-threads --baseline-eager --ion-check-range-analysis testcase.js 
Assertion failure: ins->type() == MIRType::Int32, at jit/Lowering.cpp:1670
Segmentation fault (core dumped)

This also causes a SIGTRAP for opt builds

Thread 1 "js-64-dm-linux-" received signal SIGTRAP, Trace/breakpoint trap.
0x00003f67857686c0 in ?? ()
(gdb) bt
#0  0x00003f67857686c0 in ?? ()
#1  0x00003f678575f4e2 in ?? ()
#2  0x0000000000002043 in ?? ()
#3  0x00007ffffd39da80 in ?? ()
#4  0x0000000000000000 in ?? ()
(gdb)

Comment 5

[Ted Campbell [:tcampbell]]

The input when creating the MSign is a MPhi with type Int32, and argument types of Int32 and Value. By the time we crash the input is MBox(MConstant(IntValue)) which causes the failure.

It is unclear to me what code we *expect* to reconcile the Phi typeResult.

Comment 6

[Jan de Mooij [:jandem]]

Hm MSign needs to have some sort of type policy other than NoFloatPolicy... MMathFunction uses FloatingPointPolicy, maybe we can do something like that (add SignPolicy that also supports Int32).

anba, forwarding to you, but I can also take it if needed.

Comment 7

[André Bargull [:anba]]

Attachment: bug1460436.patch

Does this type policy make sense for this case? Both test cases no longer crash/assert with these changes, but I still need to run the complete jit-tests test suite.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8974652 [details] [diff] [review]
bug1460436.patch

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

Thanks!

::: js/src/jit/TypePolicy.cpp
@@ +444,5 @@
> +    if (specialization == MIRType::Int32)
> +        return UnboxedInt32Policy<0>::staticAdjustInputs(alloc, ins);
> +
> +    // Otherwise convert input to double.
> +    MOZ_ASSERT(IsFloatingPointType(specialization));

Nit: maybe MOZ_ASSERT(specialization == MIRType::Double)? I don't think Float32 should show up here.

Comment 9

[Daniel Veditz [:dveditz]]

Guessing sec-moderate because it's hard to imagine how confusing one math type for another could get us into too much trouble.

Comment 10

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> ::: js/src/jit/TypePolicy.cpp
> @@ +444,5 @@
> > +    if (specialization == MIRType::Int32)
> > +        return UnboxedInt32Policy<0>::staticAdjustInputs(alloc, ins);
> > +
> > +    // Otherwise convert input to double.
> > +    MOZ_ASSERT(IsFloatingPointType(specialization));
> 
> Nit: maybe MOZ_ASSERT(specialization == MIRType::Double)? I don't think
> Float32 should show up here.

Float32 can show up here:
MCallOptimize accepts any floating point type (and int32) for MSign [1] and directly passes the argument to the MSign constructor [2]. So when this patch reads the argument type in the MSign constructor, we can get either Int32, Float32, or Double. 

[1] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/MCallOptimize.cpp#1532
[2] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/js/src/jit/MIR.h#7047

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2793f77caa2e91f93d84b74f7e067cf0c8b852f5

Comment 12

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 87cc17943894).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2793f77caa2e

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.