Classes: Add basic support for properties

Classes: Add basic support for properties

This adds the properties to the prototype and the constructor.

BUG=v8:3330
LOG=Y
R=dslomov@chromium.org

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

[arv (Not doing code reviews), 2014-10-27 19:17:45]

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

[arv (Not doing code reviews), 2014-10-27 19:17:46]

PTAL

[Dmitry Lomov (no reviews), 2014-10-27 21:37:14]

Comments

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2516: __ push(scratch);
Add a comment about why it is ok to skip access check here

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2525: __ ldr(ip, MemOperand(sp, kPointerSize));  //
constructor
Nit: use scratch register instead of 'ip' here

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2572: context()->Plug(r0);
The caller plugs context, no need to do it here.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2188: __ Push(scratch);
Comment on why access check is not needed.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2197: __ Peek(x1, kPointerSize);  // constructor
Nit: Use scratch register here.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2244: context()->Plug(x0);
The caller plugs context, no need to do this here.

https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h#newcode567
src/full-codegen.h:567: // Expects the class (function) in the accumulator.
Nit: add a comment that accumulator contains constructor after the function

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2430: __ mov(scratch, FieldOperand(eax,
JSFunction::kPrototypeOrInitialMapOffset));
Access check comment.

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2481: context()->Plug(eax);
Caller plugs context; no need to do it here

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2428: Register scratch = rcx;
Nit: Why rcx here, but ebx in ia32?

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2429: __ movp(scratch, FieldOperand(rax,
JSFunction::kPrototypeOrInitialMapOffset));
Access check comment.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2480: context()->Plug(rax);
Caller plugs context

https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes.js
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes...
test/mjsunit/harmony/classes.js:180: 
Additional tests: 
1. Test that 'DefineProperty' is not a store (extend a class that has
getter/setter, define method with same name)
2. Test property descriptors of things that are not __proto__.
3. Tests with super calls (do I understand correctly though that these do not
work yet since home object is not installed on methods?)

[arv (Not doing code reviews), 2014-10-27 22:20:53]

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2428: Register scratch = rcx;
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Nit: Why rcx here, but ebx in ia32? 

Because I don't know enough about the different registers on the different
platforms. I'll change it.

https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes.js
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes...
test/mjsunit/harmony/classes.js:180: 
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Additional tests: 
> 1. Test that 'DefineProperty' is not a store (extend a class that has
> getter/setter, define method with same name)

I thought I tested that but I guess my tests where just manual tests... I'll add
tests.

> 2. Test property descriptors of things that are not __proto__.

Will add.

> 3. Tests with super calls (do I understand correctly though that these do not
> work yet since home object is not installed on methods?)

I was planning on adding that in a follow up CL. I'll expand the CL
descriptions.

[arv (Not doing code reviews), 2014-10-28 09:41:45]

Code review updates

[arv (Not doing code reviews), 2014-10-28 09:42:01]

PTAL

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2516: __ push(scratch);
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Add a comment about why it is ok to skip access check here

Done.

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2525: __ ldr(ip, MemOperand(sp, kPointerSize));  //
constructor
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Nit: use scratch register instead of 'ip' here

Done.

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2572: context()->Plug(r0);
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> The caller plugs context, no need to do it here.

Done.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2188: __ Push(scratch);
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Comment on why access check is not needed.

Done.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2197: __ Peek(x1, kPointerSize);  // constructor
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Nit: Use scratch register here.

Done.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2244: context()->Plug(x0);
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> The caller plugs context, no need to do this here.

Done.

https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h#newcode567
src/full-codegen.h:567: // Expects the class (function) in the accumulator.
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Nit: add a comment that accumulator contains constructor after the function

Done.

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2430: __ mov(scratch, FieldOperand(eax,
JSFunction::kPrototypeOrInitialMapOffset));
On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> Access check comment.

Done.

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2481: context()->Plug(eax);
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Caller plugs context; no need to do it here

Done.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2428: Register scratch = rcx;
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Nit: Why rcx here, but ebx in ia32? 

Done.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2429: __ movp(scratch, FieldOperand(rax,
JSFunction::kPrototypeOrInitialMapOffset));
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Access check comment.

Done.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2480: context()->Plug(rax);
On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> Caller plugs context

Done.

[Dmitry Lomov (no reviews), 2014-10-28 10:34:09]

lgtm

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2516: __ push(scratch);
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > Add a comment about why it is ok to skip access check here
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2525: __ ldr(ip, MemOperand(sp, kPointerSize));  //
constructor
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > Nit: use scratch register instead of 'ip' here
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/arm/full-codegen-arm.cc#...
src/arm/full-codegen-arm.cc:2572: context()->Plug(r0);
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > The caller plugs context, no need to do it here.
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64.cc
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2188: __ Push(scratch);
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > Comment on why access check is not needed.
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2197: __ Peek(x1, kPointerSize);  // constructor
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > Nit: Use scratch register here.
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/arm64/full-codegen-arm64...
src/arm64/full-codegen-arm64.cc:2244: context()->Plug(x0);
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > The caller plugs context, no need to do this here.
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/680993003/diff/1/src/full-codegen.h#newcode567
src/full-codegen.h:567: // Expects the class (function) in the accumulator.
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > Nit: add a comment that accumulator contains constructor after the function
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2430: __ mov(scratch, FieldOperand(eax,
JSFunction::kPrototypeOrInitialMapOffset));
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:13, Dmitry Lomov (chromium) wrote:
> > Access check comment.
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:2481: context()->Plug(eax);
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> > Caller plugs context; no need to do it here
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2428: Register scratch = rcx;
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> > Nit: Why rcx here, but ebx in ia32? 
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2429: __ movp(scratch, FieldOperand(rax,
JSFunction::kPrototypeOrInitialMapOffset));
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> > Access check comment.
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:2480: context()->Plug(rax);
On 2014/10/28 09:42:00, arv wrote:
> On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> > Caller plugs context
> 
> Done.

Acknowledged.

https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes.js
File test/mjsunit/harmony/classes.js (right):

https://codereview.chromium.org/680993003/diff/1/test/mjsunit/harmony/classes...
test/mjsunit/harmony/classes.js:180: 
On 2014/10/27 22:20:53, arv wrote:
> On 2014/10/27 21:37:14, Dmitry Lomov (chromium) wrote:
> > Additional tests: 
> > 1. Test that 'DefineProperty' is not a store (extend a class that has
> > getter/setter, define method with same name)
> 
> I thought I tested that but I guess my tests where just manual tests... I'll
add
> tests.
> 
> > 2. Test property descriptors of things that are not __proto__.
> 
> Will add.
> 
> > 3. Tests with super calls (do I understand correctly though that these do
not
> > work yet since home object is not installed on methods?)
> 
> I was planning on adding that in a follow up CL. I'll expand the CL
> descriptions.

Acknowledged.

[arv (Not doing code reviews), 2014-10-28 12:27:42]

git rebase

[arv (Not doing code reviews), 2014-10-28 12:43:19]

Committed patchset #3 (id:40001) manually as 24934 (presubmit successful).