The interpreter engine for the core JavaScript language, independent of the browser's object model. File ONLY core JavaScript language bugs in this category. For bugs involving browser objects such as "window" and "document", use the "DOM" component. For bugs involving calls between JavaScript and C++, use the "XPConnect" component.

Created attachment 565017[details][diff][review]
rm JSOP_BLOCKCHAIN et al
This bug is about removing the JSOP_BLOCKCHAIN hacks as well as the hacky use JSOP_BLOCKCHAIN to make let expressions get the right result.
The JSOP_BLOCKCHAIN removal which does not correctly handle let expressions is mostly just reverting bug 535912, and that is attached.
The challenge with let expressions is that, for (let (x = <1>) <2>) we generate:
enterblock
<1>
setlocal
pop
<2>
This puts "x" in scope for expression <1> which is semantically (and apparently, for engine correctness) invalid. The current hack around this abuses JSOP_BLOCKCHAIN in unspeakable ways. The naive initial goal is to generate:
<1>
enterblockexpr
<2>
and have JSOP_ENTERBLOCKEXPR simply leave the current value where it is (since it's exactly where the block's value should be) and adjust fp->blockChain like normal. We'll see how that works out.

Created attachment 569126[details][diff][review]
fix block scope properly WIP 1
This is a WIP, posted so frontend-savvy hackers can tell me if I'm going about it all wrong. The patch completely ignores decompilation (doing that next) but should handle parsing/emitting/running (destructuring) let decls/blocks/exprs in statements, fors, and for-ins.

Created attachment 576672[details][diff][review]
WIP 3 (so close)
Phew, decompilation done. That was a ridiculous amount of work for such a logically small change. The decompiler really reduces the malleability of our engine; I am now 10x more in favor of killing it than I was before... perhaps in conjunction with bug 678037.
The patch is practically done, feedback is welcome. It is passing many tests but I just realized that jsreflect needs major hackage. Le sigh. On the bright side jsreflect is relatively gorgeous.

I have another assert that is caused by this patch but not fixed even in the newest version (options -m -a -n):
function f() {
var ss = [new f("abc"), new String("foobar"), new String("quux")];
for (let a6 = this ;; ) {}
}
f();
Assertion failure: uses, at /srv/repos/mozilla-central/js/src/jsinfer.cpp:4548

Comment on attachment 578158[details][diff][review]
combined patch for fuzzing v.3 (applies onto 26bac72ef060)
feedback+ from me on this, ran this on a Linux machine in 32-bit shell overnight and didn't find anything more.
asking feedback? from decoder as he has one more assert above.

Comment on attachment 577750[details][diff][review]
hoist SprintNormalFor
>-#undef LOCAL_ASSERT
>-
> static bool
> PushBlockNames(JSContext *cx, SprintStack *ss, const AtomVector &atoms)
The LOCAL_ASSERT being undefined here returns false if the assertion fails.
Of course we hope the assertion will never fail, but in SprintNormalFor we want a failing LOCAL_ASSERT to return -1.
Or if you prefer, change SprintNormalFor to return true on success and false on failure, rather than -2 and -1. I would prefer that slightly, but it doesn't really matter.
>+ jsbytecode *&pc = *ppc;
>+ ptrdiff_t &len = *plen;
>+
>+ jssrcnote *sn = js_GetSrcNote(jp->script, pc);
>+ JS_ASSERT(SN_TYPE(sn) == SRC_FOR);
>+
>+ /* Skip the JSOP_NOP or JSOP_POP bytecode. */
>+ JS_ASSERT(*pc == JSOP_NOP || *pc == JSOP_POP);
>+ pc += JSOP_NOP_LENGTH;
Please make the type of pc 'jsbytecode *', so it's a copy of *ppc rather than a reference to it. The last line quoted above is the only place we modify *ppc, so there's not much to fix up:
*ppc = (pc += JSOP_NOP_LENGTH);
Remove len entirely and just change the function to say *plen instead, in the one place where it's used.
r=me with those nits picked.

Created attachment 578421[details][diff][review]
combined patch for fuzzing v.4 (applies onto 1b3f17ffa656)
Awesome find decoder! Even though JSOP_ENTERPUSHEDBLOCK does not mutate the stack, TI wants to see nuses and ndefs cover all the let slots.

(In reply to Jason Orendorff [:jorendorff] from comment #19)
> Needs a test!
This was originally part of the down-patch which contains several tests (any attempt to decompile generator expressions at all fails... any at all); I just split it out for reviewing. It would be nice for later cset readers, though.

Created attachment 578636[details][diff][review]
combined patch for fuzzing v.5 (applies onto ab56cc75af51)
Thanks decoder! Indeed it was the same (simple) bug for both cases.
/me is still learning how TI works

Created attachment 578649[details][diff][review]
fix block scoping properly (v.3)
Rebased, with TI fix and a windows-only fix that turns try green on win32 (DO_NEXT_OP(JSOP_*_LENGTH) is wrong with the non-threaded interpreter :( ).
bhackett, could you review the TI aspects of this patch?

Thanks to the both of you. That was about as brutal as I expected...
Incidentally Gary pointed out that the combined-for-fuzzing patch includes a bunch of random changes from all over mozilla. That's a mistake; I had some hg issues and so that patch includes a bunch of m-i csets (and my patches).

Created attachment 579757[details][diff][review]
fix block scoping properly (v.4)
Ah, I didn't see comment 32. It seems the assert is invalid (caused by the completely bogus try-to-find-some-block-obj-enclosing-this-range search done by GetLocal which ends up asking for the name of the dummy slot pushed for empty destructuring). Removed the assert in GetOff and added the testcase.

Comment on attachment 577763[details][diff][review]
rm JSOP_BLOCKCHAIN et al
Review of attachment 577763[details][diff][review]:
-----------------------------------------------------------------
My main comments:
- The HAS_BLOCKCHAIN flag appears to be new. Is it? Is this because it's expensive to NULL out the blockChain_ member of the stack frame in the common case where the code contains no blocks, and you'd rather just leave it uninitialized? I would rather see this part in a separate changeset if it is an optimization (but if that's too much work and you don't see enough benefit, skip it).
- You didn't add code to mjit::Compiler::generatePrologue to initialize blockChain_. Is that for the same reason--to avoid a single store?
Apart from that I just have nits:
You didn't touch StackFrame::resetCallFrame. But then I'm not sure what that is for. It doesn't appear to be used anywhere, so perhaps just delete it.
You removed the definition of frontend::Emit5, but I think you left the declaration in frontend/BytecodeEmitter.h.
Before JSOP_BLOCKCHAIN landed, js::Interpret used to have this assertion in the code after "inline_return:":
> JS_ASSERT(!regs.fp()->hasBlockChain());
Can it be readded?
Similarly, js::Interpret used to have this assertion in the code after "exit:":
>+ JS_ASSERT_IF(!regs.fp->isGeneratorFrame(), !regs.fp->hasBlockChain());
Can it be readded?
::: js/src/frontend/BytecodeEmitter.cpp
@@ +1343,5 @@
> {
> stmt->type = type;
> stmt->flags = 0;
> stmt->blockid = tc->blockid();
> SET_STATEMENT_TOP(stmt, top);
Here you removed an assertion:
>- JS_ASSERT(!stmt->blockBox);
but you did not add:
>+ JS_ASSERT(!stmt->blockObj);
even though it did say that before the JSOP_BLOCKCHAIN patch.
Can the old assertion be reinstated?
@@ +5292,5 @@
> PushStatement(bce, &stmtInfo, STMT_WITH, bce->offset());
> if (Emit1(cx, bce, JSOP_ENTERWITH) < 0)
> return false;
>
> /* Make blockChain determination quicker. */
You removed the EmitBlockChain call but forgot to remove this comment.
::: js/src/jsopcode.cpp
@@ +2712,5 @@
> rval = OFF2STR(&ss->sprinter, todo);
> todo = -2;
> pc2 = pc + oplen;
>
> /* Skip a block chain annotation if one appears here. */
Here too, please delete the stranded comment.
::: js/src/jsopcode.tbl
@@ +442,5 @@
>
> OPDEF(JSOP_CALLPROP, 185,"callprop", NULL, 3, 1, 2, 18, JOF_ATOM|JOF_PROP|JOF_TYPESET|JOF_CALLOP|JOF_TMPSLOT3)
>
> +OPDEF(JSOP_UNUSED1, 186,"unused1", NULL, 1, 0, 0, 0, JOF_BYTE)
> +OPDEF(JSOP_UNUSED2, 187,"unused2", NULL, 1, 0, 0, 0, JOF_BYTE)
Please line the NULLs up with the preceding lines.
::: js/src/vm/Stack.h
@@ +337,5 @@
> OVERFLOW_ARGS = 0x800, /* numActualArgs > numFormalArgs */
> UNDERFLOW_ARGS = 0x1000, /* numActualArgs < numFormalArgs */
>
> /* Lazy frame initialization */
> + HAS_IMACRO_PC = 0x2000, /* frame has imacpc value available */
We can do without that.

(In reply to Jason Orendorff [:jorendorff] from comment #37)
Thanks for all the great comments.
> - The HAS_BLOCKCHAIN flag appears to be new. Is it?
Yes it is new. We worked pretty hard to get calls down to only doing 5 stores to the StackFrame and saw benefit every step of the way. There are already 7 lazily-initialized fields so *not* making blockChain_ lazy would be the confusing thing to do since it would beg the question "why not?". Furthermore, initializing eagerly would require touching the extremely delicate call IC / prologue in the methodjit (and reasoning about all the paths that can be taken) so it would be strictly more complicated to make it eager. (All this StackFrame hackery will go away with IM, btw, since IM will use custom ion frames which will let js::StackFrame only be used for cold interpreted code.)
> - You didn't add code to mjit::Compiler::generatePrologue to initialize
> blockChain_. Is that for the same reason--to avoid a single store?
Yes, and it's simpler to not reason about the 3 (maybe more now) paths through that sucker.
> You didn't touch StackFrame::resetCallFrame. But then I'm not sure what that
> is for. It doesn't appear to be used anywhere, so perhaps just delete it.
Yeah, I just noticed it was dead in a later patch.

(In reply to Jason Orendorff [:jorendorff] from comment #39)
Thank you for the careful review!
> The restriction could be removed by keeping the to-be-decompiled value
> always at the top of the stack. That is, emitting a JSOP_SWAP after
> destructuring each property/element. It's more code, but the jit throws
> the SWAP away, right?
Yeah, that could work. I think it would be a fairly major change at this point. Another simple alternative is making JSOP_PICK's immediate 3 bytes instead of 1 :)
> >- JS_ASSERT(pn3->isArity(PN_LIST));
> >+ JS_ASSERT(pn3->isArity(PN_LIST) || pn3->isArity(PN_BINARY));
> I don't understand this change.
Indeed, it doesn't exactly jump to mind (testLet.js had to remind me):
for (let (x=1) x;;) {}
> Why remove the assertion?
Parser::variables now executes before PushLetScope pushes the stmtInfo so tc->atBodyLevel(). Note: the patch pushes the assert down into the HoistVars branch.
> I don't understand the "(in the same let initializer)". Aren't other
> duplicate names in the same scope also caught by BindLet?
You're right, it is distracting without adding value; I'll take it out. The intention was to point out that, since let shadows enclosing scopes, the only redefinition error that can occur is duplicate names within the same let head (e.g., let (x,x) y).
> And since it's hard to even figuring out what this property is much less
> articulate why it can't happen, maybe we should just rip this out.
Agreed.
> In jsopcode.tbl:
> >+/* Enter a let block/expr whose slots are at the top of the stack. */
> >+OPDEF(JSOP_ENTERLET0, 185,"enterlet0", NULL, 3, -1, -1, 0, JOF_OBJECT)
> >+
> >+/* Enter a let block/expr whose slots are 1 below the top of the stack. */
> >+OPDEF(JSOP_ENTERLET1, 186,"enterlet1", NULL, 3, -1, -1, 0, JOF_OBJECT)
>
> I think these two opcodes should have nuses=0 and ndefs=0.
From irc: because of the way TI's analysis models let vars (namely, it ignores the slots of let variables while the let is in scope), we want enter to use and define each let slot.
> It looks like stubs::EnterBlock does nothing (except DEBUG-only
> assertions) for JSOP_ENTERBLOCK0/1. Can we skip calling it?
There's an fp->setBlockChain(obj) and the end.
> In jit-test/tests/basic/testLet.js:
> >+ if (got !== expect) {
> >+ print("GOT: " + got);
> >+ print("EXPECT: " + expect);
> >+ assertEq(false, true);
> >+ }
>
> Hmm. I dunno, assertEq(got, expect) seems like a better way to say
> this. On failure, you get uneval(got) and uneval(expect) in the error
> message.
The print() statements are nice b/c they align 'got' and 'expect' so that the difference jumps out at you quickly. I am happy to change to assertEq(got, expect), though.
> >+ new Function(str);
> >+ } catch(e) {
> >+ assertEq(String(e).indexOf('TypeError') == 0 || String(e).indexOf('SyntaxError') == 0, true);
>
> Why is TypeError a possibility here?
"TypeError: redeclaration of variable x"
> >+test('if (x) {let [X] = [x];return X;}');
> >+test('if (x) {let [y] = [x];return y;}');
>
> Yes, yes I really did read them all. Or at least this far.
Impressive
> While you're here, test that each 'x' in something like (x for (x in x))
> refers to the right x.
I did that earlier and then backed off: given 'var o = {ponies:true}', on both trunk and with the patch,
(o for (o in o)).next()
throws StopIteration and the array comprehension
[o for (o in o)]
is empty. Both results seem wrong so I did't want to make a test demanding wrong behavior. Since (miraculously) the changes in this bug are orthogonal to both, I'd rather leave the issue alone.