601398 - "Assertion failure: OBJ_BLOCK_DEPTH(cx, obj) + OBJ_BLOCK_COUNT(cx, obj) <= (size_t) (regs.sp - regs.fp->base()),"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function () {
    try {} finally {
        let(z) {
            return;
        }
    }
})()

asserts js debug shell on TM changeset 0230a9e80c1f without -m or -j at Assertion failure: OBJ_BLOCK_DEPTH(cx, obj) + OBJ_BLOCK_COUNT(cx, obj) <= (size_t) (regs.sp - regs.fp->base()),

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54650:427282865362
user:        Bill McCloskey
date:        Wed Sep 29 13:21:36 2010 -0700
summary:     Bug 535912 - Eliminate blockChain from JSStackFrame (r=cdleary)

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

Here's what happened. The code in js_GetBlockChain is supposed to scan over bytecode, looking for the nearest relevant opcode containing the blockchain. It is important that LEAVEBLOCK opcodes appearing inside "early exits" from blocks should not confuse it. I was using the SRC_HIDDEN source note to avoid such LEAVEBLOCK instructions.

However, this was giving the wrong blockchain for the early exit code itself. So I've made a change that tracks the blockchain changes inside an early exit, but then restores everything after the early exit is over.

Brendan, can you conform that JSOP_GOTO, JSOP_GOTOX, and JSOP_RETRVAL are the only ways that an early exit can end? It's kind of hard to determine this from the parser because I don't understand all the backpatching.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Brendan, can you conform that JSOP_GOTO, JSOP_GOTOX, and JSOP_RETRVAL are the
> only ways that an early exit can end?

And JSOP_RETURN if there is nothing for the early exit code to do. Note how the TOK_RETURN case in js_EmitTree replaces JSOP_RETURN with JSOP_SETRVAL and appends a JSOP_RETRVAL only if extra code was in fact emitted by EmitNonLocalJumpFixup.

IIRC shaver wrote version one of the fixup code, on top of my old (but pre-ES3, so pre-try/catch/finally) backpatching code. mrbkap and igor know this code too. FYI,

/be

Comment 4

[Brendan Eich [:brendan]]

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

I have not been following the blockChain elimination patches, or I'd have raised this issue earlier: source notes are not organized well for hot-path usage during interpretation. They were designed for the decompiler (including the diagnostic generating helpers, e.g. js_DecompileValueGenerator). Some O(n^2) hazards here, although late in the history of source notes (15 years old!) they grew a hashed thread-local lookup cache.

Not sure this is an issue any let-based benchmark would detect. Thoughts?

/be

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to comment #4)
> I have not been following the blockChain elimination patches, or I'd have
> raised this issue earlier: source notes are not organized well for hot-path
> usage during interpretation.

This shouldn't be a problem. The code in question should only be called from a JS debugger or when there's a with block.

Comment 6

[Brendan Eich [:brendan]]

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

Oh, ok -- that'll learn those with lusers ;-). Still wouldn't mind igor weighing in on this.

/be

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Actually, I'm still seeing some problems here. The source notes work for early exits, but I'm not sure they do what I need them to do for catch blocks. Better hold off reviewing this until I get it figured out.

Comment 8

[Igor Bukanov]

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

>From: Bill McCloskey <[e-mail]>
>try: -p linux -m none
>
>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -178,35 +178,48 @@ js_GetBlockChain(JSContext *cx, JSStackF


The proposed changes here are fine, but as a hot fix it is better to just change the assert in JSOP_POPN to replace obj (when obj is not NULL) with obj->parent() and comment there that js_GetBlockChain will return the obj for the current block, not its parent as it ignores hidden LEAVEBLOCK.

A longer term solution is not to rely on the source notes as that can be slow and not reliable as this bug demonstrates.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

This should fix the problem for real. I stopped using source notes, since those weren't doing what I needed. Instead, the patch emits more blockchain opcodes that can then be used by js_GetBlockChain to figure out the current blockchain. This should also improve performance somewhat.

I tested it by asserting in the interpreter, after every instruction, that js_GetBlockChain returns the correct blockchain. It passes trace-tests and jstests with these asserts in place, so it seems fairly solid, at least in that sense.

I've also filed bug 601986 so that we use a fast path everywhere its possible to use one. Hopefully that should address any performance concerns.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Created attachment 480972 [details] [diff] [review]
> fix

I presume the idea here is to add after each goto or return-like bytecodes a special instruction that would tell the bytecode scanner about the current block scope. This is smart since the instruction would never be executed by the interpreter as it comes after the goto and before the target of some other goto.

But from the patch it is not clear why the catch implementation is special in this regard requiring annotation-only bytecode on the execution paths. Could you explain it?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: updated patch

Here's an update to take the blockchain instructions off the runtime path. I doubt there will be a measurable performance effect, given that most code runs in a JIT now, but I guess it can't hurt to do it this way.

Comment 12

[Igor Bukanov]

Comment on attachment 481032 [details] [diff] [review]
updated patch

>From: Bill McCloskey <[e-mail]>
>try: -p linux -m none
>
>diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp
>--- a/js/src/jsemit.cpp
>+++ b/js/src/jsemit.cpp
>@@ -1510,25 +1510,30 @@ EmitNonLocalJumpFixup(JSContext *cx, JSC
>     FLUSH_POPS();
>     cg->stackDepth = depth;
>     return JS_TRUE;
> 
> #undef FLUSH_POPS
> }
> 
> static JSBool
>-EmitBlockChain(JSContext *cx, JSCodeGenerator *cg)
>+EmitKnownBlockChain(JSContext *cx, JSCodeGenerator *cg, JSObjectBox *box)
> {
>-    JSObjectBox *box = cg->blockChainBox;
>     if (box)
>         return EmitIndexOp(cx, JSOP_BLOCKCHAIN, box->index, cg);
>     else
>         return js_Emit1(cx, cg, JSOP_NULLBLOCKCHAIN) >= 0;
> }
> 
>+static JSBool
>+EmitBlockChain(JSContext *cx, JSCodeGenerator *cg)
>+{
>+    return EmitKnownBlockChain(cx, cg, cg->blockChainBox);
>+}
>+
> static ptrdiff_t
> EmitGoto(JSContext *cx, JSCodeGenerator *cg, JSStmtInfo *toStmt,
>          ptrdiff_t *lastp, JSAtomListElement *label, JSSrcNoteType noteType)
> {
>     intN index;
> 
>     if (!EmitNonLocalJumpFixup(cx, cg, toStmt))
>         return -1;
>@@ -1537,17 +1542,24 @@ EmitGoto(JSContext *cx, JSCodeGenerator 
>         index = js_NewSrcNote2(cx, cg, noteType, (ptrdiff_t) ALE_INDEX(label));
>     else if (noteType != SRC_NULL)
>         index = js_NewSrcNote(cx, cg, noteType);
>     else
>         index = 0;
>     if (index < 0)
>         return -1;
> 
>-    return EmitBackPatchOp(cx, cg, JSOP_BACKPATCH, lastp);
>+    ptrdiff_t result = EmitBackPatchOp(cx, cg, JSOP_BACKPATCH, lastp);
>+    if (result < 0)
>+        return result;
>+
>+    if (!EmitBlockChain(cx, cg))
>+        return -1;
>+
>+    return result;
> }
> 
> static JSBool
> BackPatch(JSContext *cx, JSCodeGenerator *cg, ptrdiff_t last,
>           jsbytecode *target, jsbytecode op)
> {
>     jsbytecode *pc, *stop;
>     ptrdiff_t delta, span;
>@@ -5317,26 +5329,27 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>         if (js_NewSrcNote(cx, cg, SRC_HIDDEN) < 0)
>             return JS_FALSE;
>         jmp = EmitBackPatchOp(cx, cg, JSOP_BACKPATCH, &catchJump);
>         if (jmp < 0)
>             return JS_FALSE;
> 
>         tryEnd = CG_OFFSET(cg);
> 
>+        JSObjectBox *prevBox = NULL;
>         /* If this try has a catch block, emit it. */
>         pn2 = pn->pn_kid2;
>         lastCatch = NULL;
>         if (pn2) {
>-            JSObjectBox *prevBox = NULL;
>             uintN count = 0;    /* previous catch block's population */
>             
>             /*
>              * The emitted code for a catch block looks like:
>              *
>+             * blockchain
>              * [throwing]                          only if 2nd+ catch block
>              * [leaveblock]                        only if 2nd+ catch block
>              * enterblock                          with SRC_CATCH
>              * exception
>              * [dup]                               only if catchguard
>              * setlocalpop <slot>                  or destructuring code
>              * [< catchguard code >]               if there's a catchguard
>              * [ifeq <offset to next catch block>]         " "
>@@ -5352,16 +5365,19 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>              * thrown from catch{} blocks.
>              */
>             for (pn3 = pn2->pn_head; pn3; pn3 = pn3->pn_next) {
>                 ptrdiff_t guardJump, catchNote;
> 
>                 JS_ASSERT(cg->stackDepth == depth);
>                 guardJump = GUARDJUMP(stmtInfo);
>                 if (guardJump != -1) {
>+                    if (EmitKnownBlockChain(cx, cg, prevBox) < 0)
>+                        return JS_FALSE;
>+                
>                     /* Fix up and clean up previous catch block. */
>                     CHECK_AND_SET_JUMP_OFFSET_AT(cx, cg, guardJump);
> 
>                     /*
>                      * Account for JSOP_ENTERBLOCK (whose block object count
>                      * is saved below) and pushed exception object that we
>                      * still have after the jumping from the previous guard.
>                      */
>@@ -5436,30 +5452,36 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
> 
>         /*
>          * Last catch guard jumps to the rethrow code sequence if none of the
>          * guards match. Target guardJump at the beginning of the rethrow
>          * sequence, just in case a guard expression throws and leaves the
>          * stack unbalanced.
>          */
>         if (lastCatch && lastCatch->pn_kid2) {
>+            if (EmitKnownBlockChain(cx, cg, prevBox) < 0)
>+                return JS_FALSE;
>+            
>             CHECK_AND_SET_JUMP_OFFSET_AT(cx, cg, GUARDJUMP(stmtInfo));
> 
>             /* Sync the stack to take into account pushed exception. */
>             JS_ASSERT(cg->stackDepth == depth);
>             cg->stackDepth = depth + 1;
> 
>             /*
>              * Rethrow the exception, delegating executing of finally if any
>              * to the exception handler.
>              */
>             if (js_NewSrcNote(cx, cg, SRC_HIDDEN) < 0 ||
>                 js_Emit1(cx, cg, JSOP_THROW) < 0) {
>                 return JS_FALSE;
>             }
>+
>+            if (EmitBlockChain(cx, cg) < 0)
>+                return JS_FALSE;
>         }
> 
>         JS_ASSERT(cg->stackDepth == depth);
> 
>         /* Emit finally handler if any. */
>         finallyStart = 0;   /* to quell GCC uninitialized warnings */
>         if (pn->pn_kid3) {
>             /*
>@@ -5631,16 +5653,18 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>         if (js_Emit1(cx, cg, JSOP_RETURN) < 0)
>             return JS_FALSE;
>         if (!EmitNonLocalJumpFixup(cx, cg, NULL))
>             return JS_FALSE;
>         if (top + JSOP_RETURN_LENGTH != CG_OFFSET(cg)) {
>             CG_BASE(cg)[top] = JSOP_SETRVAL;
>             if (js_Emit1(cx, cg, JSOP_RETRVAL) < 0)
>                 return JS_FALSE;
>+            if (EmitBlockChain(cx, cg) < 0)
>+                return JS_FALSE;
>         }
>         break;
> 
> #if JS_HAS_GENERATORS
>       case TOK_YIELD:
>         if (!cg->inFunction()) {
>             ReportCompileErrorNumber(cx, CG_TS(cg), pn, JSREPORT_ERROR,
>                                      JSMSG_BAD_RETURN_OR_YIELD,
>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -175,38 +175,39 @@ js_GetBlockChain(JSContext *cx, JSStackF
>         return NULL;
> 
>     JSScript *script = fp->script();
>     jsbytecode *start = script->code;
>     /* Assume that imacros don't affect blockChain */
>     jsbytecode *pc = fp->hasImacropc() ? fp->imacropc() : fp->pc(cx);
>     
>     JS_ASSERT(pc >= start && pc < script->code + script->length);
>-    
>-    ptrdiff_t oplen;
>+
>     JSObject *blockChain = NULL;
>-    for (jsbytecode *p = start; p < pc; p += oplen) {
>-        JSOp op = js_GetOpcode(cx, script, p);
>-        const JSCodeSpec *cs = &js_CodeSpec[op];
>-        oplen = cs->length;
>-        if (oplen < 0)
>-            oplen = js_GetVariableBytecodeLength(p);
>-
>-        if (op == JSOP_ENTERBLOCK) { 
>-            blockChain = script->getObject(GET_INDEX(p));
>-        } else if (op == JSOP_LEAVEBLOCK || op == JSOP_LEAVEBLOCKEXPR) {
>-            /*
>-             * Some LEAVEBLOCK instructions are due to early exits via
>-             * return/break/etc. from block-scoped loops and functions.
>-             * We should ignore these instructions, since they don't really
>-             * signal the end of the block.
>-             */
>-            jssrcnote *sn = js_GetSrcNote(script, p);
>-            if (!(sn && SN_TYPE(sn) == SRC_HIDDEN))
>+    if (*pc == JSOP_BLOCKCHAIN) {
>+        blockChain = script->getObject(GET_INDEX(pc));
>+    } else if (*pc == JSOP_NULLBLOCKCHAIN) {
>+        blockChain = NULL;
>+    } else {
>+        ptrdiff_t oplen;
>+        for (jsbytecode *p = start; p < pc; p += oplen) {
>+            JSOp op = js_GetOpcode(cx, script, p);
>+            const JSCodeSpec *cs = &js_CodeSpec[op];
>+            oplen = cs->length;
>+            if (oplen < 0)
>+                oplen = js_GetVariableBytecodeLength(p);
>+            
>+            if (op == JSOP_ENTERBLOCK)
>+                blockChain = script->getObject(GET_INDEX(p));
>+            else if (op == JSOP_LEAVEBLOCK || op == JSOP_LEAVEBLOCKEXPR)
>                 blockChain = blockChain->getParent();
>+            else if (op == JSOP_BLOCKCHAIN)
>+                blockChain = script->getObject(GET_INDEX(p));
>+            else if (op == JSOP_NULLBLOCKCHAIN)
>+                blockChain = NULL;
>         }
>     }
> 
>     return blockChain;
> }
> 
> /*
>  * This function computes the current blockChain, but only in
>@@ -6734,18 +6735,16 @@ END_CASE(JSOP_ARRAYPUSH)
>                  * Restart the handler search with updated pc and stack depth
>                  * to properly notify the debugger.
>                  */
>                 goto error;
>             }
> 
>             switch (tn->kind) {
>               case JSTRY_CATCH:
>-                JS_ASSERT(js_GetOpcode(cx, regs.fp->script(), regs.pc) == JSOP_ENTERBLOCK);
>-
> #if JS_HAS_GENERATORS
>                 /* Catch cannot intercept the closing of a generator. */
>                 if (JS_UNLIKELY(cx->exception.isMagic(JS_GENERATOR_CLOSING)))
>                     break;
> #endif
> 
>                 /*
>                  * Don't clear cx->throwing to save cx->exception from GC
>diff --git a/js/src/trace-test/tests/basic/bug601398.js b/js/src/trace-test/tests/basic/bug601398.js
>new file mode 100644
>--- /dev/null
>+++ b/js/src/trace-test/tests/basic/bug601398.js
>@@ -0,0 +1,7 @@
>+(function () {
>+    try {} finally {
>+        let(z) {
>+            return;
>+        }
>+    }
>+})()
>diff --git a/js/src/trace-test/tests/jaeger/bug563000/trap-self-as-parent.js b/js/src/trace-test/tests/jaeger/bug563000/trap-self-as-parent.js
>--- a/js/src/trace-test/tests/jaeger/bug563000/trap-self-as-parent.js
>+++ b/js/src/trace-test/tests/jaeger/bug563000/trap-self-as-parent.js
>@@ -1,15 +1,15 @@
> setDebug(true);
> x = "notset";
> 
> function myparent(nested) {
>   if (nested) {
>     /* noop call in myparent */
>-    trap(myparent, 48, "success()");
>+    trap(myparent, 49, "success()");
>   } else {
>     myparent(true);
>     x = "failure";
>     noop();
>   }
> }
> function noop() { }
> function success() { x = "success"; }
>diff --git a/js/src/trace-test/tests/jaeger/bug563000/trap-self-from-trap.js b/js/src/trace-test/tests/jaeger/bug563000/trap-self-from-trap.js
>--- a/js/src/trace-test/tests/jaeger/bug563000/trap-self-from-trap.js
>+++ b/js/src/trace-test/tests/jaeger/bug563000/trap-self-from-trap.js
>@@ -8,16 +8,16 @@ function myparent(nested) {
>     /* JSOP_CALL to doNothing in myparent with nested = true. */
>     trap(myparent, 24, "success()");
>     doNothing();
>   } else {
>     doNothing();
>   }
> }
> /* JSOP_CALL to doNothing in myparent with nested = false. */
>-trap(myparent, 34, "myparent(true)");
>+trap(myparent, 35, "myparent(true)");
> 
> function success() {
>   x = "success";
> }
> 
> myparent(false);
> assertEq(x, "success");

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Comment on attachment 481032 [details] [diff] [review]

Sorry for not removing the patch itself from the comment before hitting the submit button!

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 15

[Robert Sayre]

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

Comment 16

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug601398.js.