470137 - TM: Behavioral difference in addition of objects between JIT, non-JIT

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jeff Walden [:Waldo]]

For ease of copy-paste:

var count = 0;
function toString() { ++count; if (count == 5) return {}; return "" + count; }
for (var i = 0; i < 10; i++)
{
  var o = {valueOf: undefined, toString: toString};
  var q = 5 + o;
}

Results below with current TM:

find-waldo-now:~/moz/js-tm jwalden$ rlwrap obj-i386-apple-darwin8.11.1/dist/bin/js
js> var count = 0;
js> function toString() { ++count; if (count == 5) return {}; return "" + count; }
js> for (var i = 0; i < 10; i++)
{
  var o = {valueOf: undefined, toString: toString};
  var q = 5 + o;
}
typein:6: TypeError: can't convert o to primitive type
js> 
find-waldo-now:~/moz/js-tm jwalden$ rlwrap obj-i386-apple-darwin8.11.1/dist/bin/js -j
js> var count = 0;
js> function toString() { ++count; if (count == 5) return {}; return "" + count; }
for (var i = 0; i < 10; i++)
{
  var o = {valueOf: undefined, toString: toString};
  var q = 5 + o;
}
js> 

With JIT, at end I have count == 10 and q == 510.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Stealing, hope you don't mind, Brendan, as I suspect you hadn't spent any substantial amount of time on this yet...

We don't fall off trace currently because the return value of toString isn't assigned to any fixed-type temporaries.  The reason we don't see infinite rcursion is that the returned object does stringify using Object.prototype.toString (changing the loop termination value to 5 results in q === "5[object Object]").  I'm not sure why changing |{}| to |this| and moving the |++count| increment a little don't result in infinite recursion, but I'm not especially worried about not investigating exactly what prevents infinite recursion here.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 354101 [details] [diff] [review]
Patch

Thanks!

/be

Comment 3

[Jeff Walden [:Waldo]]

Fixed in TM:

http://hg.mozilla.org/tracemonkey/rev/65561fc2807c

Comment 4

[Jeff Walden [:Waldo]]

Hm, it's "sideExitIntoInterpreter", not "sideExits".  It seems we're actually side exiting on every loop there, no so good -- investigating...

Comment 5

[Robert Sayre]

merged to mc

Comment 6

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d6306712a5d9

Comment 7

[Bob Clary [:bc] (inactive)]

test included in js1_8_1/trace/trace-test.js 
http://hg.mozilla.org/mozilla-central/rev/8f967a7729e2

Comment 8

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2