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

RESOLVED FIXED in Firefox 46

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks 2 bugs, {assertion, regression, testcase})

Trunk
mozilla48
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed, firefox47 fixed, firefox48 fixed, firefox-esr38 affected, firefox-esr45 affected)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(1 attachment)

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.
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?
Flags: needinfo?(nicolas.b.pierron)
(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?
Flags: needinfo?(nicolas.b.pierron)
(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.
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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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
Shu-yu, are any of the bugs listed in the regression window in comment 5 a likely regressor?
Flags: needinfo?(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.
Flags: needinfo?(shu) → needinfo?(nicolas.b.pierron)
Blocks: 807461
Flags: needinfo?(nicolas.b.pierron)
Assignee: nobody → nicolas.b.pierron
Attachment #8727504 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/f7cd029bb2dc
https://hg.mozilla.org/mozilla-central/rev/005625bc9f44
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Attachment #8727504 - Flags: approval-mozilla-release?
Attachment #8727504 - Flags: approval-mozilla-beta?
Attachment #8727504 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #8727504 - Flags: approval-mozilla-release? → approval-mozilla-release-
Comment on attachment 8727504 [details] [diff] [review]
IonBuilder processSwitchEnd returns ControlStatus_Error on allocation errors.

Crash fix, taking it in Aurora47
Attachment #8727504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8727504 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.