Change LookupForWrite to always do a full lookup and check the result.

Change LookupForWrite to always do a full lookup and check the result.

If we find a property in the prototype-chain that we can overwrite, and
we have a transition, keep the holder in the lookup-result as the actual
holder. We will need it for the consistency-check in GenerateStoreField.

By directly checking the entire chain we avoid having to lazily bail out
to a copy of the miss stub while generating the Field Store IC.

Currently this CL disallows a normal non-receiver holder, given that
that would require a positive lookup + details verification to ensure
the property did not become read-only. This fixes the regressions in the
attached tests.

Committed: https://code.google.com/p/v8/source/detail?r=14061

[Toon Verwaest, 2013-03-21 15:25:07]

PTAL

[rossberg, 2013-03-22 14:07:44]

LGTM, only nits.

https://codereview.chromium.org/12810006/diff/3001/src/ic.cc
File src/ic.cc (left):

https://codereview.chromium.org/12810006/diff/3001/src/ic.cc#oldcode1591
src/ic.cc:1591: if (!holder.is_identical_to(receiver)) break;
If that's an impossible case, maybe turn it into an assertion.

https://codereview.chromium.org/12810006/diff/3001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12810006/diff/3001/src/ic.cc#newcode1440
src/ic.cc:1440: if (lookup->IsPropertyCallbacks()) return true;
Test case... :)

https://codereview.chromium.org/12810006/diff/3001/src/ic.cc#newcode1449
src/ic.cc:1449: receiver->map()->LookupTransition(*holder, *name, lookup);
Maybe add a comment explaining why you use the holder here.

https://codereview.chromium.org/12810006/diff/3001/test/mjsunit/regress/reado...
File test/mjsunit/regress/readonly1.js (right):

https://codereview.chromium.org/12810006/diff/3001/test/mjsunit/regress/reado...
test/mjsunit/regress/readonly1.js:35: return x;
You can simplify this to

  return {__proto__: p};

Same in other tests.

https://codereview.chromium.org/12810006/diff/3001/test/mjsunit/regress/reado...
test/mjsunit/regress/readonly1.js:61: // Verify that setting through the IC
fails.
Could you also add attempts to set it in strict mode (which should throw)?
Similarly in other tests.

[Toon Verwaest, 2013-03-25 11:44:44]

Addressed comments, PTAL again.

https://chromiumcodereview.appspot.com/12810006/diff/3001/src/ic.cc
File src/ic.cc (left):

https://chromiumcodereview.appspot.com/12810006/diff/3001/src/ic.cc#oldcode1591
src/ic.cc:1591: if (!holder.is_identical_to(receiver)) break;
On 2013/03/22 14:07:44, rossberg wrote:
> If that's an impossible case, maybe turn it into an assertion.

Done.

https://chromiumcodereview.appspot.com/12810006/diff/3001/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/12810006/diff/3001/src/ic.cc#newcode1440
src/ic.cc:1440: if (lookup->IsPropertyCallbacks()) return true;
On 2013/03/22 14:07:44, rossberg wrote:
> Test case... :)

Done.

https://chromiumcodereview.appspot.com/12810006/diff/3001/src/ic.cc#newcode1449
src/ic.cc:1449: receiver->map()->LookupTransition(*holder, *name, lookup);
On 2013/03/22 14:07:44, rossberg wrote:
> Maybe add a comment explaining why you use the holder here.

Done.

https://chromiumcodereview.appspot.com/12810006/diff/3001/test/mjsunit/regres...
File test/mjsunit/regress/readonly1.js (right):

https://chromiumcodereview.appspot.com/12810006/diff/3001/test/mjsunit/regres...
test/mjsunit/regress/readonly1.js:35: return x;
On 2013/03/22 14:07:44, rossberg wrote:
> You can simplify this to
> 
>   return {__proto__: p};
> 
> Same in other tests.

Done.

https://chromiumcodereview.appspot.com/12810006/diff/3001/test/mjsunit/regres...
test/mjsunit/regress/readonly1.js:61: // Verify that setting through the IC
fails.
On 2013/03/22 14:07:44, rossberg wrote:
> Could you also add attempts to set it in strict mode (which should throw)?
> Similarly in other tests.

Done.

[rossberg, 2013-03-25 12:08:09]

LGTM with comments

https://codereview.chromium.org/12810006/diff/10001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/12810006/diff/10001/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:467: // prototype chain) is is slow mode, we need to
do a negative lookup on the
"is is" -> "is in" (same for other back-ends)

https://codereview.chromium.org/12810006/diff/10001/test/mjsunit/regress/read...
File test/mjsunit/regress/readonly1.js (right):

https://codereview.chromium.org/12810006/diff/10001/test/mjsunit/regress/read...
test/mjsunit/regress/readonly1.js:69: assertThrows("s_strict(o4);");
Can't harm to say assertThrow(..., TypeError) here (likewise in other tests).

https://codereview.chromium.org/12810006/diff/11015/test/mjsunit/regress/nega...
File test/mjsunit/regress/negative_lookup.js (right):

https://codereview.chromium.org/12810006/diff/11015/test/mjsunit/regress/nega...
test/mjsunit/regress/negative_lookup.js:41: var o1 = c(p);
Perhaps add a comment warning that it is crucial for the test to create these
before putting p into slow mode.

[Toon Verwaest, 2013-03-25 12:36:21]

Addressed comments, landing.

https://chromiumcodereview.appspot.com/12810006/diff/10001/src/arm/stub-cache...
File src/arm/stub-cache-arm.cc (right):

https://chromiumcodereview.appspot.com/12810006/diff/10001/src/arm/stub-cache...
src/arm/stub-cache-arm.cc:467: // prototype chain) is is slow mode, we need to
do a negative lookup on the
On 2013/03/25 12:08:09, rossberg wrote:
> "is is" -> "is in" (same for other back-ends)

Done.

https://chromiumcodereview.appspot.com/12810006/diff/10001/test/mjsunit/regre...
File test/mjsunit/regress/readonly1.js (right):

https://chromiumcodereview.appspot.com/12810006/diff/10001/test/mjsunit/regre...
test/mjsunit/regress/readonly1.js:69: assertThrows("s_strict(o4);");
On 2013/03/25 12:08:09, rossberg wrote:
> Can't harm to say assertThrow(..., TypeError) here (likewise in other tests).

Done.

https://chromiumcodereview.appspot.com/12810006/diff/11015/test/mjsunit/regre...
File test/mjsunit/regress/negative_lookup.js (right):

https://chromiumcodereview.appspot.com/12810006/diff/11015/test/mjsunit/regre...
test/mjsunit/regress/negative_lookup.js:41: var o1 = c(p);
On 2013/03/25 12:08:09, rossberg wrote:
> Perhaps add a comment warning that it is crucial for the test to create these
> before putting p into slow mode.

Done.

[Toon Verwaest, 2013-03-25 12:55:46]

Committed patchset #5 manually as r14061 (presubmit successful).