519244 - Assertion failure: JSVAL_IS_INT(v), at ../jsapi.h:242 - js1_8/extensions/regress-452476.js

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

js1_8/extensions/regress-452476.js
Assertion failure: JSVAL_IS_INT(v), at ../jsapi.h:242

crashes opt.

regression changeset: 33135:0dfaa6e65031 user: David Anderson <[e-mail]> date: Fri Sep 25 18:20:06 2009 -0700 summary: Fix bogus return values with SETELEM without a POP when deep bailing (bug 518492, r=gal)

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

|cs| has to reflect the JSOP_POP, not the JSOP_SETELEM. You can't rebind references though, so I've re-ordered the logic.

Comment 2

[Andreas Gal :gal]

Why fix this in LeaveTree? Can't we make sure we record this right?

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #2)
> Why fix this in LeaveTree? Can't we make sure we record this right?

see bug 518492

The snapshot needs to be taken in post-state (i.e. next opcode), and the interpreter doesn't execute the JSOP_POP because it's coalesced. The problem is either that pendingGuardCondition is kind of a hack, or that JSOP_SETELEM should really be two opcodes (JSOP_SETELEMPOP and JSOP_SETELEM) because coalescing is really irritating.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 408040 [details] [diff] [review]
fix

Nit drive-by, and a comment.

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -6571,30 +6571,31 @@ LeaveTree(InterpState& state, VMSideExit
>             JSOp op = (JSOp) *regs->pc;
>             JS_ASSERT(op == JSOP_CALL || op == JSOP_APPLY || op == JSOP_NEW ||
>                       op == JSOP_GETPROP || op == JSOP_GETTHISPROP || op == JSOP_GETARGPROP ||
>                       op == JSOP_GETLOCALPROP || op == JSOP_LENGTH ||
>                       op == JSOP_GETELEM || op == JSOP_CALLELEM ||
>                       op == JSOP_SETPROP || op == JSOP_SETNAME || op == JSOP_SETMETHOD ||
>                       op == JSOP_SETELEM || op == JSOP_INITELEM ||
>                       op == JSOP_INSTANCEOF);

Blank line here.

>+            /*
>+             * JSOP_SETELEM can be coalesced with a JSOP_POP in the interpeter.
>+             * Since this doesn't re-enter the recorder, the post-state snapshot
>+             * is invalid. Fix it up here.
>+             */
>+            if (op == JSOP_SETELEM && JSOp(*(regs->pc + JSOP_SETELEM_LENGTH) == JSOP_POP)) {

It's more "honest" to do *(ptr + index) but I prefer ptr[index] here, i.e. regs->pc[JSOP_SETELEM_LENGTH], to cut down on the noise. Must be some old Pascal nerve twitching because (* starts a comment.

>+                regs->sp -= js_CodeSpec[JSOP_SETELEM].nuses;
>+                regs->sp += js_CodeSpec[JSOP_SETELEM].ndefs;
>+                regs->pc += JSOP_SETELEM_LENGTH;
>+                op = JSOP_POP;
>+            }

Blank line here too.

>             const JSCodeSpec& cs = js_CodeSpec[op];

To get rid of coalesced op dispatch would mean more ops, and some duplicated code one way or another, which we have today due to coalescing (the smaller trailing op, JSOP_POP or JSOP_IFEQ/IFNE, is coalesced by a macro such as TRY_BRANCH_AFTER_COND).

Let's say the dup. code is a draw. More ops could be done but we need to get rid of the E4X ops (see bug 441416), and it seems strictly more complex to have more ops when coalescing is about as cheap (modulo JIT maintenance :-/).

/be

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

pushed with nits as:

http://hg.mozilla.org/tracemonkey/rev/ee7f090e12ff

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Should block 1.9.2 because bug 518492 landed there.

Comment 7

[Bob Clary [:bc] (inactive)]

this is now asserting on 32bit linux Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at js/src/jsapi.h:204 on mozilla-central. Since the mc->tm merge this morning, I assume it is happening there too.

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ee7f090e12ff

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5524fb36734a

Comment 10

[Samuel Sidler (old account; do not CC)]

These bugs landed after b4 was cut. Moving flag out.