Implement loads and calls from 'super'

Implement loads and calls from 'super'

R=verwaest@chromium.org, arv@chromium.org
BUG=v8:3330
LOG=N

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

[Dmitry Lomov (no reviews), 2014-09-02 14:55:01]

This implements named loads and named calls from super. 
Only ia32 for now, please take a look before I start on the platform ports.

[arv (Not doing code reviews), 2014-09-09 22:38:46]

Looks good as far as I can tell.

Toon need to look at this too since this is still  outside my comfort zone.

https://codereview.chromium.org/527963002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/527963002/diff/1/src/compiler.cc#newcode409
src/compiler.cc:409: // TODO(turbofan): Make super work and remove this bailour.
typo

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

https://codereview.chromium.org/527963002/diff/1/test/mjsunit/harmony/super.j...
test/mjsunit/harmony/super.js:71: return this._x;
maybe assert that this === derived

var derived = new Derived();
assertEquals('derived', derived.testGetter());

https://codereview.chromium.org/527963002/diff/1/test/mjsunit/harmony/super.j...
test/mjsunit/harmony/super.js:85: }.toMethod(Derived.prototype);
wrong indentation

[Toon Verwaest, 2014-09-15 11:57:21]

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

https://codereview.chromium.org/527963002/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1293: __ j(equal, &super_lookup_failure);
What about

j(not_equal, &done)
push(Immediate(...empty_string()));
CallRuntime(...ThrowReferenceError...);
bind(&done);
mov(eax, ...Map...);
mov(eax, ...Prototype...);
context()->Plug(eax)

https://codereview.chromium.org/527963002/diff/1/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/527963002/diff/1/src/lookup.cc#newcode292
src/lookup.cc:292: if (GetHolder<JSObject>()->IsJSGlobalProxy() &&
Wth? Why do you explicitly avoid access checks on everything that's not a
JSGlobalProxy? Sounds like a security issue right there. E.g.:

o = {f:function() { return super.hasOwnProperty.constructor; }};
o.f[home_object] = otherwindow.location;
otherwindow_Function = o.f();

https://codereview.chromium.org/527963002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/527963002/diff/1/src/runtime.cc#newcode2083
src/runtime.cc:2083: it.LookupForRead();
I don't understand why you need this weird setup. Why not just do
Object::GetProperty(&it) immediately?

[Dmitry Lomov (no reviews), 2014-09-15 12:31:12]

CR comments addressed.

https://codereview.chromium.org/527963002/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/527963002/diff/1/src/compiler.cc#newcode409
src/compiler.cc:409: // TODO(turbofan): Make super work and remove this bailour.
On 2014/09/09 22:38:46, arv wrote:
> typo

Done.

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

https://codereview.chromium.org/527963002/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1293: __ j(equal, &super_lookup_failure);
On 2014/09/15 11:57:20, Toon Verwaest wrote:
> What about
> 
> j(not_equal, &done)
> push(Immediate(...empty_string()));
> CallRuntime(...ThrowReferenceError...);
> bind(&done);
> mov(eax, ...Map...);
> mov(eax, ...Prototype...);
> context()->Plug(eax)

Done.

https://codereview.chromium.org/527963002/diff/1/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/527963002/diff/1/src/lookup.cc#newcode292
src/lookup.cc:292: if (GetHolder<JSObject>()->IsJSGlobalProxy() &&
On 2014/09/15 11:57:20, Toon Verwaest wrote:
> Wth? Why do you explicitly avoid access checks on everything that's not a
> JSGlobalProxy? Sounds like a security issue right there. E.g.:
> 
> o = {f:function() { return super.hasOwnProperty.constructor; }};
> o.f[home_object] = otherwindow.location;
> otherwindow_Function = o.f();

As discussed offline, this returns otherwise, so doesn't really avoid access
checks (it is also just a copy from ic.cc), but it looks like I do not need this
anyway

https://codereview.chromium.org/527963002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/527963002/diff/1/src/runtime.cc#newcode2083
src/runtime.cc:2083: it.LookupForRead();
On 2014/09/15 11:57:20, Toon Verwaest wrote:
> I don't understand why you need this weird setup. Why not just do
> Object::GetProperty(&it) immediately?

Done.

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

https://codereview.chromium.org/527963002/diff/1/test/mjsunit/harmony/super.j...
test/mjsunit/harmony/super.js:71: return this._x;
On 2014/09/09 22:38:46, arv wrote:
> maybe assert that this === derived
> 
> var derived = new Derived();
> assertEquals('derived', derived.testGetter());

Done.

https://codereview.chromium.org/527963002/diff/1/test/mjsunit/harmony/super.j...
test/mjsunit/harmony/super.js:85: }.toMethod(Derived.prototype);
On 2014/09/09 22:38:46, arv wrote:
> wrong indentation

Done.

[Toon Verwaest, 2014-09-15 13:16:43]

... and as discussed there's a missing access check when walking the prototype
chain of the home object (e.g., would look beyond a window to which we don't
have access)

https://codereview.chromium.org/527963002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/527963002/diff/20001/src/runtime.cc#newcode2084
src/runtime.cc:2084: if (it.IsFound()) {
You don't need this it.IsFound() either... GetProperty will return undefined for
you.

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

https://codereview.chromium.org/527963002/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:72: assertTrue(this === derived);
assertSame

[Dmitry Lomov (no reviews), 2014-09-15 13:18:26]

https://codereview.chromium.org/527963002/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/527963002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1298: __ mov(eax, FieldOperand(eax,
Map::kPrototypeOffset));
As discussed offline, this skips the access check on prototype load. Oh well.
LoadFromSuper should take (home_object, this_value, name) as args.

[arv (Not doing code reviews), 2014-09-15 15:22:57]

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

https://codereview.chromium.org/527963002/diff/1/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:1299: __
push(Immediate(isolate()->factory()->empty_string()));
Can we do a better error message than an empty string?

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

https://codereview.chromium.org/527963002/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:87: }.toMethod(Derived.prototype);
Still wrong indentation. function bodies indent 2 spaces, even when used as part
of another expression/statement.

Derived.prototype.testGetter = function() {
  return super.x;
}.toMethod(Derived.prototype);

[Dmitry Lomov (no reviews), 2014-09-16 13:15:56]

PTAL. Added access checks & addressed other feedback.
Last check before platform ports.

https://codereview.chromium.org/527963002/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/527963002/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:1298: __ mov(eax, FieldOperand(eax,
Map::kPrototypeOffset));
On 2014/09/15 13:18:25, Dmitry Lomov (chromium) wrote:
> As discussed offline, this skips the access check on prototype load. Oh well.
> LoadFromSuper should take (home_object, this_value, name) as args.

Done.

https://codereview.chromium.org/527963002/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/527963002/diff/20001/src/runtime.cc#newcode2084
src/runtime.cc:2084: if (it.IsFound()) {
On 2014/09/15 13:16:43, Toon Verwaest wrote:
> You don't need this it.IsFound() either... GetProperty will return undefined
for
> you.

Done.

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

https://codereview.chromium.org/527963002/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:72: assertTrue(this === derived);
On 2014/09/15 13:16:43, Toon Verwaest wrote:
> assertSame

Done.

https://codereview.chromium.org/527963002/diff/20001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:87: }.toMethod(Derived.prototype);
On 2014/09/15 15:22:57, arv wrote:
> Still wrong indentation. function bodies indent 2 spaces, even when used as
part
> of another expression/statement.
> 
> Derived.prototype.testGetter = function() {
>   return super.x;
> }.toMethod(Derived.prototype);

Done.

[Toon Verwaest, 2014-09-16 13:56:16]

lgtm

https://codereview.chromium.org/527963002/diff/60001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/527963002/diff/60001/src/runtime.cc#newcode2239
src/runtime.cc:2239: // Define or redefine own pro//perty.
remove //

[Dmitry Lomov (no reviews), 2014-09-17 08:59:41]

dslomov@chromium.org changed reviewers:
+ ulan@chromium.org

[Dmitry Lomov (no reviews), 2014-09-17 08:59:42]

+ x64, arm, arm64 - PTAL.

ulan@ - please take a look at arm/arm64 - many thanks!

[arv (Not doing code reviews), 2014-09-17 16:49:51]

test/mjsunit/runtime-gen/loadfromsuper.js and tools/generate-runtime-tests.py
have been removed.

[arv (Not doing code reviews), 2014-09-17 16:50:16]

On 2014/09/17 at 16:49:51, arv wrote:
> test/mjsunit/runtime-gen/loadfromsuper.js and tools/generate-runtime-tests.py
have been removed.

I meant to say that they should be removed since the runtime tests have been
removed.

[Dmitry Lomov (no reviews), 2014-09-17 17:34:24]

On 2014/09/17 16:50:16, arv wrote:
> On 2014/09/17 at 16:49:51, arv wrote:
> > test/mjsunit/runtime-gen/loadfromsuper.js and
tools/generate-runtime-tests.py
> have been removed.
> 
> I meant to say that they should be removed since the runtime tests have been
> removed.

Sorry, I do not understand: there is a fuzzer seed test for
Runtime_LoadFromSuper and it is valid (but not for the two Runtime_Throw guys
that I have already added)

[arv (Not doing code reviews), 2014-09-17 17:37:57]

On 2014/09/17 at 17:34:24, dslomov wrote:
> On 2014/09/17 16:50:16, arv wrote:
> > On 2014/09/17 at 16:49:51, arv wrote:
> > > test/mjsunit/runtime-gen/loadfromsuper.js and
tools/generate-runtime-tests.py
> > have been removed.
> > 
> > I meant to say that they should be removed since the runtime tests have been
> > removed.
> 
> Sorry, I do not understand: there is a fuzzer seed test for
Runtime_LoadFromSuper and it is valid (but not for the two Runtime_Throw guys
that I have already added)

test/mjsunit/runtime-gen/ was removed in
https://code.google.com/p/v8/source/detail?r=23800

[Dmitry Lomov (no reviews), 2014-09-17 17:41:36]

On 2014/09/17 17:37:57, arv wrote:
> On 2014/09/17 at 17:34:24, dslomov wrote:
> > On 2014/09/17 16:50:16, arv wrote:
> > > On 2014/09/17 at 16:49:51, arv wrote:
> > > > test/mjsunit/runtime-gen/loadfromsuper.js and
> tools/generate-runtime-tests.py
> > > have been removed.
> > > 
> > > I meant to say that they should be removed since the runtime tests have
been
> > > removed.
> > 
> > Sorry, I do not understand: there is a fuzzer seed test for
> Runtime_LoadFromSuper and it is valid (but not for the two Runtime_Throw guys
> that I have already added)
> 
> test/mjsunit/runtime-gen/ was removed in
> https://code.google.com/p/v8/source/detail?r=23800

Oops I see :) Thanks for the heads-up!
I didn't rebase for your convenience ;)))

[Dmitry Lomov (no reviews), 2014-09-18 11:56:45]

PTAL.

Ulan suggested offline a nice restructuring for Emit* methods in
full-codegen-<paltform>.cc - it is now much nicer - thanks!
Also added comments describing the stack layout in EmitSuperCallWithLoadIC.

[Toon Verwaest, 2014-09-19 09:36:44]

still lgtm

[Dmitry Lomov (no reviews), 2014-09-19 10:57:35]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2014-09-19 10:57:36]

The CQ bit was unchecked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2014-09-19 11:08:19]

Committed patchset #8 (id:140001) manually as 24078 (presubmit successful).

[ulan, 2014-09-19 11:10:54]

I looked at arm, arm64, x64 ports. They lgtm.