[Interpreter] Support for operator new.

[Interpreter] Support for operator new.

This change add a new bytecode for operator new and implements it using
the Construct() builtin.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/8e4f9963d53913eab7fbd2f61a5733d8dc2169e7
Cr-Commit-Position: refs/heads/master@{#31293}

Committed: https://crrev.com/7557dc5a70e333ba10eb370fe6e7b7a31760d891
Cr-Commit-Position: refs/heads/master@{#31312}

[oth, 2015-10-13 12:32:40]

oth@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[oth, 2015-10-13 12:32:40]

rmcilroy@chromium.org: Please review all the changes here.

bmeurer@chromium.org: Please review changes in x64 and compiler.

Other platforms to follow if the approach here looks sound. Thanks!

[rmcilroy, 2015-10-13 14:07:30]

The approach looks great. A couple of comments, and some BytecodeGenerator tests
would be good to, thanks.

https://codereview.chromium.org/1402943002/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/1402943002/diff/20001/src/code-factory.h#newc...
src/code-factory.h:100: static Callable ConstructNew(Isolate* isolate);
You don't seem to use or define ConstructNew - remove this declaration?

https://codereview.chromium.org/1402943002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1402943002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:108: // argumnents (not including receiver)
and the first argument
/s/argumnents/arguments

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:531: BytecodeArrayBuilder&
BytecodeArrayBuilder::New(Register constructor,
nit - move above CallRuntime

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:95: size_t arg_count);
nit - move above CallRuntime

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:733: }
nit - would it be possible to pull out this code into a VisitArgs helper (shared
by CallJS and CallRuntime)? This might be easier with the
VisitForRegister/Accumulator changes so fine to do this in a later CL if it's
tricky.

https://codereview.chromium.org/1402943002/diff/20001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:856: __ PushReturnAddressFrom(kScratchRegister);
nit - could we create a helper function for the arg pushing assembly which is
shared between PushArgsAndCall and PushArgsAndConstruct.

https://codereview.chromium.org/1402943002/diff/20001/src/x64/interface-descr...
File src/x64/interface-descriptors-x64.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/x64/interface-descr...
src/x64/interface-descriptors-x64.cc:392: rdi   // the constructor.
I don't think this change was intended?

https://codereview.chromium.org/1402943002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1669: "  var c = new counter();\n"
You're not actually passing the arguments to counter here, were you intending
to?

[Benedikt Meurer, 2015-10-14 03:44:25]

compiler and x64 LGTM modulo comments.

https://codereview.chromium.org/1402943002/diff/40001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/1402943002/diff/40001/src/code-factory.h#newc...
src/code-factory.h:100: static Callable ConstructNew(Isolate* isolate);
This is probably dead code?

https://codereview.chromium.org/1402943002/diff/40001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1402943002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:788: static void
Generate_InterpreterPushArgs(MacroAssembler* masm,
Don't use static, but put the helper in an anonymous namespace instead.

[oth, 2015-10-14 08:40:10]

Thanks. Comments incorporated with a query about how far to push on the
anonymous namespace for builtins-x64.cc.

Other platforms and generator test to follow.

https://codereview.chromium.org/1402943002/diff/20001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/1402943002/diff/20001/src/code-factory.h#newc...
src/code-factory.h:100: static Callable ConstructNew(Isolate* isolate);
On 2015/10/13 14:07:29, rmcilroy wrote:
> You don't seem to use or define ConstructNew - remove this declaration?

Done.

https://codereview.chromium.org/1402943002/diff/20001/src/compiler/interprete...
File src/compiler/interpreter-assembler.h (right):

https://codereview.chromium.org/1402943002/diff/20001/src/compiler/interprete...
src/compiler/interpreter-assembler.h:108: // argumnents (not including receiver)
and the first argument
On 2015/10/13 14:07:29, rmcilroy wrote:
> /s/argumnents/arguments

Done.

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:531: BytecodeArrayBuilder&
BytecodeArrayBuilder::New(Register constructor,
On 2015/10/13 14:07:30, rmcilroy wrote:
> nit - move above CallRuntime

Done.

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:95: size_t arg_count);
On 2015/10/13 14:07:30, rmcilroy wrote:
> nit - move above CallRuntime

Done.

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:733: }
On 2015/10/13 14:07:30, rmcilroy wrote:
> nit - would it be possible to pull out this code into a VisitArgs helper
(shared
> by CallJS and CallRuntime)? This might be easier with the
> VisitForRegister/Accumulator changes so fine to do this in a later CL if it's
> tricky.

Done.

https://codereview.chromium.org/1402943002/diff/20001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:856: __ PushReturnAddressFrom(kScratchRegister);
On 2015/10/13 14:07:30, rmcilroy wrote:
> nit - could we create a helper function for the arg pushing assembly which is
> shared between PushArgsAndCall and PushArgsAndConstruct.

Done.

https://codereview.chromium.org/1402943002/diff/20001/src/x64/interface-descr...
File src/x64/interface-descriptors-x64.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/src/x64/interface-descr...
src/x64/interface-descriptors-x64.cc:392: rdi   // the constructor.
On 2015/10/13 14:07:30, rmcilroy wrote:
> I don't think this change was intended?

Done. Darn!

https://codereview.chromium.org/1402943002/diff/20001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1402943002/diff/20001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1669: "  var c = new counter();\n"
On 2015/10/13 14:07:30, rmcilroy wrote:
> You're not actually passing the arguments to counter here, were you intending
> to? 

Done.

https://codereview.chromium.org/1402943002/diff/40001/src/code-factory.h
File src/code-factory.h (right):

https://codereview.chromium.org/1402943002/diff/40001/src/code-factory.h#newc...
src/code-factory.h:100: static Callable ConstructNew(Isolate* isolate);
On 2015/10/14 03:44:25, Benedikt Meurer wrote:
> This is probably dead code?

Done. Yes, sorry about that.

https://codereview.chromium.org/1402943002/diff/40001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1402943002/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:788: static void
Generate_InterpreterPushArgs(MacroAssembler* masm,
On 2015/10/14 03:44:25, Benedikt Meurer wrote:
> Don't use static, but put the helper in an anonymous namespace instead.

We can do this though want to clarify how far to push. The use of static here
has followed the convention for helper methods in this file. Do we (a) leave
as-is for consistency, (b) move all static methods to an anonymous namespace, or
(c) just do this one?

[rmcilroy, 2015-10-14 10:18:35]

lgtm once bytecode generator tests are added and other architectures ported.

https://codereview.chromium.org/1402943002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1402943002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:744: builder()->New(constructor,
constructor, 0);
nit - I think it deserves a comment that you are passing the constructor
register as first_arg so that there is a valid register, but it won't be used
since arg_count == 0

https://codereview.chromium.org/1402943002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:765: first_arg =
temporary_register_scope.NewRegister();
ditto

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

https://codereview.chromium.org/1402943002/diff/60001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:855: // Push required padding.
nit - Push padding required for receiver which is to be constructed.

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:917: B(LoadICSloppy), R(1),
U8(5),            //
please use vector->GetIndex(...) here

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1877: 
Still hoping for a BytecodeGenerator test for new

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1684: "function counter(arg0, arg1)
{ this.count = 17; this.x = arg0; this.y = "
nit - newline after '{'

[oth, 2015-10-14 16:02:19]

Think everything's done here modulo fixing ports.

https://codereview.chromium.org/1402943002/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1402943002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:744: builder()->New(constructor,
constructor, 0);
On 2015/10/14 10:18:34, rmcilroy wrote:
> nit - I think it deserves a comment that you are passing the constructor
> register as first_arg so that there is a valid register, but it won't be used
> since arg_count == 0

Done.

https://codereview.chromium.org/1402943002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:765: first_arg =
temporary_register_scope.NewRegister();
On 2015/10/14 10:18:34, rmcilroy wrote:
> ditto

Done.

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

https://codereview.chromium.org/1402943002/diff/60001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:855: // Push required padding.
On 2015/10/14 10:18:34, rmcilroy wrote:
> nit - Push padding required for receiver which is to be constructed.

Done.

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-bytecode-generator.cc (right):

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:917: B(LoadICSloppy), R(1),
U8(5),            //
On 2015/10/14 10:18:35, rmcilroy wrote:
> please use vector->GetIndex(...) here

Done.

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-bytecode-generator.cc:1877: 
On 2015/10/14 10:18:35, rmcilroy wrote:
> Still hoping for a BytecodeGenerator test for new

Done!

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1402943002/diff/60001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:1684: "function counter(arg0, arg1)
{ this.count = 17; this.x = arg0; this.y = "
On 2015/10/14 10:18:35, rmcilroy wrote:
> nit - newline after '{'

Done.

[rmcilroy, 2015-10-15 10:30:55]

Looks great. LGTM with optional suggestion on ia32 port.

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

https://codereview.chromium.org/1402943002/diff/270001/src/ia32/builtins-ia32...
src/ia32/builtins-ia32.cc:782: // Save number of arguments on the stack below
where arguments are pushed.
nit - arguments are going to be pushed

https://codereview.chromium.org/1402943002/diff/270001/src/ia32/builtins-ia32...
src/ia32/builtins-ia32.cc:800: __ xchg(eax, ecx);
optional nit - I think you could avoid the xchg if you just keep the return
address in ecx and pass stack limit in eax, with Generate_InterpreterPushArgs
taking an argument for the register holding the stack limit.

[oth, 2015-10-15 11:16:43]

Thanks.

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

https://codereview.chromium.org/1402943002/diff/270001/src/ia32/builtins-ia32...
src/ia32/builtins-ia32.cc:782: // Save number of arguments on the stack below
where arguments are pushed.
On 2015/10/15 10:30:55, rmcilroy wrote:
> nit - arguments are going to be pushed

Done.

https://codereview.chromium.org/1402943002/diff/270001/src/ia32/builtins-ia32...
src/ia32/builtins-ia32.cc:800: __ xchg(eax, ecx);
On 2015/10/15 10:30:55, rmcilroy wrote:
> optional nit - I think you could avoid the xchg if you just keep the return
> address in ecx and pass stack limit in eax, with Generate_InterpreterPushArgs
> taking an argument for the register holding the stack limit.

Done. Also looked at stashing the stack address in that slot and subtracting esp
at the end, but it saves no instructions, but is aesthetically neater.

[oth, 2015-10-15 11:16:55]

The CQ bit was checked by oth@chromium.org

[oth, 2015-10-15 11:16:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1402943002/#ps280001
(title: "Tweak ia32.")

[commit-bot: I haz the power, 2015-10-15 11:17:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1402943002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1402943002/280001

[commit-bot: I haz the power, 2015-10-15 11:18:03]

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

[commit-bot: I haz the power, 2015-10-15 11:18:03]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[oth, 2015-10-15 11:27:29]

The CQ bit was checked by oth@chromium.org

[oth, 2015-10-15 11:27:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1402943002/#ps290001
(title: "Rebase")

[commit-bot: I haz the power, 2015-10-15 11:27:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1402943002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1402943002/290001

[commit-bot: I haz the power, 2015-10-15 11:50:50]

Committed patchset #17 (id:290001)

[commit-bot: I haz the power, 2015-10-15 11:51:15]

Patchset 17 (id:??) landed as
https://crrev.com/8e4f9963d53913eab7fbd2f61a5733d8dc2169e7
Cr-Commit-Position: refs/heads/master@{#31293}

[Michael Achenbach, 2015-10-15 12:49:19]

A revert of this CL (patchset #17 id:290001) has been created in
https://codereview.chromium.org/1402153004/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Breaks arm64 debug:
http://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20s....

[oth, 2015-10-15 15:48:02]

The CQ bit was checked by oth@chromium.org

[oth, 2015-10-15 15:48:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1402943002/#ps310001
(title: "Fix missing receiver slot.")

[commit-bot: I haz the power, 2015-10-15 15:48:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1402943002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1402943002/310001

[commit-bot: I haz the power, 2015-10-15 16:46:23]

Committed patchset #18 (id:310001)

[commit-bot: I haz the power, 2015-10-15 16:46:42]

Patchset 18 (id:??) landed as
https://crrev.com/7557dc5a70e333ba10eb370fe6e7b7a31760d891
Cr-Commit-Position: refs/heads/master@{#31312}