From 1d75ca964d4015762a0c19c1f773cc9f2a6e165e Mon Sep 17 00:00:00 2001 From: Jason Orendorff Date: Mon, 15 Jul 2013 10:28:35 -0500 Subject: [PATCH] Bug 889628, part 2 - Do not look at lhs->getOp() when selecting opcodes for destructuring assignment, except if lhs is a name and we just called BindNameToSlot. r=Waldo. --HG-- extra : rebase_source : 955e32774099d85cab5b5d6b83a9d5233493fa08 --- js/src/frontend/BytecodeEmitter.cpp | 122 ++++++++++++++++------------ js/src/frontend/Parser.cpp | 18 ---- 2 files changed, 69 insertions(+), 71 deletions(-) diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp index 8d47eb0e6ae3..1c6ef0a14e17 100644 --- a/js/src/frontend/BytecodeEmitter.cpp +++ b/js/src/frontend/BytecodeEmitter.cpp @@ -2731,65 +2731,90 @@ EmitDestructuringLHS(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn, if (Emit1(cx, bce, JSOP_POP) < 0) return false; } + } else if (emitOption == PushInitialValues) { + // The lhs is a simple name so the to-be-destructured value is + // its initial value and there is nothing to do. + JS_ASSERT(pn->getOp() == JSOP_GETLOCAL); + JS_ASSERT(pn->pn_dflags & PND_BOUND); } else { - if (emitOption == PushInitialValues) { - /* - * The lhs is a simple name so the to-be-destructured value is - * its initial value and there is nothing to do. - */ - JS_ASSERT(pn->getOp() == JSOP_GETLOCAL); - JS_ASSERT(pn->pn_dflags & PND_BOUND); - return true; - } + // All paths below must pop after assigning to the lhs. - /* All paths below must pop after assigning to the lhs. */ - - if (pn->isKind(PNK_NAME)) { + switch (pn->getKind()) { + case PNK_NAME: if (!BindNameToSlot(cx, bce, pn)) return false; - /* Allow 'const [x,y] = o', make 'const x,y; [x,y] = o' a nop. */ + // Allow 'const [x,y] = o', make 'const x,y; [x,y] = o' a nop. if (pn->isConst() && !pn->isDefn()) return Emit1(cx, bce, JSOP_POP) >= 0; - } - switch (pn->getOp()) { - case JSOP_SETNAME: - case JSOP_SETGNAME: - /* - * NB: pn is a PN_NAME node, not a PN_BINARY. Nevertheless, - * we want to emit JSOP_ENUMELEM, which has format JOF_ELEM. - * So here and for JSOP_ENUMCONSTELEM, we use EmitElemOp. - */ + switch (pn->getOp()) { + case JSOP_SETNAME: + case JSOP_SETGNAME: + // This is like ordinary assignment, but with one difference. + // + // In `a = b`, we first determine a binding for `a` (using + // JSOP_BINDNAME or JSOP_BINDGNAME), then we evaluate `b`, then + // a JSOP_SETNAME instruction. + // + // In `[a] = [b]`, per spec, `b` is evaluated first, then we + // determine a binding for `a`. Then we need to do assignment-- + // but the operands are on the stack in the wrong order for + // JSOP_SETPROP, so we use JSOP_ENUMELEM instead. + // + // EmitElemOp ordinarily works with PNK_ELEM nodes, naturally, + // but it has special code to handle PNK_NAME nodes in this one + // case. + if (!EmitElemOp(cx, pn, JSOP_ENUMELEM, bce)) + return false; + break; + + case JSOP_SETCONST: + // As above. + if (!EmitElemOp(cx, pn, JSOP_ENUMCONSTELEM, bce)) + return false; + break; + + case JSOP_SETLOCAL: + case JSOP_SETARG: + if (!EmitVarOp(cx, pn, pn->getOp(), bce)) + return false; + if (Emit1(cx, bce, JSOP_POP) < 0) + return false; + break; + + default: + MOZ_ASSUME_UNREACHABLE("EmitDestructuringLHS: bad name op"); + } + break; + + case PNK_DOT: + case PNK_ELEM: + // See the (PNK_NAME, JSOP_SETNAME) case above. + // + // In `a.x = b`, `a` is evaluated first, then `b`, then a + // JSOP_SETPROP instruction. + // + // In `[a.x] = [b]`, per spec, `b` is evaluated before `a`. Then we + // need a property set -- but the operands are on the stack in the + // wrong order for JSOP_SETPROP, so we use JSOP_ENUMELEM instead. + // + // EmitElemOp has special code to handle PNK_DOT nodes in this one + // case. if (!EmitElemOp(cx, pn, JSOP_ENUMELEM, bce)) return false; break; - case JSOP_SETCONST: - if (!EmitElemOp(cx, pn, JSOP_ENUMCONSTELEM, bce)) - return false; - break; - - case JSOP_SETLOCAL: - case JSOP_SETARG: - if (!EmitVarOp(cx, pn, pn->getOp(), bce)) - return false; - if (Emit1(cx, bce, JSOP_POP) < 0) - return false; - break; - - case JSOP_CALL: - case JSOP_EVAL: - case JSOP_FUNCALL: - case JSOP_FUNAPPLY: + case PNK_CALL: JS_ASSERT(pn->pn_xflags & PNX_SETCALL); if (!EmitTree(cx, bce, pn)) return false; - /* - * We just emitted JSOP_SETCALL which will always throw. - * Pop the call return value and the RHS. - */ + // Pop the call return value and the RHS, presumably for the + // benefit of bytecode analysis. (The interpreter will never reach + // these instructions since we just emitted JSOP_SETCALL, which + // always throws. It's possible no analyses actually depend on this + // either.) if (Emit1(cx, bce, JSOP_POP) < 0) return false; if (Emit1(cx, bce, JSOP_POP) < 0) @@ -2797,16 +2822,7 @@ EmitDestructuringLHS(ExclusiveContext *cx, BytecodeEmitter *bce, ParseNode *pn, break; default: - { - if (!EmitTree(cx, bce, pn)) - return false; - if (!EmitElemOpBase(cx, bce, JSOP_ENUMELEM)) - return false; - break; - } - - case JSOP_ENUMELEM: - JS_ASSERT(0); + MOZ_ASSUME_UNREACHABLE("EmitDestructuringLHS: bad lhs kind"); } } diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp index 2db8be0b00c3..970eae08129f 100644 --- a/js/src/frontend/Parser.cpp +++ b/js/src/frontend/Parser.cpp @@ -2919,24 +2919,6 @@ Parser::bindDestructuringVar(BindData *data, return true; } -/* - * Here, we are destructuring {... P: Q, ...} = R, where P is any id, Q is any - * LHS expression except a destructuring initialiser, and R is on the stack. - * Because R is already evaluated, the usual LHS-specialized bytecodes won't - * work. After pushing R[P] we need to evaluate Q's "reference base" QB and - * then push its property name QN. At this point the stack looks like - * - * [... R, R[P], QB, QN] - * - * We need to set QB[QN] = R[P]. This is a job for JSOP_ENUMELEM, which takes - * its operands with left-hand side above right-hand side: - * - * [rval, lval, xval] - * - * and pops all three values, setting lval[xval] = rval. But we cannot select - * JSOP_ENUMELEM yet, because the LHS may turn out to be an arg or local var, - * which can be optimized further. So we select JSOP_SETNAME. - */ template <> bool Parser::bindDestructuringLHS(ParseNode *pn)