1240521 - Assertion failure: hasLastIns(), at js/src/jit/MIRGraph.h:375 with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

var egc = 138;
function SwitchTest(value) {
    switch (value) {
        case 0:
            break
        case new Number:
            result = 8
        case oomAfterAllocations(egc):
    }
}
!(SwitchTest(4) === 4);
!(SwitchTest(true) === 2);



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x000000000043b24c in js::jit::MBasicBlock::lastIns (this=0x7ffff69986a8) at js/src/jit/MIRGraph.h:375
#0  0x000000000043b24c in js::jit::MBasicBlock::lastIns (this=0x7ffff69986a8) at js/src/jit/MIRGraph.h:375
#1  0x00000000006c4a98 in js::jit::MBasicBlock::lastIns (this=0x7ffff69986a8) at js/src/jit/MIRGraph.h:372
#2  0x0000000000676d88 in more (this=<synthetic pointer>) at js/src/jit/MIRGraph.h:889
#3  operator bool (this=<synthetic pointer>) at js/src/jit/MIRGraph.h:915
#4  js::jit::MakeMRegExpHoistable (graph=...) at js/src/jit/IonAnalysis.cpp:1828
#5  0x00000000006bcca5 in js::jit::OptimizeMIR (mir=mir@entry=0x7ffff69971c0) at js/src/jit/Ion.cpp:1531
#6  0x00000000006be7b9 in js::jit::CompileBackEnd (mir=mir@entry=0x7ffff69971c0) at js/src/jit/Ion.cpp:2011
#7  0x00000000006c1242 in js::jit::IonCompile (cx=cx@entry=0x7ffff6907800, script=script@entry=0x7ffff7e6e230, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=optimizationLevel@entry=js::jit::Normal) at js/src/jit/Ion.cpp:2282
#8  0x00000000006c19cc in js::jit::Compile (cx=cx@entry=0x7ffff6907800, script=..., script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2452
#9  0x00000000006c1e4b in js::jit::CanEnter (cx=cx@entry=0x7ffff6907800, state=...) at js/src/jit/Ion.cpp:2614
#10 0x0000000000a4b6a1 in js::RunScript (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:404
#11 0x0000000000a4b97c in js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:499
#12 0x0000000000a4d339 in js::Invoke (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., argc=argc@entry=1, argv=argv@entry=0x7fffffffce18, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:533
#13 0x00000000006017de in js::jit::DoCallFallback (cx=0x7ffff6907800, frame=0x7fffffffce58, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffce08, res=...) at js/src/jit/BaselineIC.cpp:6186
#14 0x00007ffff7ff1a1f in ?? ()
#15 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69986a8	140737330644648
rcx	0x7ffff6ca53b0	140737333842864
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffc010	140737488338960
rsp	0x7fffffffc010	140737488338960
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffbdd0	140737488338384
r11	0x7ffff6c27960	140737333328224
r12	0x7ffff6997040	140737330638912
r13	0x7ffff6998718	140737330644760
r14	0x7fffffffc570	140737488340336
r15	0x7ffff6998680	140737330644608
rip	0x43b24c <js::jit::MBasicBlock::lastIns() const+28>
=> 0x43b24c <js::jit::MBasicBlock::lastIns() const+28>:	movl   $0x177,0x0
   0x43b257 <js::jit::MBasicBlock::lastIns() const+39>:	callq  0x4a2e10 <abort()>


I wasn't able to get this test using oomTest, so it would be good to tackle it while it's still working. If it stops reproducing, you can try using a for loop passing the egc parameter on the command line rather than in the script, to figure out the right value.

Comment 1

[Benjamin Bouvier [:bbouvier]]

Could reproduce. This happens because the block has no instructions, and we're not testing hasLastIns() in MDefinitionIterator::more().

Nicolas, does this sound like the proper fix here, to take oom into account?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Could reproduce. This happens because the block has no instructions, and
> we're not testing hasLastIns() in MDefinitionIterator::more().
> 
> Nicolas, does this sound like the proper fix here, to take oom into account?

Having no instructions inside basic block is an error, all basic block should at least end with a control instruction as last instruction. If they do not, then they should not be part of the graph.

Also, why do we run this phase before the "Build SSA" spew in OptimizeMIR?

Comment 3

[Benjamin Bouvier [:bbouvier]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> > Could reproduce. This happens because the block has no instructions, and
> > we're not testing hasLastIns() in MDefinitionIterator::more().
> > 
> > Nicolas, does this sound like the proper fix here, to take oom into account?
> 
> Having no instructions inside basic block is an error, all basic block
> should at least end with a control instruction as last instruction. If they
> do not, then they should not be part of the graph.

Yes, that's exactly what happens here, as we have an OOM. A block doesn't get its last instruction because of the oom.

Comment 4

[Nicolas B. Pierron [:nbp]]

When does the OOM happen? MakeMRegExpHoistable is wrongly at the beginning of the compiler, but even with this logical error we should fail in IonBuilder.

Comment 5

[Fuzzing Team]

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

The "good" changeset has the timestamp "20151012223555" and the hash "d4ab193bab01505fe7a49e90aa45579abb279334".
The "bad" changeset has the timestamp "20151012230455" and the hash "88fa17cdbb62756da1b0133b19b74d085bd3e97d".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d4ab193bab01505fe7a49e90aa45579abb279334&tochange=88fa17cdbb62756da1b0133b19b74d085bd3e97d

Comment 6

[Gary Kwong [:gkw] [:nth10sd]]

Shu-yu, are any of the bugs listed in the regression window in comment 5 a likely regressor?

Comment 7

[Shu-yu Guo [:shu]]

I think I only perturbed the allocation patterns or something. I don't know anything about MakeMRegExpHoistable to say what's the right fix here. Flipping NI to nbp.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: IonBuilder processSwitchEnd returns ControlStatus_Error on allocation errors.

Comment 9

[Pulsebot]

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

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/005625bc9f44

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/f7cd029bb2dc
https://hg.mozilla.org/mozilla-central/rev/005625bc9f44

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8727504 [details] [diff] [review]
IonBuilder processSwitchEnd returns ControlStatus_Error on allocation errors.

Approval Request Comment
[Feature/regressing bug #]: Bug 807461
[User impact if declined]: https://crash-stats.mozilla.com/signature/?signature=js%3A%3Ajit%3A%3AMakeMRegExpHoistable#graphs
[Describe test coverage new/current, TreeHerder]: on central.
[Risks and why]: low, because this does not continue compilations which used to be scheduled incorrectly.
[String/UUID change made/needed]: N/A


Note: Cherry-pick the current patch's test case is missing an opt-build guard.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Nicolas, test-only fixes do not need uplift request to land in Aurora and Beta. I looked at the patch and it looks like it's a test fix. Is that right?

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Nicolas, test-only fixes do not need uplift request to land in Aurora and
> Beta. I looked at the patch and it looks like it's a test fix. Is that right?

Both patches have to be backported.  The second is fixing the test which is added by the first one.  Folding them would be fine too, for which I requested the uplift approval.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8727504 [details] [diff] [review]
IonBuilder processSwitchEnd returns ControlStatus_Error on allocation errors.

Not a new crash, not taking it in a 45 dot releases.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727504 [details] [diff] [review]
IonBuilder processSwitchEnd returns ControlStatus_Error on allocation errors.

Crash fix, taking it in Aurora47

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/795e561af9a4
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8832c899938

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8727504 [details] [diff] [review]
IonBuilder processSwitchEnd returns ControlStatus_Error on allocation errors.

Crash fix, we want this for beta as well. 
This may make it into beta 2.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/16ff1874e87d
https://hg.mozilla.org/releases/mozilla-beta/rev/b353fedffc7b