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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on ionmonkey revision de23a9fc29db (run with --ion -n -m --ion-eager):


function fannkuch() {
    for (var j = 0; j < 50; j++) {
        for (var i = 0; i < 0; i++) {
            arguments, XML;
        }
    }
}
fannkuch();

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: LConstant can produce MIRType_ArgObj values.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 634340 [details] [diff] [review]
LConstant can produce MIRType_ArgObj values.

Review of attachment 634340 [details] [diff] [review]:
-----------------------------------------------------------------

What are your thoughts on removing MIRType_ArgObj, and just pushing an MBox(MConstant(MagicValue(...))? The patch looks find but I think we could remove a lot of code doing this instead.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #2)
> Comment on attachment 634340 [details] [diff] [review]
> LConstant can produce MIRType_ArgObj values.
> 
> Review of attachment 634340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What are your thoughts on removing MIRType_ArgObj, and just pushing an
> MBox(MConstant(MagicValue(...))? The patch looks find but I think we could
> remove a lot of code doing this instead.

The reason why I added this MIRType_ArgObj was to be able to encode the Magic value in the snapshot. without this, there is no way get it back.  The only option we have with the OSR is to replace the MUnbox by an MConstant when the other inputs are containing a lazy arguments, and to make sure that the MPhi are working correctly when all inputs are the same constants.  Currently the easiest/cleanest one to handle is the MIRType_ArgObj because it does not imply to hack the OSR, only to add a new entry in a few switch cases.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Nicolas B. Pierron [:pierron] from comment #3)
> (In reply to David Anderson [:dvander] from comment #2)
> > Comment on attachment 634340 [details] [diff] [review]
> > LConstant can produce MIRType_ArgObj values.
> > 
> > Review of attachment 634340 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > What are your thoughts on removing MIRType_ArgObj, and just pushing an
> > MBox(MConstant(MagicValue(...))? The patch looks find but I think we could
> > remove a lot of code doing this instead.
> 
> The reason why I added this MIRType_ArgObj was to be able to encode the
> Magic value in the snapshot. without this, there is no way get it back.  The
> only option we have with the OSR is to replace the MUnbox by an MConstant
> when the other inputs are containing a lazy arguments, and to make sure that
> the MPhi are working correctly when all inputs are the same constants. 
> Currently the easiest/cleanest one to handle is the MIRType_ArgObj because
> it does not imply to hack the OSR, only to add a new entry in a few switch
> cases.

Hrm, I don't fully understand this. Nothing actually depends on the magic value. In theory we can leave it boxed and a Magic value will never escape into snapshots. And even if it does, it's pretty easy to change snapshot encoding to allow magic values.

Comment 5

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

The assert in comment 0 has mutated.

Another testcase:

this.toString = function() {
  for (p in 0) {
    for (v in 0) {
      arguments[i]
    }
  }
};
this + ""

(tested on IonMonkey changeset rev 796016ef829d with --ion-eager on Linux 64-bit debug and opt shell)

Comment 6

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/f93f38a664f0

Comment 7

[Christian Holler (:decoder)]

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