Closed Bug 691597 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Unassigned)

Details

(Keywords: assertion, testcase)

Attachments

(1 file)

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.
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)
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
Closed: 13 years ago
Resolution: --- → FIXED
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.