Closed Bug 558446 Opened 12 years ago Closed 11 years ago

CONCATN: toString methods called in wrong order

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: luke)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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?
You bet
Assignee: general → lw
Status: NEW → ASSIGNED
Attached patch fixSplinter Review
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.
Attachment #438208 - Flags: review?(jwalden+bmo)
Note to self: can remove JOF_TMPSLOT2 from JSOP_CONCATN OPDEF
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);
Attachment #438208 - Flags: review?(jwalden+bmo) → review+
(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.
http://hg.mozilla.org/tracemonkey/rev/47d29a5adb38
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/47d29a5adb38
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.