Closed Bug 776687 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: gkw, Assigned: nbp)

References

Details

(5 keywords, Whiteboard: [fuzzblocker][ion:p1:fx18])

Attachments

(3 files, 1 obsolete file)

Attached file 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.
This and bug 776748 are fuzzblockers - they should be the ones that create lots of dupes.
Whiteboard: [fuzzblocker]
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
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
Whiteboard: [fuzzblocker] → [fuzzblocker][ion:p1:fx18]
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..
Check for aliasing of Phi while eliminating Phis.
Attachment #647381 - Flags: review?(dvander)
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+
(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.
Keywords: sec-high
https://hg.mozilla.org/projects/ionmonkey/rev/7faf3ada4920 (patch)
https://hg.mozilla.org/projects/ionmonkey/rev/2169bca0c9a5 (test case)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Fixed, so it can be opened.
Group: core-security
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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)
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
Attachment #648617 - Flags: review?(dvander) → review+
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+
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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.