JM: "Assertion failure: v.isObject()"

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: dvander)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
(function() { var x = {}; x--; })();

With a 64-bit moo build and -m, I get:

Assertion failure: v.isObject(), at ../jsnum.cpp:1178
(Reporter)

Comment 1

7 years ago
sstangl, dvander said I should cc you on JM64 bugs.
The bug affects 32-bit as well. The value seen by the interpreter upon jsop_localinc() entry is a JSVAL_TYPE_OBJECT, whereas the value seen by methodjit on either platform is a JSVAL_TYPE_UNDEFINED.

On x86_64, the Value.isUndefined() check does a 64-bit comparison, but the payload is nonzero. So the Value is not undefined, and isn't anything else, so it trips an assert.

Note that we optimize away payload stores for known Undefined types -- which breaks this comparison logic. We may have to remove this optimization, or set the payload to zero for known Undefined values before a stub call.

Heading home; will debug further.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 584651
(Assignee)

Updated

7 years ago
Assignee: general → dvander
Status: NEW → ASSIGNED
Summary: JM64: "Assertion failure: v.isObject()" → JM: "Assertion failure: v.isObject()"
(Assignee)

Comment 4

7 years ago
Created attachment 463731 [details] [diff] [review]
fix

LOCALINC used to be right, then at some point we started being fast and eliding stores, and it became very wrong.

Most of the time we decompose INC ops into something like ; GET ; DUP ; ADD ; 1 etc, but we can do much better for locals. Our optimized version was passing in a vp of the local slot, to ::VpInc, without syncing it.

The new patch does all work on the stack.
Attachment #463731 - Flags: review?(sstangl)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 584659
Comment on attachment 463731 [details] [diff] [review]
fix

Looks good. If only we had some way to assert the validity of the second (currently ignored) argument to frame.merge()..
Attachment #463731 - Flags: review?(sstangl) → review+
(Assignee)

Comment 7

7 years ago
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/787e35063545
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.