1266768 - Assertion failure: ins->index()->type() == MIRType_Int32, at js/src/jit/x86-shared/Lowering-x86-shared.cpp:533

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision ae7413abfa4d (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --no-threads):

function mod(stdlib) {
    add = stdlib.Atomics.add;
    function f3()
        add(i8a, 0 | 0 && this && BUGNUMBER, 0);
    return {
        f3: f3,
        0: 0
    }
}
i8a = new Int8Array(new SharedArrayBuffer(1))
var {
    f3
} = mod(this, {})
for (i = 0; i < 1000; i++)
    f3();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000899312 in js::jit::LIRGeneratorX86Shared::lowerAtomicTypedArrayElementBinop (this=<optimized out>, ins=0x7ffff699dfd8, useI386ByteRegisters=<optimized out>) at js/src/jit/x86-shared/Lowering-x86-shared.cpp:533
#0  0x0000000000899312 in js::jit::LIRGeneratorX86Shared::lowerAtomicTypedArrayElementBinop (this=<optimized out>, ins=0x7ffff699dfd8, useI386ByteRegisters=<optimized out>) at js/src/jit/x86-shared/Lowering-x86-shared.cpp:533
#1  0x0000000000747f84 in js::jit::LIRGenerator::visitInstruction (this=0x7fffffffb1c0, ins=0x7ffff699dfd8) at js/src/jit/Lowering.cpp:4670
#2  0x000000000074829b in js::jit::LIRGenerator::visitBlock (this=0x7fffffffb1c0, block=0x7ffff699d4a0) at js/src/jit/Lowering.cpp:4747
#3  0x0000000000748683 in js::jit::LIRGenerator::generate (this=this@entry=0x7fffffffb1c0) at js/src/jit/Lowering.cpp:4796
#4  0x00000000006a6742 in js::jit::GenerateLIR (mir=mir@entry=0x7ffff699a270) at js/src/jit/Ion.cpp:1870
#5  0x00000000006aa40b in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff699a270) at js/src/jit/Ion.cpp:1965
#6  0x00000000006ab203 in js::jit::IonCompile (cx=cx@entry=0x7ffff6908800, script=script@entry=0x7ffff7e733d0, baselineFrame=baselineFrame@entry=0x7fffffffc588, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::Normal) at js/src/jit/Ion.cpp:2229
#7  0x00000000006ab98c in js::jit::Compile (cx=cx@entry=0x7ffff6908800, script=..., script@entry=..., osrFrame=osrFrame@entry=0x7fffffffc588, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2396
#8  0x00000000006ac2a6 in BaselineCanEnterAtEntry (frame=0x7fffffffc588, script=..., cx=0x7ffff6908800) at js/src/jit/Ion.cpp:2520
#9  js::jit::IonCompileScriptForBaseline (cx=0x7ffff6908800, frame=0x7fffffffc588, pc=<optimized out>) at js/src/jit/Ion.cpp:2644
#10 0x00007ffff7feb8c5 in ?? ()
[...]
#32 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff699dfd8	140737330667480
rcx	0x7ffff6ca5870	140737333844080
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffb090	140737488334992
rsp	0x7fffffffb010	140737488334864
r8	0x7ffff7fdf7c0	140737354004416
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffadd0	140737488334288
r11	0x7ffff6c27ee0	140737333329632
r12	0x7fffffffb1c0	140737488335296
r13	0x7ffff699a000	140737330651136
r14	0x7ffff699dfd8	140737330667480
r15	0x7ffff699e028	140737330667560
rip	0x899312 <js::jit::LIRGeneratorX86Shared::lowerAtomicTypedArrayElementBinop(js::jit::MAtomicTypedArrayElementBinop*, bool)+1714>
=> 0x899312 <js::jit::LIRGeneratorX86Shared::lowerAtomicTypedArrayElementBinop(js::jit::MAtomicTypedArrayElementBinop*, bool)+1714>:	movl   $0x215,0x0
   0x89931d <js::jit::LIRGeneratorX86Shared::lowerAtomicTypedArrayElementBinop(js::jit::MAtomicTypedArrayElementBinop*, bool)+1725>:	callq  0x4abcb0 <abort()>


Marking s-s because this is a type assertion that has been associated with security problems in previous bugs.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160107075925" and the hash "edc425e91206dcc79cfd12c4b21ad1bd3dbd8225".
The "bad" changeset has the timestamp "20160107080225" and the hash "70094106841ccb97143d59df7769c5ead0336832".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=edc425e91206dcc79cfd12c4b21ad1bd3dbd8225&tochange=70094106841ccb97143d59df7769c5ead0336832

Comment 2

[Jan de Mooij [:jandem]]

Not sure about that regression window, we're asserting in lowerAtomicTypedArrayElementBinop.

Comment 3

[Lars T Hansen [:lth]]

Agree that regression window is not very plausible.  Something weird is going on here.  MCallOptimize should only inline if the index expression has type int, here it arguably has type something else, not sure what that would be, the expr parenthesizes as (0 | 0) && this && BUGNUMBER, which should always be false.  Will take a further look on Monday.

Comment 4

[Lars T Hansen [:lth]]

At the time the function is inlined the type of the index operand is indeed int32.  The "index" node is an MBoundsCheck, according to lldb.  But when we get to the code generator another node is in the index slot of that MIR node, and it is now an MBox.

The replacing appears to happen in TryEliminateBoundsCheck() in IonAnalysis.cpp where a comment claims that it is "safe".  However it violates an invariant that the lowering pass expects holds because it weakens the type of the node.  Superficially, though, that code has not changed in a long time.

Not sure what to do here.  Jan, is it wrong for lowering to assume that the type sets that were computed by MCallOptimize will hold at lowering time?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #4)
> The replacing appears to happen in TryEliminateBoundsCheck() in
> IonAnalysis.cpp where a comment claims that it is "safe".

Interesting, I think the bug is that MBoundsCheck needs a MIRType::Int32 input, not something like a MIRType::Value from MBox. MBoundsCheck doesn't have a type policy; it probably needs one.

Let me check why we're seeing this only now - MBoundsCheck is ancient code.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Let me check why we're seeing this only now

I think it's because other places add MToInt32 right before the MBoundsCheck, so until now that use was never turned into an MBox by type/phi analysis.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Giving MBoundsCheck a type policy fixes it.

I think this does not affect pre-atomics/SAB code, but I'll take a closer look tomorrow.

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 8745466 [details] [diff] [review]
Patch

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

This seems plausible to me, given how much code already inserts the ToInt32 node (though I'm really not competent to say if that's a global invariant).

Comment 9

[Jan de Mooij [:jandem]]

I think this requires Atomics to be enabled, but I'll take a closer look tomorrow.

Comment 10

[Jan de Mooij [:jandem]]

Atomics-only so I pushed this:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f805f77dc07

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f805f77dc07