Note: There are a few cases of duplicates in user autocompletion which are being worked on.

IonMonkey: IonBuilder's jsop_neg() assumes unguaranteed slot availability.

RESOLVED FIXED

Status

()

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

People

(Reporter: decoder, Unassigned)

Tracking

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

Other Branch
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
The following testcase asserts on ionmonkey revision acf3c1fb7c94 (run with --ion-eager), tested on 64 bit:


function f() {
    -null;
}
f();
This one is actually pretty good.

The problem is that jsop_neg() is implemented by pushing MConstant(-1) followed by an MMul, but there is no slot in the MBasicBlock into which the MConstant can be pushed so the MMul can consume it.
Created attachment 564416 [details] [diff] [review]
Refactor jsop_neg() to not assume extra slot availability.

This changes jsop_neg() to not assume that there exists an extra slot which it can use for temporarily pushing MConstant(-1).

An alternate solution is to just give IonBuilder's nslots "just one additional slot for extra temporary padding", but it seems tremendously irritating to detect these errors if we adopt that solution, especially if they nest.
Attachment #564416 - Flags: review?(dvander)

Updated

6 years ago
Summary: IM: Assertion failure: stackPosition_ < gen()->nslots(), at ion/MIRGraph.cpp:314 → IonMonkey: IonBuilder's jsop_neg() assumes unguaranteed slot availability.
Comment on attachment 564416 [details] [diff] [review]
Refactor jsop_neg() to not assume extra slot availability.

What about adding JOF_TMPSLOT to JSOP_NEG in jsopcode.tbl?
(In reply to David Anderson [:dvander] from comment #3)
> What about adding JOF_TMPSLOT to JSOP_NEG in jsopcode.tbl?

It seems dubious to change the opcode to support a quirk of a JIT when it is simple to match that JIT to interpreter behavior.
Comment on attachment 564416 [details] [diff] [review]
Refactor jsop_neg() to not assume extra slot availability.

FWIW, JOF_TMPSLOT exists to handle these quirks. Pre-conservative GC, it allowed ops to decompose in the interpreter while keeping their temporary values rooted. In JM (where it is used frequently) it allows similar decomposition. 

>-IonBuilder::jsop_binary(JSOp op)
>+IonBuilder::jsop_binary_explicit(JSOp op, MDefinition *left, MDefinition *right)

I would just overload jsop_binary, "_explicit" feels weird.
Attachment #564416 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/578219c4f056
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

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