Handle overwriting valueOf on String objects correctly when adding...

Handle overwriting valueOf on String objects correctly when adding

This adds a check to the fast case string add to ensure that the String object still have the default valueOf function. The default valueOf is sitting on a hidden prototype of String.prototype.

Before using the fast case valueOf the object is checked for a local valueOf property. For slow case objects this check always reports true (the dictionary is not probed, so valueOf might be there) and for fast case objects the descriptor array is checked for the valueOf symbol (just liniar scan). After that the prototype is checked for beeing the initial value of String.prototype. If this all pass (that is the default valueOf is still in place) this result is cached on the map making the check fast the next time.

This is only implemented in the optimizing compiler, as the two usages of %_IsStringWrapperSafeForDefaultValueOf is never hit by the full compiler.

I will port to x64 and ARM when this has been reviewed for ia32.

I will remove the performance counters prior to final commit.

BUG=http://code.google.com/p/v8/issues/detail?id=760
TEST=test/mjsunit/regress/regress-760-1.js
TEST=test/mjsunit/regress/regress-760-2.js


Committed: http://code.google.com/p/v8/source/detail?r=5252

[Mads Ager (chromium), 2010-08-11 11:26:41]

LGTM

http://codereview.chromium.org/3117006/diff/1/2
File src/bootstrapper.cc (right):

http://codereview.chromium.org/3117006/diff/1/2#newcode1241
src/bootstrapper.cc:1241:
HeapObject::cast(string_function->initial_map()->prototype())->map());
I guess this change will only work if the prototype is a fast-case object
(otherwise a change of the valueOf function will not introduce a new map). We
should ASSERT that this is the case.

http://codereview.chromium.org/3117006/diff/1/13
File test/mjsunit/regress/regress-760-1.js (right):

http://codereview.chromium.org/3117006/diff/1/13#newcode28
test/mjsunit/regress/regress-760-1.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=760
Could you add a short text description as well?

http://codereview.chromium.org/3117006/diff/1/14
File test/mjsunit/regress/regress-760-2.js (right):

http://codereview.chromium.org/3117006/diff/1/14#newcode28
test/mjsunit/regress/regress-760-2.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=760
Ditto

[Søren Thygesen Gjesse, 2010-08-12 13:08:33]

Addressed the comments and removed the performance counters.

Please check the added x64 and ARM ports.

http://codereview.chromium.org/3117006/diff/1/2
File src/bootstrapper.cc (right):

http://codereview.chromium.org/3117006/diff/1/2#newcode1241
src/bootstrapper.cc:1241:
HeapObject::cast(string_function->initial_map()->prototype())->map());
On 2010/08/11 11:26:41, Mads Ager wrote:
> I guess this change will only work if the prototype is a fast-case object
> (otherwise a change of the valueOf function will not introduce a new map). We
> should ASSERT that this is the case.

Done.

http://codereview.chromium.org/3117006/diff/1/13
File test/mjsunit/regress/regress-760-1.js (right):

http://codereview.chromium.org/3117006/diff/1/13#newcode28
test/mjsunit/regress/regress-760-1.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=760
On 2010/08/11 11:26:41, Mads Ager wrote:
> Could you add a short text description as well?

Done.

http://codereview.chromium.org/3117006/diff/1/14
File test/mjsunit/regress/regress-760-2.js (right):

http://codereview.chromium.org/3117006/diff/1/14#newcode28
test/mjsunit/regress/regress-760-2.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=760
On 2010/08/11 11:26:41, Mads Ager wrote:
> Ditto

Done.

[Mads Ager (chromium), 2010-08-12 13:33:51]

LGTM

http://codereview.chromium.org/3117006/diff/15024/24002
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/3117006/diff/15024/24002#newcode4825
src/arm/codegen-arm.cc:4825: __ cmp(map_result_,ip);
Space after comma.

http://codereview.chromium.org/3117006/diff/15024/24002#newcode4882
src/arm/codegen-arm.cc:4882: 
Remove these lines?

http://codereview.chromium.org/3117006/diff/15024/24002#newcode4893
src/arm/codegen-arm.cc:4893: // Set the bit in the map to indicate that it has
been checked safe for
Add a line before this comment instead?

http://codereview.chromium.org/3117006/diff/15024/24018
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/3117006/diff/15024/24018#newcode6086
src/x64/codegen-x64.cc:6086: Operand(map_result_, index.reg, index.scale,
FixedArray::kHeaderSize));
Long line.

[Søren Thygesen Gjesse, 2010-08-12 13:42:23]

http://codereview.chromium.org/3117006/diff/15024/24002
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/3117006/diff/15024/24002#newcode4825
src/arm/codegen-arm.cc:4825: __ cmp(map_result_,ip);
On 2010/08/12 13:33:51, Mads Ager wrote:
> Space after comma.

Done.

http://codereview.chromium.org/3117006/diff/15024/24002#newcode4882
src/arm/codegen-arm.cc:4882: 
On 2010/08/12 13:33:51, Mads Ager wrote:
> Remove these lines?

Done.

http://codereview.chromium.org/3117006/diff/15024/24002#newcode4893
src/arm/codegen-arm.cc:4893: // Set the bit in the map to indicate that it has
been checked safe for
On 2010/08/12 13:33:51, Mads Ager wrote:
> Add a line before this comment instead?

Done.

http://codereview.chromium.org/3117006/diff/15024/24018
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/3117006/diff/15024/24018#newcode6086
src/x64/codegen-x64.cc:6086: Operand(map_result_, index.reg, index.scale,
FixedArray::kHeaderSize));
On 2010/08/12 13:33:51, Mads Ager wrote:
> Long line.

Done.