776687 - IonMonkey: Crash [@ js::ion::LIRGeneratorShared::visitConstant] or "Assertion failure: false (unexpected constant type),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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] (NOT official MoCo now)]

This and bug 776748 are fuzzblockers - they should be the ones that create lots of dupes.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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]]

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]]

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]]

Attachment: Check that MPhi are not aliasing other definitions.

Check for aliasing of Phi while eliminating Phis.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: 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] - inactive, e-mail if emergency]

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]]

(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 9

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/7faf3ada4920 (patch)
https://hg.mozilla.org/projects/ionmonkey/rev/2169bca0c9a5 (test case)

Comment 10

[Christian Holler (:decoder)]

Fixed, so it can be opened.

Comment 11

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: 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]]

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]]

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)]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug776687.js.