558446 - CONCATN: toString methods called in wrong order

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

var s = '';
var a = {toString: function () { s += 'a'; return 'a'; }};
var b = {toString: function () { s += 'b'; return 'b'; }};
function f(a, b) { return a + " " + b; }
f(a, b);
assertEq(s, 'ab');  // fails: 'ba'

The relevent bit of bytecode in f:
  getarg 0
  string " "
  getarg 1
  objtostr
  concatn 2
  add

I think ideally it should read:
  getarg 0
  objtostr
  string " "
  getarg 1
  objtostr
  concatn 3

but in any case, given a + "b" + c + d + e + f, a must be ToString-ed before the rest are evaluated.

Luke, can you take this?

Comment 1

[Luke Wagner [:luke]]

You bet

Comment 2

[Luke Wagner [:luke]]

Attachment: fix

Should've caught this with bug 545165...  The original strategy combined the results of the first N not-necessarily-string additions with concatn's results with an add which occurs after everything, thus the delayed side effect.  Stringifying early and using concatn by itself both fixes the problem and removes an unnecessary temporary.

Comment 3

[Luke Wagner [:luke]]

Note to self: can remove JOF_TMPSLOT2 from JSOP_CONCATN OPDEF

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 438208 [details] [diff] [review]
fix

>diff --git a/js/src/jsemit.cpp b/js/src/jsemit.cpp

>+            if (!pn2)
>+                break;

Nice early-exit here, we should have done this before.


>+            /*
>+             * Having seen a string literal, we know statically that the rest
>+             * of the additions are string concatenation, so we emit them as a
>+             * single concatn. First, do string conversion on the result of the
>+             * preceding zero or more additions so that any side effects of
>+             * string conversion occur before the next operand begins.
>+             */
>+            index = 0;
>+            if (pn2 != pn->pn_head) {
>+                if (!js_Emit1(cx, cg, JSOP_OBJTOSTR))
>+                    return JS_FALSE;
>+                index++;
>+            }
>+

Suggest pushing |index = 1| down below this block -- zeroing then always incrementing on success doesn't seem to have much purpose.  You can also convert the check-and-emit-maybe-fail bits into two lines rather than four.


>+            for (; pn2; pn2 = pn2->pn_next, index++) {
>                 if (!js_EmitTree(cx, cg, pn2))
>                     return JS_FALSE;
>+                if (!pn2->isLiteral() && js_Emit1(cx, cg, JSOP_OBJTOSTR) < 0)
>+                    return JS_FALSE;
>+            }

do {
  if (!js_EmitTree(...) ...;
  if (!pn2->isLiteral() && ...) ...;
  index++;
} while ((pn2 = pn2->pn_next) != NULL);

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #4)
> >+            for (; pn2; pn2 = pn2->pn_next, index++) {
> >                 if (!js_EmitTree(cx, cg, pn2))
> >                     return JS_FALSE;
> >+                if (!pn2->isLiteral() && js_Emit1(cx, cg, JSOP_OBJTOSTR) < 0)
> >+                    return JS_FALSE;
> >+            }
> 
> do {
>   if (!js_EmitTree(...) ...;
>   if (!pn2->isLiteral() && ...) ...;
>   index++;
> } while ((pn2 = pn2->pn_next) != NULL);

As a general rule, I prefer to increment loop variables, like index, in the increment clause of for loops.  It conveys the intention clearly (both to the reader and compiler).  I think this clarity is worth the single unnecessary branch.

Comment 6

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/47d29a5adb38

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/47d29a5adb38