super.property store

Super store

When the property is not found on the [[HomeObject]] prototype chain
then we should do a [[DefineOwnProperty]] on the instance.

BUG=v8:3330
LOG=N

Committed: https://crrev.com/0cffc08b667a067089c7eecffea3e8021ab2ff57
Cr-Commit-Position: refs/heads/master@{#26754}

[arv (Not doing code reviews), 2015-02-17 20:29:34]

cleanup

[arv (Not doing code reviews), 2015-02-17 20:41:22]

arv@chromium.org changed reviewers:
+ dslomov@chromium.org, verwaest@chromium.org

[arv (Not doing code reviews), 2015-02-17 20:41:22]

PTAL

https://codereview.chromium.org/934463003/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/20001/src/objects.cc#newcode3148
src/objects.cc:3148: if (data_store_mode == SUPER_PROPERTY) {
To me it looks like this if can be skipped since this code path is not specific
to super properties, it is just very hard to get here without super (you would
have to use a proxy).

https://codereview.chromium.org/934463003/diff/20001/src/objects.cc#newcode3151
src/objects.cc:3151: if (!own_lookup.IsFound()) {
Is there an existing function that can be used here? I feel dirty doing all this
logic here.

https://codereview.chromium.org/934463003/diff/20001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/934463003/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:1413: // TODO(arv): This is broken
The runtime calls SetElementWithInterceptor. It does not do the
DefineOwnProperty on the instance. SetElementWithInterceptor only has one user,
namely StoreElementToSuper in runtime-classes.cc.

[arv (Not doing code reviews), 2015-02-17 22:15:02]

Elements works

[arv (Not doing code reviews), 2015-02-17 22:24:43]

arv@chromium.org changed reviewers:
+ adamk@chromium.org

[arv (Not doing code reviews), 2015-02-17 22:34:25]

Fix arraysize

[arv (Not doing code reviews), 2015-02-17 22:40:48]

Use assertUnreachable instead of counter

[adamk, 2015-02-17 22:47:45]

https://codereview.chromium.org/934463003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/60001/src/objects.cc#newcode12884
src/objects.cc:12884: // is read-only (a declared const that has not been
initialized).  If a
Is this comment still providing value?

https://codereview.chromium.org/934463003/diff/60001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/934463003/diff/60001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:1413: // TODO(arv): This is broken
Is this a stray TODO?

https://codereview.chromium.org/934463003/diff/60001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:1414: (function
TestKeyedNumericSetterCreatingOwnProperties() {
Is there any reason this test needs to duplicate all of the code in the keyed
named property version? I'd think you could re-use exactly the same code for
elements and named properties.

[arv (Not doing code reviews), 2015-02-17 22:55:05]

https://codereview.chromium.org/934463003/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/60001/src/objects.cc#newcode12884
src/objects.cc:12884: // is read-only (a declared const that has not been
initialized).  If a
On 2015/02/17 22:47:45, adamk wrote:
> Is this comment still providing value?

I believe this is still the case. If there is a non-configurable, non-writable
property we still redefine it.

https://codereview.chromium.org/934463003/diff/60001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/934463003/diff/60001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:1413: // TODO(arv): This is broken
On 2015/02/17 22:47:45, adamk wrote:
> Is this a stray TODO?

Done.

https://codereview.chromium.org/934463003/diff/60001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:1414: (function
TestKeyedNumericSetterCreatingOwnProperties() {
On 2015/02/17 22:47:45, adamk wrote:
> Is there any reason this test needs to duplicate all of the code in the keyed
> named property version? I'd think you could re-use exactly the same code for
> elements and named properties.

That seems doable. Let me do that.

[arv (Not doing code reviews), 2015-02-17 23:48:04]

I think this is pretty broken at the moment. I'll have to look at it more
tomorrow. I'll ping when this is ready for review again.

[arv (Not doing code reviews), 2015-02-18 00:42:23]

take 2

[arv (Not doing code reviews), 2015-02-18 00:57:45]

Cleanup error messaging a bit

[arv (Not doing code reviews), 2015-02-18 01:00:04]

PTAL

I believe this is doing the right thing for all these crazy edge cases.

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode3174
src/objects.cc:3174: NewTypeError("redefine_disallowed",
Maybe I should just use strict_read_only_property here as well?

[adamk, 2015-02-18 01:26:05]

This is looking good to me, but I definitely think that Toon should take a look
at this. In particular, that big if clause is getting ugly, and the switch to
passing DEFINE_PROPERTY in from SetElementWithReceiver seems like a signficant
structural change.

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode3174
src/objects.cc:3174: NewTypeError("redefine_disallowed",
On 2015/02/18 01:00:04, arv wrote:
> Maybe I should just use strict_read_only_property here as well?

Or just make a new RedefineNonconfigurableProperty() method and factor the logic
out into that?

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode1...
src/objects.cc:12880: details.IsReadOnly() && details.kind() == kData) ||
I take it we don't need this !IsTheHole() check in the DEFINE_PROPERTY case
simply because we don't exercise that code path?

[Toon Verwaest, 2015-02-18 13:54:08]

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode3149
src/objects.cc:3149: if (data_store_mode == SUPER_PROPERTY) {
Given that this flag just switches half of the implementation of this method,
what about removing the flag and instead extracting the code above into a
separate helper function used by both SetProperty and SetSuperProperty?

The code below is a bit scary. Can you please replace it with switch
(it->state()) { ... } without "default:" to ensure all possible LookupIterator
states are handled? That would make it obvious what e.g., has to happen in the
ACCESS_CHECK case.

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode1...
src/objects.cc:12880: details.IsReadOnly() && details.kind() == kData) ||
details.kind() == kData isn't necessary since DEFINE + !Configurable + Accessor
is already handled above.

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode1...
src/objects.cc:12880: details.IsReadOnly() && details.kind() == kData) ||
On 2015/02/18 01:26:05, adamk wrote:
> I take it we don't need this !IsTheHole() check in the DEFINE_PROPERTY case
> simply because we don't exercise that code path?

We could add a DCHECK(element->IsTheHole()) inside the branch...

[arv (Not doing code reviews), 2015-02-18 16:44:52]

On 2015/02/18 01:26:05, adamk wrote:
> ... and the switch to
> passing DEFINE_PROPERTY in from SetElementWithReceiver seems like a signficant
> structural change.

SetElementWithReceiver is only ever called from store super. Also, using
DEFINE_PROPERTY is more aligned with the spec.

[arv (Not doing code reviews), 2015-02-18 17:43:58]

cleanup

[arv (Not doing code reviews), 2015-02-18 17:46:14]

Thanks.

PTAL

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode3174
src/objects.cc:3174: NewTypeError("redefine_disallowed",
On 2015/02/18 01:26:05, adamk wrote:
> On 2015/02/18 01:00:04, arv wrote:
> > Maybe I should just use strict_read_only_property here as well?
> 
> Or just make a new RedefineNonconfigurableProperty() method and factor the
logic
> out into that?

Done.

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode1...
src/objects.cc:12880: details.IsReadOnly() && details.kind() == kData) ||
On 2015/02/18 13:54:08, Toon Verwaest wrote:
> On 2015/02/18 01:26:05, adamk wrote:
> > I take it we don't need this !IsTheHole() check in the DEFINE_PROPERTY case
> > simply because we don't exercise that code path?
> 
> We could add a DCHECK(element->IsTheHole()) inside the branch...

Done.

https://codereview.chromium.org/934463003/diff/100001/src/objects.cc#newcode1...
src/objects.cc:12880: details.IsReadOnly() && details.kind() == kData) ||
On 2015/02/18 13:54:08, Toon Verwaest wrote:
> details.kind() == kData isn't necessary since DEFINE + !Configurable +
Accessor
> is already handled above.

Done.

[adamk, 2015-02-18 18:39:07]

lgtm % whitespace

https://codereview.chromium.org/934463003/diff/140001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/140001/src/objects.cc#newcode3191
src/objects.cc:3191: 
Nit: extra blank line?

https://codereview.chromium.org/934463003/diff/140001/src/objects.cc#newcode3202
src/objects.cc:3202: 
Ditto

[arv (Not doing code reviews), 2015-02-18 18:40:27]

New patchsets have been uploaded after l-g-t-m from adamk@chromium.org

[arv (Not doing code reviews), 2015-02-18 18:40:28]

Remove extra newlines

[arv (Not doing code reviews), 2015-02-18 19:09:47]

Fix issue with details of non found property

[Toon Verwaest, 2015-02-19 05:32:19]

lgtm with nit. Thanks, that looks a lot more readable (and correct) now.

https://codereview.chromium.org/934463003/diff/180001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/934463003/diff/180001/src/objects.cc#newcode3207
src/objects.cc:3207: case LookupIterator::TRANSITION: {
TRANSITION should be UNREACHABLE()

[arv (Not doing code reviews), 2015-02-19 15:31:59]

New patchsets have been uploaded after l-g-t-m from verwaest@chromium.org

[arv (Not doing code reviews), 2015-02-19 15:32:00]

Add UNREACHABLE

[arv (Not doing code reviews), 2015-02-19 15:50:14]

git rebase

[arv (Not doing code reviews), 2015-02-19 15:50:28]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2015-02-19 15:51:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/934463003/220001

[commit-bot: I haz the power, 2015-02-19 16:15:31]

Committed patchset #13 (id:220001)

[commit-bot: I haz the power, 2015-02-19 16:15:46]

Patchset 13 (id:??) landed as
https://crrev.com/0cffc08b667a067089c7eecffea3e8021ab2ff57
Cr-Commit-Position: refs/heads/master@{#26754}