460501 - Round-trip change due to "&&" constant-folding leaving extra parens

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> f = (function() { if ((1 && w.valueOf())) {} })  
function () {
    if ((w.valueOf())) {
    }
}

js> eval(uneval(f));
function () {
    if (w.valueOf()) {
    }
}

This happens with "valueOf" and "eval" but not with most other names.

Comment 1

[Jason Orendorff [:jorendorff]]

Brendan, earlier this week you mentioned the possibility of just ripping out TOK_RP throughout.  That would fix this, right?

Comment 2

[Brendan Eich [:brendan]]

Yeah, getting rid of JSOP_GROUP would fix this bug -- a bigger project than I can take on right now. You willing to try?

/be

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

This removes JSOP_GROUP.  It retains the TOK_RP nodes but doesn't emit anything for them.  Passes regrtests.

The TOK_RP nodes are useless, but I'd like to kill them in a separate patch; I am paranoid about regressions, being unfamiliar with this code, and would appreciate the separate bisect target.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 343712 [details] [diff] [review]
v1

Correction -- I wasn't testing the right thing, and there are several decompiler bugs in v1.

Comment 5

[Jason Orendorff [:jorendorff]]

For your amusement--one of those is the venerable
  new (x(y).z)
decompiling as
  new x(y).z

js_Decompile, case JSOP_NEW, has a nice comment explaining why that needs to be parenthesized and how the code supposedly handles it, but there's a bug--I think it was unwittingly depending on JSOP_GROUP all along. :-)

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> For your amusement--one of those is the venerable
>   new (x(y).z)
> decompiling as
>   new x(y).z
> 
> js_Decompile, case JSOP_NEW, has a nice comment explaining why that needs to be
> parenthesized and how the code supposedly handles it, but there's a bug--I
> think it was unwittingly depending on JSOP_GROUP all along. :-)

The dependency is witting. The other comment under JSOP_GROUP's case is:

              case JSOP_GROUP:
                cs = &js_CodeSpec[lastop];
                if ((cs->prec != 0 &&
                     cs->prec <= js_CodeSpec[NEXT_OP(pc)].prec) ||
                    pc[JSOP_GROUP_LENGTH] == JSOP_NULL ||
                    pc[JSOP_GROUP_LENGTH] == JSOP_NULLTHIS ||
                    pc[JSOP_GROUP_LENGTH] == JSOP_DUP ||
                    pc[JSOP_GROUP_LENGTH] == JSOP_IFEQ ||
                    pc[JSOP_GROUP_LENGTH] == JSOP_IFNE) {
                    /*
                     * Force parens if this JSOP_GROUP forced re-association
                     * against precedence, or if this is a call or constructor
                     * expression, or if it is destructured (JSOP_DUP), or if
                     * it is an if or loop condition test.
                     *
                     * This is necessary to handle the operator new grammar,
                     * by which new x(y).z means (new x(y))).z.  For example
                     * new (x(y).z) must decompile with the constructor
                     * parenthesized, but normal precedence has JSOP_GETPROP
                     * (for the final .z) higher than JSOP_NEW.  In general,
                     * if the call or constructor expression is parenthesized,
                     * we preserve parens.
                     */
                    op = JSOP_NAME;
                    rval = POP_STR();

If you removed this code, then you broke the other code under JSOP_NEW/CALL/EVAL.

/be

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2

This flunks two new tests, in addition to all the ones that were already failing. In both cases, the new breakage is clearly a harmless change in parenthesization due to JSOP_GROUP being removed.

js1_5/decompilation/regress-456964-01.js
  expect: o += (i + " = " + object[i] + "\n");
  actual: o += i + " = " + object[i] + "\n";
js1_7/decompilation/regress-352026.js
  expect: function ( ) { z ( ( yield 3 ) ) ; } 
  actual: function ( ) { z ( yield 3 ) ; } 

I had to tweak the logic for decompiling binary expressions to get
cases like (function () a + (b + c) + d) right.

I changed the precedence of the JSOP_DEL* opcodes from 17 (like new) to 15 (like the prefix operators ! ~ - ++ --), reflecting the precedence of delete expressions rather than the minimum permissible precedence of their subexpressions.  This was necessary to force parentheses when decompiling |new (delete foo)| and in turn necessitated changes to how those opcodes are decompiled.

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to comment #7)
> This flunks two new tests, in addition to all the ones that were already
> failing.

By this I mean only that tracemonkey is in a failing state, I hope just because we haven't merged recent updates to js/src/public-failures.txt.

The patch is good stuff!

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 343946 [details] [diff] [review]
v2

Thanks for taking this on. Few can face the decompiler and not blink -- impressive patchwork.

Need to rev jsxdrapi.h's JSXDR_BYTECODE_VERSION when removing opcodes.

>@@ -5132,25 +5131,21 @@ js_Interpret(JSContext *cx)
>             }
> 
>             id = ATOM_TO_JSID(atom);
>             if (js_FindPropertyHelper(cx, id, &obj, &obj2, &prop, &entry) < 0)
>                 goto error;
>             if (!prop) {
>                 /* Kludge to allow (typeof foo == "undefined") tests. */
>                 endpc = script->code + script->length;
>-                for (pc2 = regs.pc + JSOP_NAME_LENGTH; pc2 < endpc; pc2++) {
>-                    op2 = (JSOp)*pc2;
>-                    if (op2 == JSOP_TYPEOF) {
>-                        PUSH_OPND(JSVAL_VOID);
>-                        len = JSOP_NAME_LENGTH;
>-                        DO_NEXT_OP(len);
>-                    }
>-                    if (op2 != JSOP_GROUP)
>-                        break;
>+                op2 = (JSOp) *(regs.pc + JSOP_NAME_LENGTH);

Nit: prefer p[i] to the longer *(p + i) form, even when p is a pointer not an array.

>+/*
>+ * To parenthesize correctly in case JSOP_NEW, we must know whether the
>+ * left-hand side contains any non-parenthesized function calls.  So when

s/non-/un/

>+ * building a MemberExpression or CallExpression, we set ss->opcodes[n] to
>+ * JSOP_CALL if this is true: |foo().x.y| gets JSOP_CALL, not JSOP_GETPROP.

See below for evolved comment relocation opportunity.

>+                rval = POP_STR_PREC(cs->prec + (inXML ? 0 : cs->format & JOF_LEFTASSOC ? 1 : 0));
>+                lval = POP_STR_PREC(cs->prec + (inXML ? 0 : cs->format & JOF_LEFTASSOC ? 0 : 1));

Nit: parenthesize condition to left of ? as a rule -- but:

Why not use (cs->prec + (inXML || !(cs->format & JOF_LEFTASSOC)) for the second line (!! instead of ! for the first line)? Hardly seems clearer spelled out with ?: but it is certainly longer.

>+                    op = JSOP_IFEQ;  /* extra parens around assignment */
>                     js_printf(jp, "\t} while (%s);\n", POP_STR());

Make a POP_COND_STR() to common these op = JSOP_IFEQ hacks? Better, it would use your POP_STR_PREC and not mutate op at all.

>-                     * This is necessary to handle the operator new grammar,
>-                     * by which new x(y).z means (new x(y))).z.  For example
>-                     * new (x(y).z) must decompile with the constructor
>-                     * parenthesized, but normal precedence has JSOP_GETPROP
>-                     * (for the final .z) higher than JSOP_NEW.  In general,
>-                     * if the call or constructor expression is parenthesized,
>-                     * we preserve parens.

Want an evolution of this comment to live on, mainly because it shows both cases and discusses precedence relations. JSOP_CALL, JSOP_GETPROP, and the other ops that call PROPAGATE_CALLNESS, all having the same precedence, and this should be commented too.

>diff --git a/js/src/jsopcode.tbl b/js/src/jsopcode.tbl
>--- a/js/src/jsopcode.tbl
>+++ b/js/src/jsopcode.tbl

Please update the big precedence table comment above the first OPDEF.

>@@ -5691,37 +5691,18 @@ PrimaryExpr(JSContext *cx, JSTokenStream
>             return NULL;
>         pn2 = ParenExpr(cx, ts, tc, pn, &genexp);
>         if (!pn2)
>             return NULL;
>         if (genexp)
>             return pn2;
> 
>         MUST_MATCH_TOKEN(TOK_RP, JSMSG_PAREN_IN_PAREN);
>-        if (pn2->pn_type == TOK_RP ||
>-            (js_CodeSpec[pn2->pn_op].prec >= js_CodeSpec[JSOP_GETPROP].prec &&
>-             !afterDot)) {
>-            /*
>-             * Avoid redundant JSOP_GROUP opcodes, for efficiency and mainly
>-             * to help the decompiler look ahead from a JSOP_ENDINIT to see a
>-             * JSOP_GROUP followed by a POP or POPV.  That sequence means the
>-             * parentheses are mandatory, to disambiguate object initialisers
>-             * as expression statements from block statements.
>-             *
>-             * Also drop pn if pn2 is a member or a primary expression of any
>-             * kind.  This is required to avoid generating a JSOP_GROUP that
>-             * will null the |obj| interpreter register, causing |this| in any
>-             * call of that member expression to bind to the global object.
>-             */
>-            RecycleTree(pn, tc);
>-            pn = pn2;
>-        } else {
>-            pn->pn_type = TOK_RP;
>-            pn->pn_kid = pn2;
>-        }
>+        pn->pn_type = TOK_RP;
>+        pn->pn_kid = pn2;

Are you sure code elsewhere (jsemit.cpp) does not depend on this code? I'd defer removing it if you can until that followup bug to remove TOK_RP nodes from the AST.

/be

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: interdiff v2-v3

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: v3

Adopt all brendan's suggestions.

Comment 12

[Jason Orendorff [:jorendorff]]

I see I inadvertently added a second blank line before a comment in jsopcode.cpp.  I'll delete that for checkin.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 344096 [details] [diff] [review]
v3

> #define POP_STR()             PopStr(ss, op)
>-#define LOCAL_ASSERT(expr)    LOCAL_ASSERT_RV(expr, NULL)
>+#define POP_STR_PREC(prec)    PopStrPrec(ss, prec)
>+
>+/*
>+ * Pop a condition expression for if/for/while. JSOP_IFEQ's precedence forces
>+ * extra parens around assignment, which avoids a strict-mode warning.
>+ */
>+#define POP_COND_STR()        POP_STR_PREC(js_CodeSpec[JSOP_IFEQ].prec)

Defining POP_COND_STR as PopStr(ss, JSOP_IFEQ) would minimize code size. r=me with that.

/be

Comment 14

[Jason Orendorff [:jorendorff]]

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

Comment 15

[Jesse Ruderman]

Before the patch, both of these were stable:
(function() { if(a = b) { } })
(function() { if((a = b)) { } })

After the patch, both decompile to the latter.  I think this is an improvement, since it makes a strict warning go away.  I'll adjust jsfunfuzz's "checkForExtraParens" to stop complaining.

I don't think this new behavior helps in "for" conditions, though.  I filed bug 461108 for that.

Comment 16

[Bob Clary [:bc] (inactive)]

this breaks js1_8_1/decompilation/regress-346642-01.js 14 and 15. jorendorff says that is expected. I'll update the tests in the next push.

Comment 17

[Bob Clary [:bc] (inactive)]

(In reply to comment #7)

> js1_5/decompilation/regress-456964-01.js
>   expect: o += (i + " = " + object[i] + "\n");
>   actual: o += i + " = " + object[i] + "\n";

the expect expression here is not part of the checked in test. Did you make local revisions?

Comment 18

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/37b3fdbb0f07
/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-460501.js,v  <--  regress-460501.js
initial revision: 1.1