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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 2 bugs, 5 keywords)

Other Branch
assertion, crash, regression, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fuzzblocker][ion:p1:fx18])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
This and bug 776748 are fuzzblockers - they should be the ones that create lots of dupes.
Whiteboard: [fuzzblocker]
(Reporter)

Comment 2

5 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

5 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
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..
(Assignee)

Comment 5

5 years ago
Created attachment 647381 [details] [diff] [review]
Check that MPhi are not aliasing other definitions.

Check for aliasing of Phi while eliminating Phis.
Attachment #647381 - Flags: review?(dvander)
(Assignee)

Comment 6

5 years ago
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.
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

5 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.
Keywords: sec-high
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/7faf3ada4920 (patch)
https://hg.mozilla.org/projects/ionmonkey/rev/2169bca0c9a5 (test case)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Fixed, so it can be opened.
Group: core-security
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Comment 12

5 years ago
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.
Attachment #648617 - Flags: review?(dvander)
(Assignee)

Updated

5 years ago
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

5 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.
Attachment #648617 - Flags: review?(dvander) → review+
(Assignee)

Comment 14

5 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

5 years ago
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 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.