Last Comment Bug 765480 - IonMonkey: Crash [@ LInstruction] or [@ js::ion::LIRGeneratorShared::visitConstant] or "Assertion failure: false (unexpected constant type),"
: IonMonkey: Crash [@ LInstruction] or [@ js::ion::LIRGeneratorShared::visitCon...
Status: RESOLVED FIXED
[jsbugmon:update]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Linux
: -- major (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on:
Blocks: jsfunfuzz langfuzz IonFuzz 735402 771460
  Show dependency treegraph
 
Reported: 2012-06-16 05:12 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 08:40 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
LConstant can produce MIRType_ArgObj values. (1.56 KB, patch)
2012-06-19 03:51 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2012-06-16 05:12:28 PDT
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] 2012-06-19 03:51:39 PDT
Created attachment 634340 [details] [diff] [review]
LConstant can produce MIRType_ArgObj values.
Comment 2 David Anderson [:dvander] 2012-06-19 11:36:06 PDT
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] 2012-06-20 05:32:37 PDT
(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] 2012-06-22 15:45:12 PDT
(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] 2012-06-29 18:16:57 PDT
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] 2012-07-06 03:53:26 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/f93f38a664f0
Comment 7 Christian Holler (:decoder) 2013-01-14 08:40:45 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug765480.js.

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