Closed
Bug 776687
Opened 12 years ago
Closed 12 years ago
IonMonkey: Crash [@ js::ion::LIRGeneratorShared::visitConstant] or "Assertion failure: false (unexpected constant type),"
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: gkw, Assigned: nbp)
References
Details
(5 keywords, Whiteboard: [fuzzblocker][ion:p1:fx18])
Attachments
(3 files, 1 obsolete file)
8.98 KB,
text/plain
|
Details | |
3.99 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
2.48 KB,
patch
|
dvander
:
review+
nbp
:
checkin+
|
Details | Diff | Splinter Review |
(function() { for (var a = 0; a < 9; a++) { if (c) { while (arguments[2])(function() {}) } } })() asserts js debug shell on IonMonkey changeset 23a84dbb258f with --ion-eager and -a at Assertion failure: false (unexpected constant type), Looking at the stack it seems to be accessing a weird address 0x13dfe0 so locking s-s.
Reporter | ||
Comment 1•12 years ago
|
||
This and bug 776748 are fuzzblockers - they should be the ones that create lots of dupes.
Whiteboard: [fuzzblocker]
Reporter | ||
Comment 2•12 years ago
|
||
autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 98007:b02a7b214e49 user: Nicolas Pierron date: Fri Jun 15 07:37:06 2012 -0700 summary: Handle arguments[i] (Bug 735406 part 3, r=dvander)
Blocks: 735406
Assignee | ||
Comment 3•12 years ago
|
||
The problem is caused by an MPhi which looks like x MPhi a x where a is the MConstant definition of the magic argument value which has to be captured inside the inner loop and which is created at the beginning. This assertion failure is a substitute to a compilation failure on release builds.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Updated•12 years ago
|
Whiteboard: [fuzzblocker] → [fuzzblocker][ion:p1:fx18]
Comment 4•12 years ago
|
||
Luke has a demo which hits the same problem. Here's a reduced test case FWIW: function f() { while (true) { while (true) { arguments[0]; } } } f(1); (In reply to Nicolas B. Pierron [:pierron] from comment #3) > > This assertion failure is a substitute to a compilation failure on release > builds. No in release builds JS_NOT_REACHED calls abort() so this will crash..
Assignee | ||
Comment 5•12 years ago
|
||
Check for aliasing of Phi while eliminating Phis.
Attachment #647381 -
Flags: review?(dvander)
Assignee | ||
Comment 6•12 years ago
|
||
Use the Unused and the InWorklist flags to mark all used Phis while removing the redundant Phi not detected during the first traversal.
Attachment #647381 -
Attachment is obsolete: true
Attachment #647381 -
Flags: review?(dvander)
Attachment #647693 -
Flags: review?(dvander)
Comment on attachment 647693 [details] [diff] [review] Remove redundant MPhi created by the removal of redundant MPhi. Review of attachment 647693 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonAnalysis.cpp @@ +143,5 @@ > + // The removal of Phis can produce newly redundant phis. > + if (MDefinition *redundant = IsPhiRedundant(phi)) { > + phi->replaceAllUsesWith(redundant); > + if (redundant->isPhi() && !redundant->isUnused()) > + redundant->setUnused(); nit: you can remove the condition and use setUnusedUnchecked(). Is it safe to do this at all because we're guaranteed to enqueue the replacement below? @@ +396,5 @@ > for (ReversePostorderIterator block(graph.rpoBegin()); block != graph.rpoEnd(); block++) { > for (MPhiIterator phi(block->phisBegin()); phi != block->phisEnd();) { > if (phi->type() <= MIRType_Null) { > replaceRedundantPhi(*phi); > + phi->isUnused(); What is this call to phi->isUnused?
Attachment #647693 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #7) > Comment on attachment 647693 [details] [diff] [review] > Remove redundant MPhi created by the removal of redundant MPhi. > > Review of attachment 647693 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/ion/IonAnalysis.cpp > @@ +143,5 @@ > > + // The removal of Phis can produce newly redundant phis. > > + if (MDefinition *redundant = IsPhiRedundant(phi)) { > > + phi->replaceAllUsesWith(redundant); > > + if (redundant->isPhi() && !redundant->isUnused()) > > + redundant->setUnused(); > > nit: you can remove the condition and use setUnusedUnchecked(). Is it safe > to do this at all because we're guaranteed to enqueue the replacement below? Exactly, the following loop will check that we can enqueue the redundant element in the list. > @@ +396,5 @@ > > for (ReversePostorderIterator block(graph.rpoBegin()); block != graph.rpoEnd(); block++) { > > for (MPhiIterator phi(block->phisBegin()); phi != block->phisEnd();) { > > if (phi->type() <= MIRType_Null) { > > replaceRedundantPhi(*phi); > > + phi->isUnused(); > > What is this call to phi->isUnused? I will remove it, I think it was suppose to be phi->setUnused() but apparently it does not matter.
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/7faf3ada4920 (patch) https://hg.mozilla.org/projects/ionmonkey/rev/2169bca0c9a5 (test case)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Comment 11•12 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Assignee | ||
Comment 12•12 years ago
|
||
This is a follow-up patch because the previous one is fixing things a bit by luck because of the order in which Phis are inserted in the queue.
Attachment #648617 -
Flags: review?(dvander)
Assignee | ||
Updated•12 years ago
|
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 13•12 years ago
|
||
The latest patch is failing for the same reason as reported in the first comment, but with the following test case when ran with --ion-eager: var SECTION = "lexical-015"; (function() { A: for (var a = 0; a < 9; a++) { B: for (var b = 0; b < a; b++) { while (SECTION[a - b]) { if (arguments[a - b]) break B; else continue A; } } } })() This Bug concern the removal MPhi with redundant operands, as opposed as Bug 779818 which also solve this issue but by working around the MPhi redundancy detection. A better MPhi redundancy detection will imply adding some graph analysis to detect redundancy at a deeper level than the current implementation, which seems overkill for the current purpose. In the mean time, the follow-up patch remove extra redundant phis and enable the previous test to PASS when Bug 779818 is not applied.
Updated•12 years ago
|
Attachment #648617 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 648617 [details] [diff] [review] Re-evaluate definitions in which we replaced the phi. https://hg.mozilla.org/projects/ionmonkey/rev/f823c4da0b25
Attachment #648617 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Status: RESOLVED → VERIFIED
Comment 15•12 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 16•11 years ago
|
||
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug776687.js.
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•