470300 - "Assertion failure: StackBase(fp) + blockDepth == regs.sp" with |let|

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> for (let a = 0; a < 3; ++a) { let b = '' + []; }

Assertion failure: StackBase(fp) + blockDepth == regs.sp, at ../jsinterp.cpp:6853

js> for (let a = 0; a < 7; ++a) { let e = 8; if (a > 3) { let x; } }

Assertion failure: !fp->blockChain || OBJ_GET_PARENT(cx, obj) == fp->blockChain, at ../jsinterp.cpp:6814

I'm hitting these assertions frequently, and they seem to be related to each other.  Could this be a regression from bug 469927?

Comment 1

[Brendan Eich [:brendan]]

We are failing to update fp->blockChain on side-exit from trace. I will fix.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix, may need optimization obfuscation

I can squeeze the block's object table index into existing FrameInfo space by shrinking argc, but this is the simplest patch. If it doesn't noticeably slow anything down, I'll stop here.

/be

Comment 4

[Brendan Eich [:brendan]]

I'm probably going to check this in tonight, anticipating Andreas's r+ (he has been in on the design of the patch).

/be

Comment 5

[Andreas Gal :gal]

Comment on attachment 353759 [details] [diff] [review]
fix, may need optimization obfuscation

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -1949,16 +1949,17 @@ TraceRecorder::snapshot(ExitType exitTyp
>     exit->calldepth = callDepth;
>     exit->numGlobalSlots = ngslots;
>     exit->numStackSlots = stackSlots;
>     exit->numStackSlotsBelowCurrentFrame = cx->fp->callee
>         ? nativeStackOffset(&cx->fp->argv[-2])/sizeof(double)
>         : 0;
>     exit->exitType = exitType;
>     exit->addGuard(rec);
>+    exit->block = fp->blockChain;
>     exit->ip_adj = ip_adj;
>     exit->sp_adj = (stackSlots * sizeof(double)) - treeInfo->nativeStackBase;
>     exit->rp_adj = exit->calldepth * sizeof(FrameInfo);
>     memcpy(getTypeMap(exit), typemap, typemap_size);
> 
>     /* BIG FAT WARNING: If compilation fails, we currently don't reset the lirbuf so its safe
>        to keep references to the side exits here. If we ever start rewinding those lirbufs,
>        we have to make sure we purge the side exits that then no longer will be in valid
>@@ -2854,16 +2855,25 @@ js_SynthesizeFrame(JSContext* cx, const 
>     newifp->frame.argc = argc;
> 
>     jsbytecode* imacro_pc = FI_IMACRO_PC(fi, cx->fp);
>     jsbytecode* script_pc = FI_SCRIPT_PC(fi, cx->fp);
>     newifp->callerRegs.pc = imacro_pc ? imacro_pc : script_pc;
>     newifp->callerRegs.sp = cx->fp->slots + fi.s.spdist;
>     cx->fp->imacpc = imacro_pc ? script_pc : NULL;
> 
>+    JS_ASSERT(!(cx->fp->flags & JSFRAME_POP_BLOCKS));
>+    if (fi.block != cx->fp->blockChain) {
>+#ifdef DEBUG
>+        for (JSObject* obj = fi.block; obj != cx->fp->blockChain; obj = STOBJ_GET_PARENT(obj))
>+            JS_ASSERT(obj);
>+#endif
>+        cx->fp->blockChain = fi.block;

What is this piece of code trying to achieve here? Should this be just cx->fp->blockChain = fi.block? The
debug code can have the if it needs it. Also, I suggest extracting fp here to save the indirection. This
path is very hot. Also further up where we keep reading cx->fp.

>+    }
>+
>     newifp->frame.argv = newifp->callerRegs.sp - argc;
>     JS_ASSERT(newifp->frame.argv);
> #ifdef DEBUG
>     // Initialize argv[-1] to a known-bogus value so we'll catch it if
>     // someone forgets to initialize it later.
>     newifp->frame.argv[-1] = JSVAL_HOLE;
> #endif
>     JS_ASSERT(newifp->frame.argv >= StackBase(cx->fp) + 2);
>@@ -3648,27 +3658,30 @@ js_ExecuteTree(JSContext* cx, Fragment* 
>         debug_only_v(printf("synthesized shallow frame for %s:%u@%u\n",
>                             fp->script->filename, js_FramePCToLineNumber(cx, fp),
>                             FramePCOffset(fp));)
> #endif        
>     }
> 
>     /* Adjust sp and pc relative to the tree we exited from (not the tree we entered into).
>        These are our final values for sp and pc since js_SynthesizeFrame has already taken
>-       care of all frames in between. */
>-    JSStackFrame* fp = cx->fp;
>+       care of all frames in between. But first we recover fp->blockChain, which comes from
>+       the side exit struct. */
>+    JSStackFrame* fp = cx->fp;
>+
>+    JS_ASSERT(!(fp->flags & JSFRAME_POP_BLOCKS));
>+    fp->blockChain = innermost->block;
> 
>     /* If we are not exiting from an inlined frame the state->sp is spbase, otherwise spbase
>        is whatever slots frames around us consume. */
>     DECODE_IP_ADJ(innermost->ip_adj, fp);
>     fp->regs->sp = StackBase(fp) + (innermost->sp_adj / sizeof(double)) - calldepth_slots;
>     JS_ASSERT_IF(!fp->imacpc,
>                  fp->slots + fp->script->nfixed +
>                  js_ReconstructStackDepth(cx, fp->script, fp->regs->pc) == fp->regs->sp);
>-                                              
> 
> #if defined(JS_JIT_SPEW) && (defined(NANOJIT_IA32) || (defined(NANOJIT_AMD64) && defined(__GNUC__)))
>     uint64 cycles = rdtsc() - start;
> #elif defined(JS_JIT_SPEW)
>     uint64 cycles = 0;
> #endif
> 
>     debug_only_v(printf("leaving trace at %s:%u@%u, op=%s, lr=%p, exitType=%d, sp=%d, "
>@@ -6580,33 +6593,37 @@ TraceRecorder::interpretedFunctionCall(j
>         *m++ = determineSlotType(vp);
>     );
> 
>     if (argc >= 0x8000)
>         ABORT_TRACE("too many arguments");
> 
>     FrameInfo fi = {
>         JSVAL_TO_OBJECT(fval),
>+        fp->blockChain,
>         ENCODE_IP_ADJ(fp, fp->regs->pc),
>         typemap,
>         { { fp->regs->sp - fp->slots, argc | (constructing ? 0x8000 : 0) } }
>     };
> 
>     unsigned callDepth = getCallDepth();
>     if (callDepth >= treeInfo->maxCallDepth)
>         treeInfo->maxCallDepth = callDepth + 1;
> 
>-    lir->insStorei(INS_CONSTPTR(fi.callee), lirbuf->rp,
>-                   callDepth * sizeof(FrameInfo) + offsetof(FrameInfo, callee));
>-    lir->insStorei(INS_CONSTPTR(fi.ip_adj), lirbuf->rp,
>-                   callDepth * sizeof(FrameInfo) + offsetof(FrameInfo, ip_adj));
>-    lir->insStorei(INS_CONSTPTR(fi.typemap), lirbuf->rp,
>-                   callDepth * sizeof(FrameInfo) + offsetof(FrameInfo, typemap));
>-    lir->insStorei(INS_CONST(fi.word), lirbuf->rp,
>-                   callDepth * sizeof(FrameInfo) + offsetof(FrameInfo, word));
>+#define STORE_AT_RP(name)                                                     \
>+    lir->insStorei(INS_CONSTPTR(fi.name), lirbuf->rp,                         \
>+                   callDepth * sizeof(FrameInfo) + offsetof(FrameInfo, name))
>+
>+    STORE_AT_RP(callee);
>+    STORE_AT_RP(block);
>+    STORE_AT_RP(ip_adj);
>+    STORE_AT_RP(typemap);
>+    STORE_AT_RP(word);
>+
>+#undef STORE_AT_RP
> 
>     atoms = fun->u.i.script->atomMap.vector;
>     return true;
> }
> 
> JS_REQUIRES_STACK bool
> TraceRecorder::record_JSOP_CALL()
> {
>diff --git a/js/src/jstracer.h b/js/src/jstracer.h
>--- a/js/src/jstracer.h
>+++ b/js/src/jstracer.h
>@@ -186,16 +186,17 @@ enum ExitType {
>     OOM_EXIT,
>     OVERFLOW_EXIT,
>     UNSTABLE_LOOP_EXIT,
>     TIMEOUT_EXIT
> };
> 
> struct VMSideExit : public nanojit::SideExit
> {
>+    JSObject* block;
>     intptr_t ip_adj;
>     intptr_t sp_adj;
>     intptr_t rp_adj;
>     int32_t calldepth;
>     uint32 numGlobalSlots;
>     uint32 numStackSlots;
>     uint32 numStackSlotsBelowCurrentFrame;
>     ExitType exitType;
>@@ -243,17 +244,18 @@ public:
>     TreeInfo(nanojit::Fragment* _fragment) : unstableExits(NULL) {
>         fragment = _fragment;
>     }
>     ~TreeInfo();
> };
> 
> struct FrameInfo {
>     JSObject*       callee;     // callee function object
>-    intptr_t        ip_adj;     // callee script-based pc index and imacro pc
>+    JSObject*       block;      // caller block chain head
>+    intptr_t        ip_adj;     // caller script-based pc index and imacro pc
>     uint8*          typemap;    // typemap for the stack frame
>     union {
>         struct {
>             uint16  spdist;     // distance from fp->slots to fp->regs->sp at JSOP_CALL
>             uint16  argc;       // actual argument count, may be < fun->nargs
>         } s;
>         uint32      word;       // for spdist/argc LIR store in record_JSOP_CALL
>     };

Looks good. We should measure perf impact (if any).

Comment 6

[Brendan Eich [:brendan]]

Attachment: patch I'm about to commit to m-c

Comment 7

[Brendan Eich [:brendan]]

Fixed in m-c:

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

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/65c745704a53

Comment 9

[Bob Clary [:bc] (inactive)]

Checking in js1_7/extensions/regress-470300-01.js
Checking in js1_7/extensions/regress-470300-02.js
http://hg.mozilla.org/mozilla-central/rev/aa3026fa8b4f

Comment 10

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2