Closed
Bug 1240521
Opened 7 years ago
Closed 7 years ago
Assertion failure: hasLastIns(), at js/src/jit/MIRGraph.h:375 with OOM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: decoder, Assigned: nbp)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])
Attachments
(1 file)
1.55 KB,
patch
|
h4writer
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
(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)
Comment 3•7 years ago
|
||
(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.
Assignee | ||
Comment 4•7 years ago
|
||
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.
Updated•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 5•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8727504 -
Flags: review?(hv1989)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → nicolas.b.pierron
Updated•7 years ago
|
Attachment #8727504 -
Flags: review?(hv1989) → review+
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f7cd029bb2dc https://hg.mozilla.org/mozilla-central/rev/005625bc9f44
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 12•7 years ago
|
||
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?
Assignee | ||
Updated•7 years ago
|
status-firefox45:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr38:
--- → affected
status-firefox-esr45:
--- → affected
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)
Assignee | ||
Comment 14•7 years ago
|
||
(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 15•7 years ago
|
||
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 17•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/795e561af9a4 https://hg.mozilla.org/releases/mozilla-aurora/rev/f8832c899938
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+
Comment 19•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/16ff1874e87d https://hg.mozilla.org/releases/mozilla-beta/rev/b353fedffc7b
You need to log in
before you can comment on or make changes to this bug.
Description
•