Last Comment Bug 776687 - IonMonkey: Crash [@ js::ion::LIRGeneratorShared::visitConstant] or "Assertion failure: false (unexpected constant type),"
: IonMonkey: Crash [@ js::ion::LIRGeneratorShared::visitConstant] or "Assertion...
Status: VERIFIED FIXED
[fuzzblocker][ion:p1:fx18]
: assertion, crash, regression, sec-high, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- critical (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
: general
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz IonFuzz 735406
  Show dependency treegraph
 
Reported: 2012-07-23 13:26 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-14 08:12 PST (History)
9 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stacks (8.98 KB, text/plain)
2012-07-23 13:26 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
Check that MPhi are not aliasing other definitions. (2.71 KB, patch)
2012-07-30 18:10 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Remove redundant MPhi created by the removal of redundant MPhi. (3.99 KB, patch)
2012-07-31 15:04 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
Re-evaluate definitions in which we replaced the phi. (2.48 KB, patch)
2012-08-02 22:25 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
nicolas.b.pierron: checkin+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2012-07-23 13:26:34 PDT
Created attachment 645044 [details]
stacks

(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.
Comment 1 Gary Kwong [:gkw] [:nth10sd] 2012-07-23 16:21:48 PDT
This and bug 776748 are fuzzblockers - they should be the ones that create lots of dupes.
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2012-07-23 17:12:17 PDT
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)
Comment 3 Nicolas B. Pierron [:nbp] 2012-07-24 18:42:17 PDT
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.
Comment 4 Jan de Mooij [:jandem] 2012-07-30 02:23:17 PDT
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..
Comment 5 Nicolas B. Pierron [:nbp] 2012-07-30 18:10:30 PDT
Created attachment 647381 [details] [diff] [review]
Check that MPhi are not aliasing other definitions.

Check for aliasing of Phi while eliminating Phis.
Comment 6 Nicolas B. Pierron [:nbp] 2012-07-31 15:04:03 PDT
Created attachment 647693 [details] [diff] [review]
Remove redundant MPhi created by the removal of redundant MPhi.

Use the Unused and the InWorklist flags to mark all used Phis while removing the redundant Phi not detected during the first traversal.
Comment 7 David Anderson [:dvander] 2012-07-31 18:16:23 PDT
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?
Comment 8 Nicolas B. Pierron [:nbp] 2012-08-01 10:17:58 PDT
(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.
Comment 10 Christian Holler (:decoder) 2012-08-01 18:21:02 PDT
Fixed, so it can be opened.
Comment 11 Christian Holler (:decoder) 2012-08-01 19:03:16 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 12 Nicolas B. Pierron [:nbp] 2012-08-02 22:25:26 PDT
Created attachment 648617 [details] [diff] [review]
Re-evaluate definitions in which we replaced the phi.

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.
Comment 13 Nicolas B. Pierron [:nbp] 2012-08-02 22:34:00 PDT
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.
Comment 14 Nicolas B. Pierron [:nbp] 2012-08-06 17:13:35 PDT
Comment on attachment 648617 [details] [diff] [review]
Re-evaluate definitions in which we replaced the phi.

https://hg.mozilla.org/projects/ionmonkey/rev/f823c4da0b25
Comment 15 Christian Holler (:decoder) 2012-08-06 18:46:53 PDT
JSBugMon: This bug has been automatically verified fixed.
Comment 16 Christian Holler (:decoder) 2013-01-14 08:12:58 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug776687.js.

Note You need to log in before you can comment on or make changes to this bug.