Handle accessors on the prototype chain in StoreICs.

Handle accessors on the prototype chain in StoreICs.

Made stub compiler function signatures a bit more consistent on the way.

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

[rossberg, 2012-07-04 10:34:09]

LGTM, with comment

http://codereview.chromium.org/10735003/diff/1/test/mjsunit/object-define-pro...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/10735003/diff/1/test/mjsunit/object-define-pro...
test/mjsunit/object-define-property.js:1144: testSetterOnProto(446, obj3);
Maybe you want some additional tests here where LookupForWrite finds an accessor
pair but one without a setter. E.g. you remove an existing setter, but it still
has a getter, so that it is readonly.

[Sven Panne, 2012-07-04 11:40:24]

Landing...

http://codereview.chromium.org/10735003/diff/1/test/mjsunit/object-define-pro...
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/10735003/diff/1/test/mjsunit/object-define-pro...
test/mjsunit/object-define-property.js:1144: testSetterOnProto(446, obj3);
On 2012/07/04 10:34:09, rossberg wrote:
> Maybe you want some additional tests here where LookupForWrite finds an
accessor
> pair but one without a setter. E.g. you remove an existing setter, but it
still
> has a getter, so that it is readonly.

Done.

[Vyacheslav Egorov (Google), 2012-07-04 11:48:34]

http://codereview.chromium.org/10735003/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/10735003/diff/1/src/ic.cc#newcode1322
src/ic.cc:1322: if (!FLAG_es5_readonly) return false;
Sorry, I do not understand why es5_readonly semantics is essential here.

We already lookup and find setters on the prototype chain and then invoke them
on writes. What breaks if we start caching that?

[rossberg, 2012-07-04 13:18:17]

http://codereview.chromium.org/10735003/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/10735003/diff/1/src/ic.cc#newcode1322
src/ic.cc:1322: if (!FLAG_es5_readonly) return false;
On 2012/07/04 11:48:34, Vyacheslav Egorov (Google) wrote:
> Sorry, I do not understand why es5_readonly semantics is essential here.
> 
> We already lookup and find setters on the prototype chain and then invoke them
> on writes. What breaks if we start caching that?

Perhaps it isn't necessary, but this patch amends the fixes I made earlier and
(had to) put behind that flag -- which, despite the flag's name, also fixed the
treatment of inherited setters, except for the oversight here.

[Sven Panne, 2012-07-04 13:44:34]

On 2012/07/04 13:18:17, rossberg wrote:
> Perhaps it isn't necessary, but this patch amends the fixes I made earlier and
> (had to) put behind that flag -- which, despite the flag's name, also fixed
the
> treatment of inherited setters, except for the oversight here.

Hmmm, thinking about it a bit, I am not convinced anymore that we need the test.
In addition to Slava's argument, all our tests + tests262 still work when the
test is removed. It might actually be clearer when it is removed, but I don't
have any strong feelings about it. Any opinions?

[rossberg, 2012-07-04 13:55:06]

On 2012/07/04 13:44:34, Sven Panne wrote:
> On 2012/07/04 13:18:17, rossberg wrote:
> > Perhaps it isn't necessary, but this patch amends the fixes I made earlier
and
> > (had to) put behind that flag -- which, despite the flag's name, also fixed
> the
> > treatment of inherited setters, except for the oversight here.
> 
> Hmmm, thinking about it a bit, I am not convinced anymore that we need the
test.
> In addition to Slava's argument, all our tests + tests262 still work when the
> test is removed. It might actually be clearer when it is removed, but I don't
> have any strong feelings about it. Any opinions?

Well, all _our_ tests work fine with the flag turned on anyway. ;) The
incompatibilities arose downstream, with V8 WebKit bindings relying on the
previous incorrect semantics (or at least I saw plenty of breakage on our WebKit
bots). I think Arv has confirmed that no breakage exists in Chromium anymore, so
I will soon try again to remove the flag anyway.