[es6] Support super.property in eval and arrow functions

[es6] Support super.property in eval and arrow functions

When we enter a method that needs access to the [[HomeObject]]
we allocate a local variable `.home_object` and assign it the
value from the [[HomeObject]] private symbol. Something along
the lines of:

  method() {
    var .home_object = %ThisFunction()[home_object_symbol];
    ...
  }

BUG=v8:3867, v8:4031
LOG=N

Committed: https://crrev.com/44e9810345cea9bfd6861905bc6856db7db5a25c
Cr-Commit-Position: refs/heads/master@{#28644}

[arv (Not doing code reviews), 2015-05-12 15:55:20]

arv@chromium.org changed reviewers:
+ caitpotter88@gmail.com, dslomov@chromium.org, wingo@igalia.com

[arv (Not doing code reviews), 2015-05-12 15:55:20]

This works but could be improved.

- This uses a runtime function to load the [[HomeObject]] into a local variable
when a method is entered (similar to the arguments variable). I should change it
to generate assembly instead.

- SuperReference now visits 2 variables. One for `this` and one for
`.home_object`. The problem with that is that we use SuperReference both for
super.property and super() and super() does not depend on the home object so I
had to still check scope uses_super_property. It might be cleaner/better to
introduce 2 different AST nodes.

We still need to track use_super_property since we need to install the home
object on a lazily parsed function (in classes and object ltiterals).

It is not clear if I should introduce a new Variable::Kind value. The only use I
can see is for a possible dcheck.

---

Alternative approach that mostly worked but I gave up on:

I initially tried to solve this problem by having the parser inject the
assignment to the `.home_object` var. It works except that it adds extra runtime
costs when it isn't needed but the thing that convinced me to ive up in that
approach was that it created another entry when stepping through a method in the
debugger.... strange.

---

Anyway, I would love some feedback before your EOD if possible so I can finish
this while you are sleeping.

[Dmitry Lomov (no reviews), 2015-05-12 16:44:47]

The approach looks good to me overall.
It will be nice to keep the LoadIC for home object instead of using a runtime
function.

I do not think a new Variable::Kind is warranted (for DCHECK, you can just
compare the name).

AST node split between super() and super.prop seems reasonable to me as well.

[arv (Not doing code reviews), 2015-05-12 19:58:28]

Try feedback slot

[arv (Not doing code reviews), 2015-05-12 20:00:51]

On 2015/05/12 19:58:28, arv wrote:
> Try feedback slot

I'm having issues with the ast-numbering/feedback slot.

The numbering happens too late so by the time FullCodeGenerator::Generate is
called the feedback slot is still invalid.

[wingo, 2015-05-13 06:21:33]

https://codereview.chromium.org/1135243004/diff/20001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/1135243004/diff/20001/src/ast-numbering.cc#ne...
src/ast-numbering.cc:489: ReserveFeedbackSlots(node);
Could it be that this needs to go in AstNumberingVisitor::Renumber?  Doing it
here is for functions nested in the function being compiled.  For the function
being compiled, Renumber() is the place.  Kinda confusing :)

[arv (Not doing code reviews), 2015-05-13 15:05:41]

i have no idea

[arv (Not doing code reviews), 2015-05-13 15:16:44]

Andy, I realize that I don't understand this. Any hints?

https://codereview.chromium.org/1135243004/diff/40001/src/ast-numbering.cc
File src/ast-numbering.cc (right):

https://codereview.chromium.org/1135243004/diff/40001/src/ast-numbering.cc#ne...
src/ast-numbering.cc:515: ReserveFeedbackSlots(node);
I'm confused by this. It passes my super tests but trigger a lot of other
asserts all over.

[arv (Not doing code reviews), 2015-05-13 15:44:01]

On 2015/05/13 15:16:44, arv wrote:
> Andy, I realize that I don't understand this. Any hints?
> 
> https://codereview.chromium.org/1135243004/diff/40001/src/ast-numbering.cc
> File src/ast-numbering.cc (right):
> 
>
https://codereview.chromium.org/1135243004/diff/40001/src/ast-numbering.cc#ne...
> src/ast-numbering.cc:515: ReserveFeedbackSlots(node);
> I'm confused by this. It passes my super tests but trigger a lot of other
> asserts all over.

Actually, it seems fine. The tests need to be updated to take into account that
all functions now have 1 ic slot to start with.

[arv (Not doing code reviews), 2015-05-13 16:04:27]

Update tests

[arv (Not doing code reviews), 2015-05-13 16:05:45]

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

[arv (Not doing code reviews), 2015-05-13 16:05:46]

Adding Ben since... IC

[arv (Not doing code reviews), 2015-05-13 16:11:14]

arv@chromium.org changed reviewers:
- caitpotter88@gmail.com

[arv (Not doing code reviews), 2015-05-13 16:11:14]

arv@chromium.org changed required reviewers:
+ wingo@igalia.com

[arv (Not doing code reviews), 2015-05-13 16:11:53]

No longer WIP. I'll start the other ports today.

[arv (Not doing code reviews), 2015-05-13 17:09:51]

cleanup

[arv (Not doing code reviews), 2015-05-13 20:18:45]

All ports done

[arv (Not doing code reviews), 2015-05-13 20:19:36]

PTAL

Now this is only gated on Andy's CL staying in the tree...

[arv (Not doing code reviews), 2015-05-13 21:15:12]

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

[arv (Not doing code reviews), 2015-05-13 21:15:13]

Thanks Ben.

Toon, could you look at the IC stuff?

[mvstanton, 2015-05-15 09:48:56]

mvstanton@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2015-05-15 09:48:57]

I've reviewed the feedback vector stuff, and it looks good. Yes, it's
unfortunate that tests have to magically know which IC slot to query. Danno
proposed a runtime intrinsic that would record the ic slot you are interested
in, then these magic numbers could be eliminated, something like:

int index_to_slot;
CompileRun("function foo(a) {"
           "  index_to_slot = %RecordNextSlot();"
           "  return a[i];");
// Now you can query the feedback vector at index_to_slot.

I should probably do this work before our tests accumulate more magic numbers...

[wingo, 2015-05-15 14:59:35]

for what it's worth, lgtm

https://codereview.chromium.org/1135243004/diff/100001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1135243004/diff/100001/src/preparser.h#newcod...
src/preparser.h:3386: while (scope->is_eval_scope() || scope->is_arrow_scope())
{
Still makes sense to me to factor this out to a method on Scope, or to add an
optional arg to DeclarationScope or something.

https://codereview.chromium.org/1135243004/diff/100001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-literals-super.js (right):

https://codereview.chromium.org/1135243004/diff/100001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-literals-super.js:145: eval() {
This eval is not in the scope of the method?  Strange; some day I'll understand
all of ES6...

[arv (Not doing code reviews), 2015-05-15 15:11:52]

https://codereview.chromium.org/1135243004/diff/100001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-literals-super.js (right):

https://codereview.chromium.org/1135243004/diff/100001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-literals-super.js:145: eval() {
On 2015/05/15 14:59:35, wingo wrote:
> This eval is not in the scope of the method?  Strange; some day I'll
understand
> all of ES6...

The method name does not create a binding. The motivation was that having a
binding to the method would not be correct since the method does not work
without being invoked with the correct this.

  method() {}

is thus similar to

  method: function() {}

and not

  method: function method() {
    assert(this instanceof Something)
    method();  // will fail
    method.call(this);  // ok, ignoring the iloop
    this.method();  // is better 
  }

[arv (Not doing code reviews), 2015-05-20 21:13:34]

rebase

[arv (Not doing code reviews), 2015-05-20 22:38:34]

I've cleanly applied the patch now.

There is some problem related to call spread.
test/mjsunit/harmony/spread-call-super-property.js is failing now.

I'm also seeing an invalid BailoutId in the same test with TF.

I'll look closer tomorrow.

[arv (Not doing code reviews), 2015-05-21 17:56:20]

On 2015/05/20 22:38:34, arv wrote:
> I've cleanly applied the patch now.
> 
> There is some problem related to call spread.
> test/mjsunit/harmony/spread-call-super-property.js is failing now.
> 
> I'm also seeing an invalid BailoutId in the same test with TF.
> 
> I'll look closer tomorrow.

Caitlin, it actually seems like it is the rest params that makes this fail.

[caitp (gmail), 2015-05-21 18:12:35]

On 2015/05/21 17:56:20, arv wrote:
> On 2015/05/20 22:38:34, arv wrote:
> > I've cleanly applied the patch now.
> > 
> > There is some problem related to call spread.
> > test/mjsunit/harmony/spread-call-super-property.js is failing now.
> > 
> > I'm also seeing an invalid BailoutId in the same test with TF.
> > 
> > I'll look closer tomorrow.
> 
> Caitlin, it actually seems like it is the rest params that makes this fail.

The invalid BailoutId or the other thing?

IIRC we're doing some finicky stuff because of newTarget being pushed on the
stack, (so we change the restIndex depending on if newTarget is pushed or not).
Did you make any changes to that code?

[arv (Not doing code reviews), 2015-05-21 19:42:32]

On 2015/05/21 18:12:35, caitp wrote:
> On 2015/05/21 17:56:20, arv wrote:
> > On 2015/05/20 22:38:34, arv wrote:
> > > I've cleanly applied the patch now.
> > > 
> > > There is some problem related to call spread.
> > > test/mjsunit/harmony/spread-call-super-property.js is failing now.
> > > 
> > > I'm also seeing an invalid BailoutId in the same test with TF.
> > > 
> > > I'll look closer tomorrow.
> > 
> > Caitlin, it actually seems like it is the rest params that makes this fail.
> 
> The invalid BailoutId or the other thing?

The other thing but iI think they are related. Reading the [[HomeObject]] should
never fail so it should be OK to use Bailout ID NONE.
>
> IIRC we're doing some finicky stuff because of newTarget being pushed on the
> stack, (so we change the restIndex depending on if newTarget is pushed or
not).
> Did you make any changes to that code?

I don't think that is it... see next message coming a seconed after this one

[arv (Not doing code reviews), 2015-05-21 19:42:42]

https://codereview.chromium.org/1135243004/diff/120001/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1135243004/diff/120001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:309: __ movp(LoadDescriptor::ReceiverRegister(),
rdi);
Caitlin, I think the problem is that rdi no longer holds the function. I think I
saw that when I was stepping through (I'll have to verify next week). What is
strange is that if the rest params somehow causes this then I would assume
arguments would also have problems?

[caitp (gmail), 2015-05-21 19:47:50]

caitpotter88@gmail.com changed reviewers:
+ caitpotter88@gmail.com

[caitp (gmail), 2015-05-21 19:47:51]

https://codereview.chromium.org/1135243004/diff/120001/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1135243004/diff/120001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:309: __ movp(LoadDescriptor::ReceiverRegister(),
rdi);
On 2015/05/21 19:42:42, arv wrote:
> Caitlin, I think the problem is that rdi no longer holds the function. I think
I
> saw that when I was stepping through (I'll have to verify next week). What is
> strange is that if the rest params somehow causes this then I would assume
> arguments would also have problems?

interesting --- so, I would expect `rdi` to be saved when invoking the runtime
function. Is that not the case?

Anyways, try pushing `rdi` before the call, and then restoring it after?

[caitp (gmail), 2015-05-21 20:32:35]

https://codereview.chromium.org/1135243004/diff/120001/src/x64/full-codegen-x...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/1135243004/diff/120001/src/x64/full-codegen-x...
src/x64/full-codegen-x64.cc:309: __ movp(LoadDescriptor::ReceiverRegister(),
rdi);
On 2015/05/21 19:47:51, caitp wrote:
> On 2015/05/21 19:42:42, arv wrote:
> > Caitlin, I think the problem is that rdi no longer holds the function. I
think
> I
> > saw that when I was stepping through (I'll have to verify next week). What
is
> > strange is that if the rest params somehow causes this then I would assume
> > arguments would also have problems?
> 
> interesting --- so, I would expect `rdi` to be saved when invoking the runtime
> function. Is that not the case?
> 
> Anyways, try pushing `rdi` before the call, and then restoring it after?

TIL rdi is not caller-saved on x86/x86_64. Maybe in this particular calling
convention, it's not callee-saved either (or maybe in optimized builds, the
compiler isn't able to figure out that rdi needs to be saved, so it doesn't?)

Probably manually saving the register is the only option :|

[arv (Not doing code reviews), 2015-05-26 16:17:49]

release mode works

[arv (Not doing code reviews), 2015-05-26 17:04:44]

Fix bailout id none assertion

[arv (Not doing code reviews), 2015-05-26 17:10:45]

cleanup

[arv (Not doing code reviews), 2015-05-26 17:17:47]

arv@chromium.org changed reviewers:
+ mstarzinger@chromium.org
- mvstanton@chromium.org, titzer@chromium.org, verwaest@chromium.org

[arv (Not doing code reviews), 2015-05-26 17:17:48]

PTAL

Michi, see comment in below. Is this what you were referring to last week?

https://codereview.chromium.org/1135243004/diff/180001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1135243004/diff/180001/src/ast.h#newcode2736
src/ast.h:2736: home_object_feedback_slot_ = slot;
Initially I removed this feedback slot but it seems like TF needs it to do
super.x since it creates a bailout based on the SuperReference. However, it
seems like TF does not handle SuperReference in VisitProperty. For the full
codegen, this bailout is not needed because we only have a runtime at this
point.

How does TF do super.prop?

[arv (Not doing code reviews), 2015-05-26 18:43:48]

TF should stack overflow since it does not support super.prop

[arv (Not doing code reviews), 2015-05-26 18:46:29]

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

[arv (Not doing code reviews), 2015-05-26 18:46:30]

PTAL

Adam, care to take a look since Dmitry is ooo this week?

https://codereview.chromium.org/1135243004/diff/180001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/1135243004/diff/180001/src/ast.h#newcode2736
src/ast.h:2736: home_object_feedback_slot_ = slot;
On 2015/05/26 17:17:48, arv wrote:
> Initially I removed this feedback slot but it seems like TF needs it to do
> super.x since it creates a bailout based on the SuperReference. However, it
> seems like TF does not handle SuperReference in VisitProperty. For the full
> codegen, this bailout is not needed because we only have a runtime at this
> point.
> 
> How does TF do super.prop?

I figured it out. TF does not support super. It calls SetStackOverflow() to
signal that it is not supported. I'm now doing this earlier in VisitProperty to
prevent the DCHECK.

[arv (Not doing code reviews), 2015-05-26 18:51:53]

git rebase

[adamk, 2015-05-26 19:32:33]

lgtm

The IC stuff is not my forte, obviously, but it looks like mvstanton already
signed off on that?

https://codereview.chromium.org/1135243004/diff/220001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1135243004/diff/220001/src/preparser.h#newcod...
src/preparser.h:344: DCHECK(scope_type != FUNCTION_SCOPE && scope_type !=
ARROW_SCOPE);
You can split this into two DCHECKs to make errors clearer.

https://codereview.chromium.org/1135243004/diff/220001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-literals-super.js (right):

https://codereview.chromium.org/1135243004/diff/220001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-literals-super.js:136: (function
TestSuperPropertyInEval() {
Name is not complete: "...InEvalAndArrow", maybe?

https://codereview.chromium.org/1135243004/diff/220001/test/mjsunit/harmony/s...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/1135243004/diff/220001/test/mjsunit/harmony/s...
test/mjsunit/harmony/super.js:2071: (function TestSuperPropertyInEval() {
Same name complaint.

[arv (Not doing code reviews), 2015-05-26 19:39:47]

Code review cleanup

[arv (Not doing code reviews), 2015-05-26 19:39:59]

https://codereview.chromium.org/1135243004/diff/220001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1135243004/diff/220001/src/preparser.h#newcod...
src/preparser.h:344: DCHECK(scope_type != FUNCTION_SCOPE && scope_type !=
ARROW_SCOPE);
On 2015/05/26 19:32:33, adamk wrote:
> You can split this into two DCHECKs to make errors clearer.

Done.

https://codereview.chromium.org/1135243004/diff/220001/test/mjsunit/harmony/o...
File test/mjsunit/harmony/object-literals-super.js (right):

https://codereview.chromium.org/1135243004/diff/220001/test/mjsunit/harmony/o...
test/mjsunit/harmony/object-literals-super.js:136: (function
TestSuperPropertyInEval() {
On 2015/05/26 19:32:33, adamk wrote:
> Name is not complete: "...InEvalAndArrow", maybe?

I split it into two functions instead.

[arv (Not doing code reviews), 2015-05-26 19:40:11]

The CQ bit was checked by arv@chromium.org

[arv (Not doing code reviews), 2015-05-26 19:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1135243004/#ps240001
(title: "Code review cleanup")

[commit-bot: I haz the power, 2015-05-26 19:40:26]

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

[commit-bot: I haz the power, 2015-05-26 20:29:54]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-05-26 20:30:06]

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