692274 - remove the two blockChain hacks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: 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.

Comment 1

[Luke Wagner [:luke]]

Attachment: 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.

Comment 2

[Luke Wagner [:luke]]

Attachment: fix block scope properly WIP 2

I'm going to be on vacation and gone for a week, so posting a WIP.  The patch has decompilation working for let with non-group-assignment destructuring.  Still to go is group-assigment and let-in-for-head.  To my surprise neither generator exprs nor array comprehensions seem to need touching.  I was hoping for better code reuse between the decompilation of let( and const/var/let-non-( but the decompilation strategies are just so different.

Here's a fun sample:

dis(function() { for (let i in o) {} })

Before:
main:
00000:  enterblock depth 0 {i: 0}
00003:  nop
00004:  nullblockchain
00005:  nop
00006:  blockchain depth 0 {i: 0}
00009:  nop
00010:  nullblockchain
00011:  getgname "o"
00014:  nop
00015:  blockchain depth 0 {i: 0}
00018:  iter 1
00020:  goto 32 (+12)
00023:  trace 0
00026:  iternext 1
00028:  setlocal 0
00031:  pop
00032:  moreiter
00033:  ifne 23 (-10)
00036:  enditer
00037:  leaveblock 1
00042:  stop

After:
00000:  push
00001:  getgname "o"
00004:  iter 1
00006:  enterpushedblock depth 0 {i: 0}
00009:  goto 21 (+12)
00012:  trace 0
00015:  iternext 1
00017:  setlocal 0
00020:  pop
00021:  moreiter
00022:  ifne 12 (-10)
00025:  leavepushedblock
00026:  enditer
00027:  pop
00028:  stop

Comment 4

[Luke Wagner [:luke]]

Attachment: 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.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix bug when decompiling generator expressions found while testing

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 577749 [details] [diff] [review]
fix bug when decompiling generator expressions found while testing

Oops, wrong patch.

Comment 7

[Luke Wagner [:luke]]

Attachment: hoist SprintNormalFor

Simple hoisting, reused in later patch.

Comment 8

[Luke Wagner [:luke]]

Attachment: fix bug when decompiling generator expressions found while testing

This is the one I meant earlier.

Comment 9

[Luke Wagner [:luke]]

Attachment: rm JSOP_BLOCKCHAIN et al

This patch just backs out bug 535912 and its followups.  It does not fix the dynamic scoping issues and thus fails several jit tests that were added after bug 535912 landed.

Comment 10

[Luke Wagner [:luke]]

Attachment: fix block scoping properly

This is the big patch that changes parsing, emitting, decompiling and reflecting.

So far its green try.

Comment 11

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing (applies onto b12dd7f965d0)

Gary, could you give this patch a proper fuzzing?

Comment 12

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

Comment on attachment 577770 [details] [diff] [review]
combined patch for fuzzing (applies onto b12dd7f965d0)

function f() {
    ""(this.z)
}
dis(f)
trap(f, 0, '')
f()

outputs:

flags: NULL_CLOSURE
loc     op
-----   --
main:
00000:  string ""  <-- trap goes here
00003:  push
00004:  this
00005:  getprop "z"
00008:  call 1
00011:  pop
00012:  stop

Source notes:
 ofs  line    pc  delta desc     args
---- ---- ----- ------ -------- ------
  0:    1     0 [   0] newline 
  1:    2     5 [   5] pcbase   offset 1
  3:    2     8 [   3] pcbase   offset 8

Assertion failure: *pc != JSOP_TRAP,

Comment 13

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing v.2 (applies onto b12dd7f965d0)

Thanks Gary!  I hate JSOP_TRAP.  New patch includes testcase; let's give 'er another go!

Comment 14

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

Comment on attachment 577819 [details] [diff] [review]
combined patch for fuzzing v.2 (applies onto b12dd7f965d0)

These results were from a session of overnight fuzzing:

for (let[[w, [N, c]]] = "";;) {}

Assertion failure: (depthBefore - bce->stackDepth) >= -1,

===

with({}) {
    for (let c in []) {
        with({}) {
            let([] = l) {}
        }
    }
}
try {
    let b = x(), V, z, i, x
} catch (e) {}

(pass this testcase in as a CLI argument)

Assertion failure: JSID_IS_ATOM(shape.propid),

===

String(function() {
    for (let {} = [
        []
    ] = {};;) {}
})

Assertion failure: ((js::SrcNoteType)(((*(sn) >> 3) >= SRC_XDELTA) ? SRC_XDELTA : *(sn) >> 3)) == SRC_DESTRUCT,

Comment 15

[Luke Wagner [:luke]]

Attachment: combined patch for fuzzing v.3 (applies onto 26bac72ef060)

Thanks Gary!  1 (added) assert botch, 1 non-local assert that needed fixup, 1 non-local case of hacks in GetLocal that makes me glad we have fuzzing.

Comment 16

[Christian Holler (:decoder)]

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 17

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

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 18

[Jason Orendorff [:jorendorff]]

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.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 577760 [details] [diff] [review]
fix bug when decompiling generator expressions found while testing

Needs a test!

Comment 20

[Luke Wagner [:luke]]

Attachment: 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.

Comment 21

[Luke Wagner [:luke]]

Attachment: fix block scoping properly (v.2)

Refreshing the patch for review.

Comment 22

[Luke Wagner [:luke]]

(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.

Comment 23

[Christian Holler (:decoder)]

Comment on attachment 578421 [details] [diff] [review]
combined patch for fuzzing v.4 (applies onto 1b3f17ffa656)

Review of attachment 578421 [details] [diff] [review]:
-----------------------------------------------------------------

I found two more asserts (probably the same bug) that only occur with this patch (all tests against 1b3f17ffa656 + patch, options -m -n -a):

var x = -false;
switch(x) {
    case 2, 11: 
    let y = 42;
}

=> Assertion failure: [infer failure] Missing type pushed 0: float, at jsinfer.cpp:348

IsWhiteSpace(Array.prototype.concat.toString().charAt(0)).substring(0,17)
function IsWhiteSpace( string ) {
  var cc = string.charCodeAt(0);
  switch (cc) {
    case IsWhiteSpace:  
    let strippedString = { sin: substring.sin };
  }
}

=> Assertion failure: [infer failure] Missing type pushed 0: int, at jsinfer.cpp:348

Same also for string, but that is probably also a duplicate. I also found a TI issue that is on m-c without the patch which involves switch but not let. It could be related though and I'll file soon and Cc you.

Comment 24

[Luke Wagner [:luke]]

Attachment: 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

Comment 25

[Luke Wagner [:luke]]

Attachment: fix bug when decompiling generator expressions found while testing

That r- is annoying me.

Comment 26

[Luke Wagner [:luke]]

Attachment: 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?

Comment 27

[Brian Hackett [Laid off!]]

Comment on attachment 578649 [details] [diff] [review]
fix block scoping properly (v.3)

Review of attachment 578649 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinfer.cpp
@@ +3974,5 @@
> +         * the stack (such as the condition to a switch) whose constraints must
> +         * be propagated.
> +         */
> +        poppedTypes(pc, 0)->addSubset(cx, &pushed[StackDefs(script, pc) - 1]);
> +        /* FALL THROUGH */

It may be better to use defCount instead of calling StackDefs.  Can you put this case below the ENTERBLOCK/ENTERLET0 case below, with no fallthrough and just note the same thing applies as in ENTERLET0?

Comment 28

[Luke Wagner [:luke]]

For fun I totaled the JSScript::dataSize for the runtime after startup and the patch queue seems to save about 16K.  Memshrink:P1!! ;-)

Comment 29

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

Comment on attachment 578636 [details] [diff] [review]
combined patch for fuzzing v.5 (applies onto ab56cc75af51)

I found the similar asserts as decoder in comment 23 and all have been fixed with this version.

Comment 30

[Christian Holler (:decoder)]

Comment on attachment 578636 [details] [diff] [review]
combined patch for fuzzing v.5 (applies onto ab56cc75af51)

Review of attachment 578636 [details] [diff] [review]:
-----------------------------------------------------------------

Four more hours of fuzzing with this patch haven't brought up any asserts :) If I find anything else I'll let you know.

Comment 31

[Luke Wagner [:luke]]

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).

Comment 32

[Christian Holler (:decoder)]

It seems I found one more assert caused by this:

var obj = {};
let([] = gczeal) 3; 
new let ( i = "String/match-002.js" ) new i [ obj[i] ];

=> Assertion failure: off <= -2, at js/src/jsopcode.cpp:1248

Comment 33

[Luke Wagner [:luke]]

Attachment: combined patch (rebased to apply on f0f0ec491b9e)

Gary asked for this to test whether this fixes another bug he found.

Comment 34

[Luke Wagner [:luke]]

Attachment: 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 35

[Luke Wagner [:luke]]

Attachment: combined patch (rebased to apply on f0f0ec491b9e)

... and the combined patch.

Comment 37

[Jason Orendorff [:jorendorff]]

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.

Comment 38

[Luke Wagner [:luke]]

(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.

Comment 39

[Jason Orendorff [:jorendorff]]

Comment on attachment 579757 [details] [diff] [review]
fix block scoping properly (v.4)

Well, I can't find a thing wrong with this.

I admit my eyes glazed over a little bit in the decompiler. Woof.

In BytecodeEmitter.cpp:
>+static JSObject *
>+CurrentBlock(BytecodeEmitter *bce)
>+{
>+    /*
>+     * In a few cases (switch, for-in) we enter a block after hav
>+     */

Apparently you got distracted in the middle of writing th

In UpdateDepth:
>+    /*
>+     * Specially handle any case that would call js_GetIndexFromBytecode since
>+     * it requires a well-formed script. This and the absence of JSOP_TRAP
>+     * allows us to safely pass NULL as the 'script' parameter.
>+     */
>+    intN nuses, ndefs;
>+    if (op == JSOP_ENTERBLOCK) {
>+        nuses = 0;
>+        ndefs = OBJ_BLOCK_COUNT(cx, CurrentBlock(bce));
>+    } else if (op == JSOP_ENTERLET0) {
>+        nuses = ndefs = OBJ_BLOCK_COUNT(cx, CurrentBlock(bce));
>+    } else if (op == JSOP_ENTERLET1) {
>+        nuses = ndefs = OBJ_BLOCK_COUNT(cx, CurrentBlock(bce)) + 1;
>+    } else {
>+        nuses = StackUses(NULL, pc);
>+        ndefs = StackDefs(NULL, pc);
>+    }

JSOP_TRAP is gone; update the comment. If JSOP_ENTERLET[01] have nuses=0
and ndefs=0 (see comments further below), then those cases can be
removed. A special case is still needed for JSOP_ENTERBLOCK, but the
little function CurrentBlock is only called once, so its code can be
folded back in here.

Still in UpdateDepth:
>-    if (bce->stackDepth < 0) {
>-        char numBuf[12];
>-        TokenStream *ts;
>-
>-        JS_snprintf(numBuf, sizeof numBuf, "%d", target);
>-        ts = &bce->parser->tokenStream;
>-        JS_ReportErrorFlagsAndNumber(cx, JSREPORT_WARNING,

What a lot of nonsense! Thanks for deleting it.

In EmitNonLocalJumpFixup:
>+            if (stmt->flags & SIF_FOR_BLOCK) {
>+                /*
>+                 * For a for-let-in statement, pushing/popping the block is
>+                 * interleaved with JSOP_(END)ITER. Just handle both together
>+                 * here and skip over the enclosing STMT_FOR_IN_LOOP.
>+                 */

I think eventually we want to swap this so that the AST structure of
  for (let A in B) C
is:
  for ($tmp in B) let (A = $tmp) C

A pure desugaring, like `for (let;;)` except inside-out.

This is because contrary to our current practice, ES6 is going to
specify that each iteration of the loop gets a fresh binding of the
let-variables, so that the loop can create closures and each one sees a
separate binding for A.

Once we do that, the complication discussed in the above comment goes
away, right? We burn an extra stack slot on $tmp, but I think the
simplification is worth it.

No action needed, just thinking out loud here.

>+ * EmitDestructuringLHS assumes the to-be-destructured value has been pushed on
>+ * the stack and emits code to destructure a single lhs expressions (either a
>+ * name or a compound []/{} expression).

Nit: "a single lhs expression", not "expressions"

>+ * If emitOption is InitializeVars, the to-be-destructured value is assigned to
>+ * locals (popped the expression afterwards) and ultimately the initial slot is
>+ * popped (-1 total depth change).

I don't think I understand what's going on here. Maybe "(popped the
expression afterwards)" is redundant now and should just be deleted.

>+             * Per its post-condition, EmitDestructuringOpsHelper has left the
>+             * to-be-destructuring value on top of the stack.

Nit: "the to-be-destructured value".

In comment on EmitDestructuringOpsHelper:
> /*
>  * Recursive helper for EmitDestructuringOps.
>+
>+ * EmitDestructuringOpsHelper assumes the to-be-destructured value has been

Stray blank line here. And:

>+ * lhs expression. (Same post-condition as EmitDestructuringOpsHelper)

"Same post-condition as EmitDestructuringLHS".

In EmitDestructuringOpsHelper:
>+                /*
>+                 * After '[x,y]' in 'let ([[x,y], z] = o)', the stack is
>+                 *   | to-be-decompiled-value | x | y |
>+                 * The goal is:
>+                 *   | x | y | z |
>+                 * so emit a pick to produce the intermediate state
>+                 *   | x | y | to-be-decompiled-value |
>+                 * before destructuring z. This gives the loop invariant that
>+                 * the to-be-compiled-value is always on top of the stack.
>+                 */
>+                JS_ASSERT((bce->stackDepth - bce->stackDepth) >= -1);
>+                uintN pickDistance = (uintN)((bce->stackDepth + 1) - depthBefore);
>+                if (pickDistance > 0) {
>+                    if (pickDistance > jsbytecode(-1)) {
>+                        ReportCompileErrorNumber(cx, bce->tokenStream(), pn3, JSREPORT_ERROR,
>+                                                 JSMSG_TOO_MANY_LOCALS);
>+                        return JS_FALSE;
>+                    }

Heh. 256 variables should be enough for anyone!

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?

I dunno, maybe 256 variables *should* be enough for everyone.

>+MaybeEmitLetGroupDecl(JSContext *cx, BytecodeEmitter *bce, ParseNode *pn,

I have to admit, I'm slightly amazed you did this. But OK!

>+/*
>+ * pnLet represents one of:
>+ *
>+ *   let-expression:   (let (x = y) EXPR)
>+ *   let-statement:    let (x = y) { ... }
>+ *
>+ * For a let-expression 'let (x = a, [y,z] = b) e', EmitLet produces:
>+ *
>+ *  bytecode          stackDepth  srcnotes
>+ *  evaluate a        +1
>+ *  evaluate b        +1
>+ *  dup               +1          SRC_DESTRUCTLET + offset to enterlet0
>+ *  destructure y
>+ *  dup               +1          SRC_DESTRUCTLET + offset to enterlet0
>+ *  destructure z
>+ *  pop               -1
>+ *  enterlet0                     SRC_DECL + offset to leaveblockexpr
>+ *  evaluate e        +1
>+ *  leaveblockexpr    -3          SRC_PCBASE + offset to evaluate a

The actual bytecode emitted for this expression uses JSOP_PICK as it
goes about destructuring.

In EmitLexicalScope:
>-#if defined DEBUG_brendan || defined DEBUG_mrbkap
>-        /* There must be no source note already output for the next op. */
>-        JS_ASSERT(bce->noteCount() == 0 ||
>-                  bce->lastNoteOffset() != bce->offset() ||
>-                  !GettableNoteForNextOp(bce));
>-#endif

Turn this on for everyone rather than delete it?

If the assertion is obsolete, you can also delete the helper function
GettableNoteForNextOp.

In EmitNormalFor:
>             if (pn3->isKind(PNK_VAR) || pn3->isKind(PNK_CONST) || pn3->isKind(PNK_LET)) {
>                 /*
>                  * Check whether a destructuring-initialized var decl
>                  * was optimized to a group assignment.  If so, we do
>                  * not need to emit a pop below, so switch to a nop,
>                  * just for the decompiler.
>                  */
>-                JS_ASSERT(pn3->isArity(PN_LIST));
>+                JS_ASSERT(pn3->isArity(PN_LIST) || pn3->isArity(PN_BINARY));
>                 if (pn3->pn_xflags & PNX_GROUPINIT)
>                     op = JSOP_NOP;
>             }
>         }
>         bce->flags &= ~TCF_IN_FOR_INIT;
>     }

I don't understand this change.


In frontend/Parser.cpp, struct BindData:
>     union {
>         struct {
>+            VarContext varContext;
>+            JSObject *blockObj;
>             uintN   overflow;
>         } let;
>     };

What? A union with only one--oh, I see what's going on here. initLet,
initVarOrConst. Well, OK, this is pre-existing oddness. Filed bug 712168
to tidy it up.

In BindLet:
>-    /*
>-     * Body-level 'let' is the same as 'var' currently -- this may change in a
>-     * successor standard to ES5 that specifies 'let'.
>-     */
>-    JS_ASSERT(!tc->atBodyLevel());

Why remove the assertion?

>  */
> static bool
>-CheckDestructuring(JSContext *cx, BindData *data, ParseNode *left, TreeContext *tc)
>+CheckDestructuring(JSContext *cx, BindData *data, ParseNode *left, TreeContext *tc,
>+                   bool toplevel = true)

Consider mentioning in the comment what toplevel means? For a while I
expected it to mean something like "at global scope" or "not in any
function". (Well, sure it's obvious once you figure it out!)

In NewBindingNode:
>+    /*
>+     * If this name is being injected into an existing block/function, see if
>+     * it has already been declared or if it resolves an outstanding lexdep.
>+     * Otherwise, this is a let block/expr that introduces a new scope and thus
>+     * shadows existing decls and doesn't resolve existing lexdeps. Duplicate
>+     * names (in the same let initializer) are caught by BindLet).
>+     */

I don't understand the "(in the same let initializer)". Aren't other
duplicate names in the same scope also caught by BindLet?

Also there is a stray ')' at the end of this comment.

In Parser::letStatement:
>     do {

I filed follow-up bug 711662 to get rid of the weird control flow here.

In Parser::variables:
>     /* Make sure that statement set up the tree context correctly. */
>-    StmtInfo *scopeStmt = tc->topScopeStmt;
>-    if (let) {
>+    if (blockObj) {
>+        StmtInfo *scopeStmt = tc->topScopeStmt;
>         while (scopeStmt && !(scopeStmt->flags & SIF_SCOPE)) {
>             JS_ASSERT(!STMT_MAYBE_SCOPE(scopeStmt));
>             scopeStmt = scopeStmt->downScope;
>         }
>-        JS_ASSERT(scopeStmt);
>     }

I'm having trouble making sense of the complicated assertion here. Now
that blockObj may or may not have anything to do with the static scope
chain currently in tc->topScopeChain, the rest of the method no longer
depends on the property being asserted here. Right?

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. At
least the comment should be improved.


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.

In jsinfer.cpp, ScriptAnalysis::analyzeTypesBytecode:
>+      case JSOP_ENTERLET1:
>+        /*
>+         * JSOP_ENTERLET1 enters a let block with an unrelated value on top of
>+         * the stack (such as the condition to a switch) whose constraints must
>+         * be propagated. The other values are ignored for the same reason as
>+         * JSOP_ENTERLET0.
>+         */
>+        poppedTypes(pc, 0)->addSubset(cx, &pushed[defCount - 1]);
>+        break;

If nuses=0 and ndefs=0, I think there's no need to do anything here. (In
fact... it seems like by pretending that we pop and re-push all those
values, we must be throwing away type information that we could use. But
I don't know this code well enough to say.)

Same for IgnorePushed: if defs=0, then it is never called.

In jsopcode.cpp, js::StackUses:
>+      case JSOP_ENTERLET0:
>+        return NumBlockSlots(script, pc);
>+      case JSOP_ENTERLET1:
>+        return NumBlockSlots(script, pc) + 1;

If nuses=0, this isn't necessary.

In js::StackDefs:
>+    return op == JSOP_ENTERLET1 ? n + 1 : n;

If ndefs=0, the extra check isn't necessary.


In jsopcode.cpp, NumBlockSlots:
>+    JSContext *cx = NULL;
>+    JSObject *obj = NULL;
>+    GET_OBJECT_FROM_BYTECODE(script, pc, 0, obj);
>+    return OBJ_BLOCK_COUNT(NULL, obj);
>+}

lol, can we just remove the cx argument from js_GetIndexFromBytecode?

In DecompileDestructuringLHS, case JSOP_PICK:
>+         * Thus 'x' is consists of 1 - 3. The caller (DecompileDestructuring or

Delete the word "is".

In the comment for AssignBlockNamesToPushedSlots:
>+ * In the scope of a let, the var (decompiler stack) slots must contain the
>+ * corresponding var's name. This function updates the N top slots with the N
>+ * variable names stored in 'atoms'.

Using the word "var" to mean let-variables is too weird. "variable" would be ok.

In Decompile, case JSOP_DUP:
>+                        AutoFree<char> rhs(cx, JS_strdup(cx, PopStr(ss, JSOP_SETLOCAL)));
>+                        if (!rhs.ptr())
>+                            return NULL;

I don't really care but I think you could use Dup and DupBuffer here instead.


In methodjit/Compiler.cpp:
> void
> mjit::Compiler::enterBlock(JSObject *obj)
> {
>     /* For now, don't bother doing anything for this opcode. */
>     frame.syncAndForgetEverything();
>     masm.move(ImmPtr(obj), Registers::ArgReg1);
>-    uint32 n = js_GetEnterBlockStackDefs(cx, script, PC);
>     INLINE_STUBCALL(stubs::EnterBlock, REJOIN_NONE);
>-    frame.enterBlock(n);
>+    if (*PC == JSOP_ENTERBLOCK)
>+        frame.enterBlock(StackDefs(script, PC));
> }

It looks like stubs::EnterBlock does nothing (except DEBUG-only
assertions) for JSOP_ENTERBLOCK0/1. Can we skip calling it?


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.

>+    var got = fun(arg);
>+    var expect = result;
>+    if (got !== expect) {
>+        print("GOT:" + got);
>+        print("EXPECT: " + expect);
>+        assertEq(false, true);
>+    }

assertEq(fun(arg), result);

>+function isError(str)
>+{
>+    var caught = false;
>+    try {
>+        new Function(str);
>+    } catch(e) {
>+        assertEq(String(e).indexOf('TypeError') == 0 || String(e).indexOf('SyntaxError') == 0, true);
>+        caught = true;
>+    }
>+    assertEq(caught, true);
>+}

Why is TypeError a possibility here?

You could load(libdir + 'asserts.js') and use assertThrowsInstanceOf
here. I think it is worth distinguishing the TypeErrors from the
SyntaxErrors but maybe it's just me.

>+test('try {return let (x = eval("throw x")) x;} catch (e) {return x;}');

I'm not sure but I think you might have meant to return e rather than x.
It wouldn't hurt to test both ways.

>+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.

>+test('for (let i in x) {let (i = x) {return i;}}');

Please add
  test('for (let i in x) { let i = x; return i; }');
to test that those two scopes are different.

Several of these tests made me cry.

>+test('for (let i in x) {break;}return x;');
>+test('a:for (let i in x) {for (let j in x) {break a;}}return x;');

Please add one like this using an eval, to make sure that breaking out
of the loop really exits the let-scope. Like this:

  test('for (let x in ...) { ... break ... } return eval("x");');

Scopes not tested here: indirect eval within a let-scope; nested direct
eval in let-scopes; let-scopes in direct eval code; let-declarations in
strict-mode direct eval.

All these tests seem to test read access to variables; it would be good
to have some covering assignment and inc/dec.

We've talked about the pros and cons of cartesian-product testing. I'm
not against it, but if you're going to use the technique it seems better
to use loops than copy and paste! If nothing else it would have been
quicker to review... tut tut :)

>+// genexps
>+test('return (i for (i in x)).next();', {ponies:true});
>+test('return (eval("i") for (i in x)).next();', {ponies:true});
>+test('return (eval("i") for (i in eval("x"))).next();', {ponies:true});
>+test('try {return (eval("throw i") for (i in x)).next();} catch (e) {return e;}', {ponies:true});

While you're here, test that each 'x' in something like (x for (x in x))
refers to the right x.

Same for array comprehensions.

An example or two using for-each in combination with genexps and
comprehensions would be welcome.

Comment 40

[Luke Wagner [:luke]]

(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.

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44412b34781d
https://hg.mozilla.org/mozilla-central/rev/2454b814b09f
https://hg.mozilla.org/mozilla-central/rev/9272bb82eeba
https://hg.mozilla.org/mozilla-central/rev/38344f96b3e3
https://hg.mozilla.org/mozilla-central/rev/9ed956b9a3b1

Comment 43

[Andy Wingo [:wingo]]

See also bug 927782.