Fix memento initialization when constructing from new call

Fix memento initialization when constructing from new call

Additionally, push the allocation site or undefined independently of creating a memento to preserve a fixed size for the construct frames.

BUG=

Committed: https://crrev.com/3285e3bf071a2575a827c5b29fe389a72dbf8966
Cr-Commit-Position: refs/heads/master@{#29719}

[Michael Lippautz, 2015-07-16 13:47:11]

mlippautz@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Lippautz, 2015-07-16 13:47:12]

PTAL

Initial ports. I cannot even turn on the flag temporarily because super()
support is still missing. (Only tests failing locally with the flag turned on
are involving super calls.)

[Michael Starzinger, 2015-07-16 14:08:29]

Looking good, just nits.

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

https://codereview.chromium.org/1239593003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:120: // Always push a potential allocation site to
preserve a fixed frame size.
nit: Ideally this behavior will become the default once we turn the flag on. So
I think it might even be fine to just drop this comment and move the push into
the block at line 124 without specifically mentioning the allocation site.
Applies to all architectures.

https://codereview.chromium.org/1239593003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:256: __ mov(edx, Operand(esp, 3 * kPointerSize));
Nice catch!

https://codereview.chromium.org/1239593003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:428: // Always push a potential allocation site to
preserve a fixed frame size.
Likewise.

[Michael Lippautz, 2015-07-16 14:15:35]

Will do other ports now.

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

https://codereview.chromium.org/1239593003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:120: // Always push a potential allocation site to
preserve a fixed frame size.
On 2015/07/16 14:08:28, Michael Starzinger wrote:
> nit: Ideally this behavior will become the default once we turn the flag on.
So
> I think it might even be fine to just drop this comment and move the push into
> the block at line 124 without specifically mentioning the allocation site.
> Applies to all architectures.

Done.

https://codereview.chromium.org/1239593003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:256: __ mov(edx, Operand(esp, 3 * kPointerSize));
On 2015/07/16 14:08:28, Michael Starzinger wrote:
> Nice catch!

Acknowledged.

https://codereview.chromium.org/1239593003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:428: // Always push a potential allocation site to
preserve a fixed frame size.
On 2015/07/16 14:08:28, Michael Starzinger wrote:
> Likewise.

Done.

[Michael Lippautz, 2015-07-16 15:22:03]

PTAL

[Michael Starzinger, 2015-07-16 15:40:12]

General note: You only get coverage for anything that is guarded by
emit_debug_code(), like the assertion in AssertUndefinedOrAllocationSite, when
you use debug mode. The test driver will pass --debug-code (which is also
required) for you.

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

https://codereview.chromium.org/1239593003/diff/60001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:477: __ AssertUndefinedOrAllocationSite(r6, r4);
This will clobber r4, which contains the untagged JSObject at this point, you
should be able to use r0 here.

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

https://codereview.chromium.org/1239593003/diff/60001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:341: __ push(a2);
Multiple pushes seem to be going on here, I think this line is bogus.

https://codereview.chromium.org/1239593003/diff/60001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:477: __ AssertUndefinedOrAllocationSite(t3, t0);
This will clobber t0, which contains the untagged JSObject at this point, you
should be able to use a0 here.

[Michael Lippautz, 2015-07-16 16:55:12]

Tests using debug builds also ran successfully.

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

https://codereview.chromium.org/1239593003/diff/60001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:477: __ AssertUndefinedOrAllocationSite(r6, r4);
On 2015/07/16 15:40:12, Michael Starzinger wrote:
> This will clobber r4, which contains the untagged JSObject at this point, you
> should be able to use r0 here.

Done.

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

https://codereview.chromium.org/1239593003/diff/60001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:341: __ push(a2);
On 2015/07/16 15:40:12, Michael Starzinger wrote:
> Multiple pushes seem to be going on here, I think this line is bogus.

whoops, done.

https://codereview.chromium.org/1239593003/diff/60001/src/mips64/builtins-mip...
src/mips64/builtins-mips64.cc:477: __ AssertUndefinedOrAllocationSite(t3, t0);
On 2015/07/16 15:40:12, Michael Starzinger wrote:
> This will clobber t0, which contains the untagged JSObject at this point, you
> should be able to use a0 here.

Done.

[Michael Starzinger, 2015-07-17 07:54:04]

LGTM after one last comment is addressed.

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

https://codereview.chromium.org/1239593003/diff/80001/src/mips/builtins-mips....
src/mips/builtins-mips.cc:342: __ push(a2);
Still multiple pushes going on here I think!

[Michael Lippautz, 2015-07-17 08:08:31]

Thanks a lot!

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

https://codereview.chromium.org/1239593003/diff/80001/src/mips/builtins-mips....
src/mips/builtins-mips.cc:342: __ push(a2);
On 2015/07/17 07:54:03, Michael Starzinger wrote:
> Still multiple pushes going on here I think!

Done.

[Michael Lippautz, 2015-07-17 08:08:41]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2015-07-17 08:08:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1239593003/#ps100001
(title: "addressed comment")

[commit-bot: I haz the power, 2015-07-17 08:08:45]

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

[Michael Lippautz, 2015-07-17 08:09:00]

The CQ bit was unchecked by mlippautz@chromium.org

[Michael Lippautz, 2015-07-17 08:46:22]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2015-07-17 08:46:28]

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

[commit-bot: I haz the power, 2015-07-17 08:51:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-07-17 08:51:49]

Patchset 6 (id:??) landed as
https://crrev.com/3285e3bf071a2575a827c5b29fe389a72dbf8966
Cr-Commit-Position: refs/heads/master@{#29719}