Introduce a BuiltinsConstructStub that sets up new.target and does a [[call]] per ES6 9.3.2

Introduce a BuiltinsConstructStub that sets up new.target and does a [[call]] per ES6 9.3.2

BUG=

Committed: https://crrev.com/469d9bfa8de49477dc4222ef9e0eb7d3d0d43d36
Cr-Commit-Position: refs/heads/master@{#32120}

[Toon Verwaest, 2015-11-16 16:46:10]

Description was changed from

==========
Introduce a BuiltinConstructStub that sets up new.target and does a [[call]] per
ES6 9.3.2

BUG=
==========

to

==========
Introduce a BuiltinsConstructStub that sets up new.target and does a [[call]]
per ES6 9.3.2

BUG=
==========

[Toon Verwaest, 2015-11-16 17:11:02]

verwaest@chromium.org changed reviewers:
+ bmeurer@chromium.org, mstarzinger@chromium.org

[Toon Verwaest, 2015-11-16 17:11:02]

WIP

[Toon Verwaest, 2015-11-17 13:53:37]

Patchset #1 (id:1) has been deleted

[Toon Verwaest, 2015-11-17 13:53:43]

Patchset #1 (id:20001) has been deleted

[Toon Verwaest, 2015-11-17 13:53:48]

Patchset #1 (id:40001) has been deleted

[Toon Verwaest, 2015-11-17 13:53:53]

Patchset #1 (id:60001) has been deleted

[Toon Verwaest, 2015-11-17 14:22:14]

Patchset #1 (id:80001) has been deleted

[Toon Verwaest, 2015-11-17 14:22:19]

Patchset #1 (id:100001) has been deleted

[Toon Verwaest, 2015-11-17 14:49:29]

Patchset #1 (id:120001) has been deleted

[Toon Verwaest, 2015-11-17 14:52:05]

Patchset #1 (id:140001) has been deleted

[Toon Verwaest, 2015-11-17 14:58:40]

Patchset #1 (id:160001) has been deleted

[Toon Verwaest, 2015-11-17 14:59:34]

verwaest@chromium.org changed reviewers:
+ ishell@chromium.org, ulan@chromium.org

[Toon Verwaest, 2015-11-17 14:59:34]

@ishell: ptal
@ulan: ptal for heap
@mstarzinger,@bmeurer: fyi (comments welcome)

[Igor Sheludko, 2015-11-18 09:16:07]

First round of comments.

I think RegExp constructor should also use JSBuiltinsConstructStub, since
IsRegExp call could have side effects. And DateView (as you already noticed).

https://codereview.chromium.org/1448933002/diff/180001/src/runtime/runtime-cl...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/1448933002/diff/180001/src/runtime/runtime-cl...
src/runtime/runtime-classes.cc:145: Handle<Code>
stub(isolate->builtins()->JSBuiltinsConstructStub());
Please add a comment about why do we use JSBuiltinsConstructStub for derived
classes to avoid confusion.

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

https://codereview.chromium.org/1448933002/diff/180001/src/x64/builtins-x64.c...
src/x64/builtins-x64.cc:356: // arguments
Please reformat the comment.

[Toon Verwaest, 2015-11-18 13:24:57]

Addressed comments

https://codereview.chromium.org/1448933002/diff/180001/src/runtime/runtime-cl...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/1448933002/diff/180001/src/runtime/runtime-cl...
src/runtime/runtime-classes.cc:145: Handle<Code>
stub(isolate->builtins()->JSBuiltinsConstructStub());
On 2015/11/18 09:16:07, Igor Sheludko wrote:
> Please add a comment about why do we use JSBuiltinsConstructStub for derived
> classes to avoid confusion.

Done.

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

https://codereview.chromium.org/1448933002/diff/180001/src/x64/builtins-x64.c...
src/x64/builtins-x64.cc:356: // arguments
On 2015/11/18 09:16:07, Igor Sheludko wrote:
> Please reformat the comment.

Done.

[Yang, 2015-11-18 14:22:47]

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

[Yang, 2015-11-18 14:22:48]

Regexp part lgtm. But please add test cases and make sure Chromium's tests pass.

[Toon Verwaest, 2015-11-18 16:30:38]

Added regexp tests and some fixes...

[Michael Starzinger, 2015-11-18 17:27:41]

FYI, I duped the following CL into this:
https://codereview.chromium.org/1221803004/

[ulan, 2015-11-19 09:03:54]

heap changes lgtm.

[Igor Sheludko, 2015-11-19 09:30:53]

Second round of comments. Only the tests are left.

https://codereview.chromium.org/1448933002/diff/360001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1448933002/diff/360001/src/api.cc#newcode6116
src/api.cc:6116: if ((flags & RegExp::kIgnoreCase) != 0) flags_buf[num_flags++]
= 'i';
Yang, this function does not support new flags yet.

https://codereview.chromium.org/1448933002/diff/360001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1448933002/diff/360001/src/js/promise.js#newc...
src/js/promise.js:67: if (IS_UNDEFINED(new.target)) throw
MakeTypeError(kNotAPromise, this);
Perhaps this should be done even before checking resolver.

https://codereview.chromium.org/1448933002/diff/360001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1448933002/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:80: flags = (REGEXP_GLOBAL(pattern) ? 'g' : '')
We do this kind of thing in three places already, what about wrapping this code
to a helper function?

https://codereview.chromium.org/1448933002/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:90: pattern = pattern.source;
Shouldn't this be a REGEXP_SOURCE(pattern) instead?

BTW, let's be uniform and put this line after next line and avoid introducing
var input_patter.

[Toon Verwaest, 2015-11-19 09:39:53]

Addressed comment

https://codereview.chromium.org/1448933002/diff/360001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1448933002/diff/360001/src/api.cc#newcode6116
src/api.cc:6116: if ((flags & RegExp::kIgnoreCase) != 0) flags_buf[num_flags++]
= 'i';
On 2015/11/19 09:30:52, Igor Sheludko wrote:
> Yang, this function does not support new flags yet.

Acknowledged.

https://codereview.chromium.org/1448933002/diff/360001/src/js/promise.js
File src/js/promise.js (right):

https://codereview.chromium.org/1448933002/diff/360001/src/js/promise.js#newc...
src/js/promise.js:67: if (IS_UNDEFINED(new.target)) throw
MakeTypeError(kNotAPromise, this);
On 2015/11/19 09:30:53, Igor Sheludko wrote:
> Perhaps this should be done even before checking resolver.

%NewObject internally will check that new.target is a JSReceiver (and
constructable) in debug mode. That should be fine...

https://codereview.chromium.org/1448933002/diff/360001/src/js/regexp.js
File src/js/regexp.js (right):

https://codereview.chromium.org/1448933002/diff/360001/src/js/regexp.js#newco...
src/js/regexp.js:90: pattern = pattern.source;
On 2015/11/19 09:30:53, Igor Sheludko wrote:
> Shouldn't this be a REGEXP_SOURCE(pattern) instead?
> 
> BTW, let's be uniform and put this line after next line and avoid introducing
> var input_patter.

We actually have to do Get(..."source") and "flags" according to the spec, both
of which are observable.

[Yang, 2015-11-19 09:46:32]

https://codereview.chromium.org/1448933002/diff/360001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1448933002/diff/360001/src/api.cc#newcode6116
src/api.cc:6116: if ((flags & RegExp::kIgnoreCase) != 0) flags_buf[num_flags++]
= 'i';
On 2015/11/19 09:30:52, Igor Sheludko wrote:
> Yang, this function does not support new flags yet.

Good catch. See https://codereview.chromium.org/1457883003/

[Toon Verwaest, 2015-11-19 10:27:47]

Added ports...

[Igor Sheludko, 2015-11-19 10:50:04]

lgtm

https://codereview.chromium.org/1448933002/diff/360001/test/mjsunit/es6/class...
File test/mjsunit/es6/classes-subclass-builtins.js (right):

https://codereview.chromium.org/1448933002/diff/360001/test/mjsunit/es6/class...
test/mjsunit/es6/classes-subclass-builtins.js:632: assertEquals(o.__proto__,
f.prototype);
assertTrue(p2 === f.prototype);

[Toon Verwaest, 2015-11-19 11:00:25]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2015-11-19 11:00:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, ulan@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/1448933002/#ps430001
(title: " ")

[commit-bot: I haz the power, 2015-11-19 11:00:38]

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

[commit-bot: I haz the power, 2015-11-19 11:20:04]

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

[commit-bot: I haz the power, 2015-11-19 11:20:05]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/12029)

[Toon Verwaest, 2015-11-19 12:37:25]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2015-11-19 12:37:26]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
ulan@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1448933002/#ps450001
(title: " ")

[commit-bot: I haz the power, 2015-11-19 12:37:34]

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

[commit-bot: I haz the power, 2015-11-19 12:48:41]

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

[commit-bot: I haz the power, 2015-11-19 12:48:42]

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

[Toon Verwaest, 2015-11-19 14:12:08]

The CQ bit was checked by verwaest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 14:12:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1448933002/470001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1448933002/470001

[Toon Verwaest, 2015-11-19 14:30:40]

The CQ bit was unchecked by verwaest@chromium.org

[Toon Verwaest, 2015-11-19 16:08:33]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2015-11-19 16:08:33]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
ulan@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1448933002/#ps470001
(title: " ")

[commit-bot: I haz the power, 2015-11-19 16:08:52]

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

[commit-bot: I haz the power, 2015-11-19 16:11:06]

Committed patchset #16 (id:470001)

[commit-bot: I haz the power, 2015-11-19 16:11:18]

Patchset 16 (id:??) landed as
https://crrev.com/469d9bfa8de49477dc4222ef9e0eb7d3d0d43d36
Cr-Commit-Position: refs/heads/master@{#32120}