Handle accessors on the prototype chain in StoreICs. (issue 10735003)

[ross...@chromium.org]

LGTM, with comment


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

http://codereview.chromium.org/10735003/diff/1/test/mjsunit/object-define-property.js#newcode1144
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.

http://codereview.chromium.org/10735003/

[sven...@chromium.org]

Reviewers: rossberg, Vyacheslav Egorov,

Message:
Landing...

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

http://codereview.chromium.org/10735003/diff/1/test/mjsunit/object-define-property.js#newcode1144
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.

Description:
Handle accessors on the prototype chain in StoreICs.

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


Please review this at http://codereview.chromium.org/10735003/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/arm/stub-cache-arm.cc
M src/ia32/stub-cache-ia32.cc
M src/ic.cc
M src/mips/stub-cache-mips.cc
M src/stub-cache.h
M src/stub-cache.cc
M src/x64/stub-cache-x64.cc
M test/mjsunit/accessor-map-sharing.js
M test/mjsunit/object-define-property.js

[veg...@google.com]

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?

http://codereview.chromium.org/10735003/

[ross...@chromium.org]

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.

http://codereview.chromium.org/10735003/

[sven...@chromium.org]

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?

http://codereview.chromium.org/10735003/

[Vyacheslav Egorov]

My main concern here was that the comment above the check did not
actually explain why the check is needed (e.g. which corner case stubs
can't handle).

I have a strong feeling that the check is redundant (e.g. I expect
stubs to match the semantics of runtime and correctly detect MISS
caused by a mutation of objects in the chain from receiver to the
holder)

--
Vyacheslav Egorov

[ross...@chromium.org]

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.

http://codereview.chromium.org/10735003/

[Vyacheslav Egorov]

But it's should fixed semantics affect only cases when setter
_appears_ on the prototype and store site has a stub compiled for
assignment into holder?

--
Vyacheslav Egorov

[Vyacheslav Egorov]

argh, gibberish should read:

but should not the fixed semantics...
--
Vyacheslav Egorov

[Andreas Rossberg]

On 4 July 2012 15:59, Vyacheslav Egorov <veg...@google.com> wrote:

argh, gibberish should read:

but should not the fixed semantics...
--
Vyacheslav Egorov

On Wed, Jul 4, 2012 at 3:58 PM, Vyacheslav Egorov <veg...@google.com> wrote:
> But it's should fixed semantics affect only cases when setter
> _appears_ on the prototype and store site has a stub compiled for
> assignment into holder?

Yes, as I said, it _probably_ doesn't matter. But when I recently fixed most of the setters-appearing-on-prototype behaviour along with readonliness, 370 tests broke. Which is why I introduced the flag as a temporary measure. I didn't look at all of the failures individually, so I don't know if setters were among the causes, or if it only was readonliness.

Either way, I intend to find out soon. ;)