Last Comment Bug 691597 - IonMonkey: IonBuilder's jsop_neg() assumes unguaranteed slot availability.
: IonMonkey: IonBuilder's jsop_neg() assumes unguaranteed slot availability.
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- major (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: langfuzz
  Show dependency treegraph
 
Reported: 2011-10-03 17:11 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 07:57 PST (History)
4 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Refactor jsop_neg() to not assume extra slot availability. (2.09 KB, patch)
2011-10-03 18:05 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2011-10-03 17:11:41 PDT
The following testcase asserts on ionmonkey revision acf3c1fb7c94 (run with --ion-eager), tested on 64 bit:


function f() {
    -null;
}
f();
Comment 1 Sean Stangl [:sstangl] 2011-10-03 17:44:11 PDT
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.
Comment 2 Sean Stangl [:sstangl] 2011-10-03 18:05:49 PDT
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.
Comment 3 David Anderson [:dvander] 2011-10-04 13:51:49 PDT
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?
Comment 4 Sean Stangl [:sstangl] 2011-10-04 14:00:45 PDT
(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 5 David Anderson [:dvander] 2011-10-04 14:18:48 PDT
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.
Comment 6 Sean Stangl [:sstangl] 2011-10-05 16:10:53 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/578219c4f056
Comment 7 Christian Holler (:decoder) 2013-01-14 07:57:46 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/ion/bug691597.js.

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