The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks: 3 bugs, {assertion, testcase})

Other Branch
x86
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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();
(Assignee)

Updated

5 years ago
Assignee: general → nicolas.b.pierron
(Assignee)

Comment 1

5 years ago
Created attachment 634340 [details] [diff] [review]
LConstant can produce MIRType_ArgObj values.
Attachment #634340 - Flags: review?(dvander)
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.
(Assignee)

Comment 3

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

Updated

5 years ago
Blocks: 735402
(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.
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)
Summary: IonMonkey: Assertion failure: unexpected constant type, at ion/shared/Lowering-shared.cpp:69 → IonMonkey: Crash [@ LInstruction] or [@ js::ion::LIRGeneratorShared::visitConstant] or "Assertion failure: false (unexpected constant type),"
Crash Signature: [@ LInstruction] [@ js::ion::LIRGeneratorShared::visitConstant]
Hardware: x86 → All
Blocks: 349611
Attachment #634340 - Flags: review?(dvander) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/f93f38a664f0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Hardware: All → x86
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 771460
(Reporter)

Comment 7

4 years ago
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug765480.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.