[es6] Make new.target work in functions

[es6] Make new.target work in functions

This makes new.target work in [[Call]] and [[Construct]] of ordinary
functions.

We achieve this by introducing a new construct stub for functions that
uses the new.target variable. The construct stub pushes the original
constructor just above the receiver in the construct frame.

BUG=v8:3887
LOG=N
R=adamk@chromium.org, dslomov@chromium.org

Committed: https://crrev.com/7a63bf77eb7610afdc1a968f7660781e5160ba8d
Cr-Commit-Position: refs/heads/master@{#29358}

[arv (Not doing code reviews), 2015-06-23 21:11:18]

Add arrow tests

[arv (Not doing code reviews), 2015-06-23 21:12:31]

PTAL

Any other edge cases that I should try at this point?

[adamk, 2015-06-23 21:40:17]

I don't see any obvious things missing from your test cases, this looks ready to
port to me.

[Dmitry Lomov (no reviews), 2015-06-24 09:50:34]

On 2015/06/23 21:40:17, adamk wrote:
> I don't see any obvious things missing from your test cases, this looks ready
to
> port to me.

+1

[arv (Not doing code reviews), 2015-06-25 17:08:52]

Trying to get arm and mips working

[arv (Not doing code reviews), 2015-06-25 17:12:43]

Mips porters, I'm having some issues with both mips and mips64. It seems like
this change should be straight forward.

Would you mind taking a look?

Thanks.

[arv (Not doing code reviews), 2015-06-25 17:14:28]

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

[paul.l..., 2015-06-25 18:48:19]

The stack layout is mixed up somewhere. I've not got a real fix, but this diff
gets it a little farther:

diff --git a/src/mips/builtins-mips.cc b/src/mips/builtins-mips.cc
index e7ba9f1..adc4fb5 100644
--- a/src/mips/builtins-mips.cc
+++ b/src/mips/builtins-mips.cc
@@ -646,6 +646,8 @@ static void Generate_JSConstructStubHelper(MacroAssembler*
masm,
       __ bind(&count_incremented);
     }
 
+    __ Pop(a1);
+
     // Retrieve smi-tagged arguments count from the stack.
     __ lw(a0, MemOperand(sp, 0));
     __ SmiUntag(a0);
@@ -654,7 +656,6 @@ static void Generate_JSConstructStubHelper(MacroAssembler*
masm,
     if (use_new_target) {
       __ Pop(a3);  // new.target
     }
-    __ Pop(a1);
 
     if (use_new_target) {
       __ Push(a3, t4, t4);

a0 was getting a tagged pointer, rather than the smi argument count you wanted.

I have to leave now for an appointment, I will be back in ~1.5 hours, and can
look more.

If you want to look further, build mipsel.debug with snapshot=off, then start up
the shell with --trace-sim flag. You can see where it fails, and watch the
loads/stores to the stack.

With the above patch it now fails where it is trying to offset the stack by some
integer value, and is again loading a tagged pointer from stack into a1, rather
than an Smi.

  0x26727d14  8fa20000       lw      v0, 0(sp)               00000002 <--
[f53cae60]    (65888)
  0x26727d18  8fa50004       lw      a1, 4(sp)               26727941 <--
[f53cae64]    (65889)    <<--- this gets left-shifted an added to sp, as if it
was smi.

[arv (Not doing code reviews), 2015-06-25 18:58:32]

On 2015/06/25 18:48:19, paul.l... wrote:
> The stack layout is mixed up somewhere. I've not got a real fix, but this diff
> gets it a little farther:
> 
> diff --git a/src/mips/builtins-mips.cc b/src/mips/builtins-mips.cc
> index e7ba9f1..adc4fb5 100644
> --- a/src/mips/builtins-mips.cc
> +++ b/src/mips/builtins-mips.cc
> @@ -646,6 +646,8 @@ static void Generate_JSConstructStubHelper(MacroAssembler*
> masm,
>        __ bind(&count_incremented);
>      }
>  
> +    __ Pop(a1);
> +
>      // Retrieve smi-tagged arguments count from the stack.
>      __ lw(a0, MemOperand(sp, 0));
>      __ SmiUntag(a0);
> @@ -654,7 +656,6 @@ static void Generate_JSConstructStubHelper(MacroAssembler*
> masm,
>      if (use_new_target) {
>        __ Pop(a3);  // new.target
>      }
> -    __ Pop(a1);
>  
>      if (use_new_target) {
>        __ Push(a3, t4, t4);
> 
> a0 was getting a tagged pointer, rather than the smi argument count you
wanted.
> 
> I have to leave now for an appointment, I will be back in ~1.5 hours, and can
> look more.
> 
> If you want to look further, build mipsel.debug with snapshot=off, then start
up
> the shell with --trace-sim flag. You can see where it fails, and watch the
> loads/stores to the stack.
> 
> With the above patch it now fails where it is trying to offset the stack by
some
> integer value, and is again loading a tagged pointer from stack into a1,
rather
> than an Smi.
> 
>   0x26727d14  8fa20000       lw      v0, 0(sp)               00000002 <--
> [f53cae60]    (65888)
>   0x26727d18  8fa50004       lw      a1, 4(sp)               26727941 <--
> [f53cae64]    (65889)    <<--- this gets left-shifted an added to sp, as if it
> was smi.

Thanks. Very useful. Hopefully I'll have it working by the time you come back.

[arv (Not doing code reviews), 2015-06-25 19:21:15]

Fix push/pop issue on mips

[arv (Not doing code reviews), 2015-06-25 19:58:08]

Also adjust the create memento branch

[arv (Not doing code reviews), 2015-06-25 19:59:52]

On 2015/06/25 18:58:32, arv wrote:
> On 2015/06/25 18:48:19, paul.l... wrote:
> > The stack layout is mixed up somewhere. I've not got a real fix, but this
diff
> > gets it a little farther:
> > 
> > diff --git a/src/mips/builtins-mips.cc b/src/mips/builtins-mips.cc
> > index e7ba9f1..adc4fb5 100644
> > --- a/src/mips/builtins-mips.cc
> > +++ b/src/mips/builtins-mips.cc
> > @@ -646,6 +646,8 @@ static void
Generate_JSConstructStubHelper(MacroAssembler*
> > masm,
> >        __ bind(&count_incremented);
> >      }
> >  
> > +    __ Pop(a1);
> > +
> >      // Retrieve smi-tagged arguments count from the stack.
> >      __ lw(a0, MemOperand(sp, 0));
> >      __ SmiUntag(a0);
> > @@ -654,7 +656,6 @@ static void
Generate_JSConstructStubHelper(MacroAssembler*
> > masm,
> >      if (use_new_target) {
> >        __ Pop(a3);  // new.target
> >      }
> > -    __ Pop(a1);
> >  
> >      if (use_new_target) {
> >        __ Push(a3, t4, t4);
> > 
> > a0 was getting a tagged pointer, rather than the smi argument count you
> wanted.
> > 
> > I have to leave now for an appointment, I will be back in ~1.5 hours, and
can
> > look more.
> > 
> > If you want to look further, build mipsel.debug with snapshot=off, then
start
> up
> > the shell with --trace-sim flag. You can see where it fails, and watch the
> > loads/stores to the stack.
> > 
> > With the above patch it now fails where it is trying to offset the stack by
> some
> > integer value, and is again loading a tagged pointer from stack into a1,
> rather
> > than an Smi.
> > 
> >   0x26727d14  8fa20000       lw      v0, 0(sp)               00000002 <--
> > [f53cae60]    (65888)
> >   0x26727d18  8fa50004       lw      a1, 4(sp)               26727941 <--
> > [f53cae64]    (65889)    <<--- this gets left-shifted an added to sp, as if
it
> > was smi.
> 
> Thanks. Very useful. Hopefully I'll have it working by the time you come back.

I fixed the push/pop discrepancy and now both mips and mips64 error out with
"Unaligned write".

[paul.l..., 2015-06-25 21:56:17]

On 2015/06/25 19:59:52, arv wrote:
> I fixed the push/pop discrepancy and now both mips and mips64 error out with
> "Unaligned write".

The unaligned write is because the stack pointer is corrupted when we try to pop
all the arguments off the stack at end of function, by using argument count.
    __ lw(a1, MemOperand(sp, offset));
    // Leave construct frame.
  }
  __ sll(t0, a1, kPointerSizeLog2 - 1);
  __ Addu(sp, sp, t0);
  __ Addu(sp, sp, kPointerSize);

a1 gets loaded with some tagged pointer, rather than the argument count, so
after this sequence the stack pointer is wrong, but also not a multiple of
kPointerSize, so the next access is 'unaligned'.

The problem is still here:
  0x3d427d30  8fa20000       lw      v0, 0(sp)               00000002 <--
[f5381e60]    (65892)
  0x3d427d34  8fa50004       lw      a1, 4(sp)               3d427941 <--
[f5381e64]    (65893)

The smi arg size goes into v0, which should have the receiver. But I've still
not seen where things are going off track. If it helps you, here is memory
around the sp near f5381e60:

sim> mem 0xf5381e50
  0xf5381e50:  0xf5381e70 -180871568 (smi -90435784)
  0xf5381e54:  0x3d427d08 1027767560 (smi 513883780)
  0xf5381e58:  0x3b01940d  989959181 (0x3b01940d <a Boolean with map 0x30a09891
value = 0x25808211 <true>>)
  0xf5381e5c:  0x3b01940d  989959181 (0x3b01940d <a Boolean with map 0x30a09891
value = 0x25808211 <true>>)
  0xf5381e60:  0x00000002          2 (smi 1)
  0xf5381e64:  0x3d427941 1027766593 (0x3d427941 <Code: BUILTIN>)
  0xf5381e68:  0x00000012         18 (smi 9)
  0xf5381e6c:  0x3b016efd  989949693 (0x3b016efd <FixedArray[64]>)
  0xf5381e70:  0xf5381f14 -180871404 (smi -90435702)
  0xf5381e74:  0x3d442678 1027876472 (smi 513938236)

Looks like arm port is failing in same spot. I'll keep looking....

[paul.l..., 2015-06-26 04:02:23]

paul.lind@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[paul.l..., 2015-06-26 04:02:24]

Simple issue, just took me a while to spot it. The arguments copy loop is based
on a3/r3, and that was no longer being setup. See comments.

The old way used one less instruction on arm & mips. You could do the load from
stack after your Push()'s, using the variable offset to sp like you did
elsewhere: int offset = (use_new_target ? 3 : 2) * kPointerSize;

(You could also make the copy loop counter an int instead Smi, but you'd still
have to move to a3, so you would still use one more instruction. It's not in the
loop, so it probably doesn't matter)

With the changes, arm, mips, mips64 all passed optdebug.quickcheck

https://codereview.chromium.org/1203813002/diff/80001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/1203813002/diff/80001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:652: __ add(r2, fp,
Operand(StandardFrameConstants::kCallerSPOffset));
Set up r3 as Smi-tagged loop counter.
  __ SmiTag(r3, r0);

https://codereview.chromium.org/1203813002/diff/80001/src/mips/builtins-mips.cc
File src/mips/builtins-mips.cc (right):

https://codereview.chromium.org/1203813002/diff/80001/src/mips/builtins-mips....
src/mips/builtins-mips.cc:386: __ MultiPushReversed(a0.bit() | a1.bit() |
a3.bit());
nit: this would be more readable as Push(a0, a1, a3); and similar just below. No
functional change, and not related to your CL.

https://codereview.chromium.org/1203813002/diff/80001/src/mips/builtins-mips....
src/mips/builtins-mips.cc:668: 
Set up a3 as Smi-tagged loop counter.
  __ SmiTag(a3, a0);

https://codereview.chromium.org/1203813002/diff/80001/src/mips64/builtins-mip...
File src/mips64/builtins-mips64.cc (right):

https://codereview.chromium.org/1203813002/diff/80001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:386: __ MultiPushReversed(a0.bit() | a1.bit() |
a3.bit());
nit: more readable as Push(a0, a1, a2); here and just below.

https://codereview.chromium.org/1203813002/diff/80001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:675: 
Move untagged argument count to a3 for loop counter.
  __ mov(a3, a0);

[arv (Not doing code reviews), 2015-06-26 16:23:42]

All working

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

All done. PTAL

Paul, thank you so much for helping me sort out the bugs.

[adamk, 2015-06-26 18:44:53]

lgtm

[arv (Not doing code reviews), 2015-06-26 18:45:23]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2015-06-26 18:45:43]

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

[commit-bot: I haz the power, 2015-06-26 18:48:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-26 18:48:58]

Exceeded global retry quota

[arv (Not doing code reviews), 2015-06-26 18:49:24]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2015-06-26 18:49:45]

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

[commit-bot: I haz the power, 2015-06-26 18:50:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-26 18:50:11]

Exceeded global retry quota

[adamk, 2015-06-26 18:52:46]

Looks like mksnapshot might be failing?

[arv (Not doing code reviews), 2015-06-26 18:54:27]

On 2015/06/26 18:52:46, adamk wrote:
> Looks like mksnapshot might be failing?

Yeah. I probable screwed up the ia32 at some point... looking.

[arv (Not doing code reviews), 2015-06-26 19:02:38]

Fixing the same issue throughout all the changes.

https://codereview.chromium.org/1203813002/diff/120001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1203813002/diff/120001/src/ia32/builtins-ia32...
src/ia32/builtins-ia32.cc:414: __ mov(eax, Operand(esp));
This change was the cause for the ia32 issue.

[arv (Not doing code reviews), 2015-06-26 19:06:48]

Fix issue with ia32

[adamk, 2015-06-26 19:16:26]

lgtm

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

The CQ bit was checked by arv@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1203813002/#ps160001
(title: "Add test again. It got lost in last patchset")

[commit-bot: I haz the power, 2015-06-29 18:27:58]

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

[commit-bot: I haz the power, 2015-06-29 18:29:26]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-06-29 18:29:44]

Patchset 9 (id:??) landed as
https://crrev.com/7a63bf77eb7610afdc1a968f7660781e5160ba8d
Cr-Commit-Position: refs/heads/master@{#29358}