[es6] implement Reflect.apply() & Reflect.construct()

[es6] implement Reflect.apply() & Reflect.construct()

BUG=v8:3900
LOG=N
R=dslomov@chromium.org

Committed: https://crrev.com/d21fd15467e16f185e511dbfbaeef7caddfe804a
Cr-Commit-Position: refs/heads/master@{#27316}

[caitp (gmail), 2015-02-18 22:44:57]

Basic work on an implementation of Reflect.apply() (x64 only) which shares code
with FunctionApply.

Establishing this and ReflectConstruct should help simplify the implementation
of spreadcalls, so I'd love to get these in

[caitp (gmail), 2015-02-18 23:44:57]

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

[caitp (gmail), 2015-02-19 23:10:32]

On 2015/02/18 22:44:57, caitp wrote:
> Basic work on an implementation of Reflect.apply() (x64 only) which shares
code
> with FunctionApply.
> 
> Establishing this and ReflectConstruct should help simplify the implementation
> of spreadcalls, so I'd love to get these in

ReflectConstruct is proving to be a lot more difficult to implement :[

[arv (Not doing code reviews), 2015-02-20 00:33:21]

On 2015/02/19 23:10:32, caitp wrote:
> On 2015/02/18 22:44:57, caitp wrote:
> > Basic work on an implementation of Reflect.apply() (x64 only) which shares
> code
> > with FunctionApply.
> > 
> > Establishing this and ReflectConstruct should help simplify the
implementation
> > of spreadcalls, so I'd love to get these in
> 
> ReflectConstruct is proving to be a lot more difficult to implement :[

Yeah. We need to implement new.target too. We need to track usage of new.target
and then use a different construct stub if that is present.

[Dmitry Lomov (no reviews), 2015-02-27 11:24:25]

The approach looks good to me. Can you send an intent-to-implement? I for one
will be super happy to see this getting in!

On 2015/02/20 00:33:21, arv wrote:
> On 2015/02/19 23:10:32, caitp wrote:
> > On 2015/02/18 22:44:57, caitp wrote:
> > > Basic work on an implementation of Reflect.apply() (x64 only) which shares
> > code
> > > with FunctionApply.
> > > 
> > > Establishing this and ReflectConstruct should help simplify the
> implementation
> > > of spreadcalls, so I'd love to get these in
> > 
> > ReflectConstruct is proving to be a lot more difficult to implement :[
> 
> Yeah. We need to implement new.target too. We need to track usage of
new.target
> and then use a different construct stub if that is present.

In fact, ReflectConstruct is independent from new.target syntax support.
All construct stubs are already ready to accept NewTarget argument.
What you need to do is to implement an INLINE runtime function (see e.g.
DefaultConstructorSuperCall for inspiration) and in that function, generate a
call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
Here is an example for that:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...

[caitp (gmail), 2015-02-27 14:00:12]

On 2015/02/27 11:24:25, Dmitry Lomov (chromium) wrote:
> The approach looks good to me. Can you send an intent-to-implement? I for one
> will be super happy to see this getting in!
> 

Will do that today.

BTW, do you have an opinion on the weirdness in the bootstrapper? It seems
really weird to me that initializing that `InitializeExperimentalGlobal()` runs
after `InstallExperimentalNatives()`, since I need to install the Reflect global
before putting things in it. Maybe there's a better approach for that

> On 2015/02/20 00:33:21, arv wrote:
> > On 2015/02/19 23:10:32, caitp wrote:
> > > On 2015/02/18 22:44:57, caitp wrote:
> > > > Basic work on an implementation of Reflect.apply() (x64 only) which
shares
> > > code
> > > > with FunctionApply.
> > > > 
> > > > Establishing this and ReflectConstruct should help simplify the
> > implementation
> > > > of spreadcalls, so I'd love to get these in
> > > 
> > > ReflectConstruct is proving to be a lot more difficult to implement :[
> > 
> > Yeah. We need to implement new.target too. We need to track usage of
> new.target
> > and then use a different construct stub if that is present.
> 
> In fact, ReflectConstruct is independent from new.target syntax support.
> All construct stubs are already ready to accept NewTarget argument.
> What you need to do is to implement an INLINE runtime function (see e.g.
> DefaultConstructorSuperCall for inspiration) and in that function, generate a
> call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
> Here is an example for that:
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...

I'll take another look at ReflectConstruct today

[caitp (gmail), 2015-02-27 18:06:24]

On 2015/02/27 14:00:12, caitp wrote:
> On 2015/02/27 11:24:25, Dmitry Lomov (chromium) wrote:
> > The approach looks good to me. Can you send an intent-to-implement? I for
one
> > will be super happy to see this getting in!
> > 
> 
> Will do that today.
> 
> BTW, do you have an opinion on the weirdness in the bootstrapper? It seems
> really weird to me that initializing that `InitializeExperimentalGlobal()`
runs
> after `InstallExperimentalNatives()`, since I need to install the Reflect
global
> before putting things in it. Maybe there's a better approach for that
> 
> > On 2015/02/20 00:33:21, arv wrote:
> > > On 2015/02/19 23:10:32, caitp wrote:
> > > > On 2015/02/18 22:44:57, caitp wrote:
> > > > > Basic work on an implementation of Reflect.apply() (x64 only) which
> shares
> > > > code
> > > > > with FunctionApply.
> > > > > 
> > > > > Establishing this and ReflectConstruct should help simplify the
> > > implementation
> > > > > of spreadcalls, so I'd love to get these in
> > > > 
> > > > ReflectConstruct is proving to be a lot more difficult to implement :[
> > > 
> > > Yeah. We need to implement new.target too. We need to track usage of
> > new.target
> > > and then use a different construct stub if that is present.
> > 
> > In fact, ReflectConstruct is independent from new.target syntax support.
> > All construct stubs are already ready to accept NewTarget argument.
> > What you need to do is to implement an INLINE runtime function (see e.g.
> > DefaultConstructorSuperCall for inspiration) and in that function, generate
a
> > call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
> > Here is an example for that:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...
> 
> I'll take another look at ReflectConstruct today

So the issue with ReflectConstruct right now, is passing the right feedback
vector to CallConstructStub. I think this is sort of related to
https://code.google.com/p/v8/issues/detail?id=3930. Is there any way to call
construct stubs without a type feedback vector? or better, use a feedback vector
slot from the caller of ReflectConstruct

[Dmitry Lomov (no reviews), 2015-02-27 20:09:29]

On 2015/02/27 18:06:24, caitp wrote:
> On 2015/02/27 14:00:12, caitp wrote:
> > On 2015/02/27 11:24:25, Dmitry Lomov (chromium) wrote:
> > > The approach looks good to me. Can you send an intent-to-implement? I for
> one
> > > will be super happy to see this getting in!
> > > 
> > 
> > Will do that today.
> > 
> > BTW, do you have an opinion on the weirdness in the bootstrapper? It seems
> > really weird to me that initializing that `InitializeExperimentalGlobal()`
> runs
> > after `InstallExperimentalNatives()`, since I need to install the Reflect
> global
> > before putting things in it. Maybe there's a better approach for that
> > 
> > > On 2015/02/20 00:33:21, arv wrote:
> > > > On 2015/02/19 23:10:32, caitp wrote:
> > > > > On 2015/02/18 22:44:57, caitp wrote:
> > > > > > Basic work on an implementation of Reflect.apply() (x64 only) which
> > shares
> > > > > code
> > > > > > with FunctionApply.
> > > > > > 
> > > > > > Establishing this and ReflectConstruct should help simplify the
> > > > implementation
> > > > > > of spreadcalls, so I'd love to get these in
> > > > > 
> > > > > ReflectConstruct is proving to be a lot more difficult to implement :[
> > > > 
> > > > Yeah. We need to implement new.target too. We need to track usage of
> > > new.target
> > > > and then use a different construct stub if that is present.
> > > 
> > > In fact, ReflectConstruct is independent from new.target syntax support.
> > > All construct stubs are already ready to accept NewTarget argument.
> > > What you need to do is to implement an INLINE runtime function (see e.g.
> > > DefaultConstructorSuperCall for inspiration) and in that function,
generate
> a
> > > call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
> > > Here is an example for that:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...
> > 
> > I'll take another look at ReflectConstruct today
> 
> So the issue with ReflectConstruct right now, is passing the right feedback
> vector to CallConstructStub. I think this is sort of related to
> https://code.google.com/p/v8/issues/detail?id=3930. Is there any way to call
> construct stubs without a type feedback vector? or better, use a feedback
vector
> slot from the caller of ReflectConstruct
You can just pass 'null'

[Dmitry Lomov (no reviews), 2015-02-27 20:19:03]

On 2015/02/27 14:00:12, caitp wrote:
> On 2015/02/27 11:24:25, Dmitry Lomov (chromium) wrote:
> > The approach looks good to me. Can you send an intent-to-implement? I for
one
> > will be super happy to see this getting in!
> > 
> 
> Will do that today.
> 
> BTW, do you have an opinion on the weirdness in the bootstrapper? It seems
> really weird to me that initializing that `InitializeExperimentalGlobal()`
runs
> after `InstallExperimentalNatives()`, since I need to install the Reflect
global
> before putting things in it. Maybe there's a better approach for that

You are right, the order of these is off.
Yours is the first CL since our feature management redesign that adds a new
global.
I'll take a look at fixing this.

> 
> > On 2015/02/20 00:33:21, arv wrote:
> > > On 2015/02/19 23:10:32, caitp wrote:
> > > > On 2015/02/18 22:44:57, caitp wrote:
> > > > > Basic work on an implementation of Reflect.apply() (x64 only) which
> shares
> > > > code
> > > > > with FunctionApply.
> > > > > 
> > > > > Establishing this and ReflectConstruct should help simplify the
> > > implementation
> > > > > of spreadcalls, so I'd love to get these in
> > > > 
> > > > ReflectConstruct is proving to be a lot more difficult to implement :[
> > > 
> > > Yeah. We need to implement new.target too. We need to track usage of
> > new.target
> > > and then use a different construct stub if that is present.
> > 
> > In fact, ReflectConstruct is independent from new.target syntax support.
> > All construct stubs are already ready to accept NewTarget argument.
> > What you need to do is to implement an INLINE runtime function (see e.g.
> > DefaultConstructorSuperCall for inspiration) and in that function, generate
a
> > call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
> > Here is an example for that:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...
> 
> I'll take another look at ReflectConstruct today

[Dmitry Lomov (no reviews), 2015-02-27 23:20:44]

On 2015/02/27 20:09:29, Dmitry Lomov (chromium) wrote:
> On 2015/02/27 18:06:24, caitp wrote:
> > On 2015/02/27 14:00:12, caitp wrote:
> > > On 2015/02/27 11:24:25, Dmitry Lomov (chromium) wrote:
> > > > The approach looks good to me. Can you send an intent-to-implement? I
for
> > one
> > > > will be super happy to see this getting in!
> > > > 
> > > 
> > > Will do that today.
> > > 
> > > BTW, do you have an opinion on the weirdness in the bootstrapper? It seems
> > > really weird to me that initializing that `InitializeExperimentalGlobal()`
> > runs
> > > after `InstallExperimentalNatives()`, since I need to install the Reflect
> > global
> > > before putting things in it. Maybe there's a better approach for that
> > > 
> > > > On 2015/02/20 00:33:21, arv wrote:
> > > > > On 2015/02/19 23:10:32, caitp wrote:
> > > > > > On 2015/02/18 22:44:57, caitp wrote:
> > > > > > > Basic work on an implementation of Reflect.apply() (x64 only)
which
> > > shares
> > > > > > code
> > > > > > > with FunctionApply.
> > > > > > > 
> > > > > > > Establishing this and ReflectConstruct should help simplify the
> > > > > implementation
> > > > > > > of spreadcalls, so I'd love to get these in
> > > > > > 
> > > > > > ReflectConstruct is proving to be a lot more difficult to implement
:[
> > > > > 
> > > > > Yeah. We need to implement new.target too. We need to track usage of
> > > > new.target
> > > > > and then use a different construct stub if that is present.
> > > > 
> > > > In fact, ReflectConstruct is independent from new.target syntax support.
> > > > All construct stubs are already ready to accept NewTarget argument.
> > > > What you need to do is to implement an INLINE runtime function (see e.g.
> > > > DefaultConstructorSuperCall for inspiration) and in that function,
> generate
> > a
> > > > call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
> > > > Here is an example for that:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...
> > > 
> > > I'll take another look at ReflectConstruct today
> > 
> > So the issue with ReflectConstruct right now, is passing the right feedback
> > vector to CallConstructStub. I think this is sort of related to
> > https://code.google.com/p/v8/issues/detail?id=3930. Is there any way to call
> > construct stubs without a type feedback vector? or better, use a feedback
> vector
> > slot from the caller of ReflectConstruct
> You can just pass 'null'

s/null/undefined/ - sorry!

[caitp (gmail), 2015-03-03 13:50:15]

On 2015/02/27 23:20:44, Dmitry Lomov (chromium) wrote:
> On 2015/02/27 20:09:29, Dmitry Lomov (chromium) wrote:
> > On 2015/02/27 18:06:24, caitp wrote:
> > > On 2015/02/27 14:00:12, caitp wrote:
> > > > On 2015/02/27 11:24:25, Dmitry Lomov (chromium) wrote:
> > > > > The approach looks good to me. Can you send an intent-to-implement? I
> for
> > > one
> > > > > will be super happy to see this getting in!
> > > > > 
> > > > 
> > > > Will do that today.
> > > > 
> > > > BTW, do you have an opinion on the weirdness in the bootstrapper? It
seems
> > > > really weird to me that initializing that
`InitializeExperimentalGlobal()`
> > > runs
> > > > after `InstallExperimentalNatives()`, since I need to install the
Reflect
> > > global
> > > > before putting things in it. Maybe there's a better approach for that
> > > > 
> > > > > On 2015/02/20 00:33:21, arv wrote:
> > > > > > On 2015/02/19 23:10:32, caitp wrote:
> > > > > > > On 2015/02/18 22:44:57, caitp wrote:
> > > > > > > > Basic work on an implementation of Reflect.apply() (x64 only)
> which
> > > > shares
> > > > > > > code
> > > > > > > > with FunctionApply.
> > > > > > > > 
> > > > > > > > Establishing this and ReflectConstruct should help simplify the
> > > > > > implementation
> > > > > > > > of spreadcalls, so I'd love to get these in
> > > > > > > 
> > > > > > > ReflectConstruct is proving to be a lot more difficult to
implement
> :[
> > > > > > 
> > > > > > Yeah. We need to implement new.target too. We need to track usage of
> > > > > new.target
> > > > > > and then use a different construct stub if that is present.
> > > > > 
> > > > > In fact, ReflectConstruct is independent from new.target syntax
support.
> > > > > All construct stubs are already ready to accept NewTarget argument.
> > > > > What you need to do is to implement an INLINE runtime function (see
e.g.
> > > > > DefaultConstructorSuperCall for inspiration) and in that function,
> > generate
> > > a
> > > > > call to CallConstructStub (SUPER_CONSTRUCTOR_CALL variant).
> > > > > Here is an example for that:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/x64/full-co...
> > > > 
> > > > I'll take another look at ReflectConstruct today
> > > 
> > > So the issue with ReflectConstruct right now, is passing the right
feedback
> > > vector to CallConstructStub. I think this is sort of related to
> > > https://code.google.com/p/v8/issues/detail?id=3930. Is there any way to
call
> > > construct stubs without a type feedback vector? or better, use a feedback
> > vector
> > > slot from the caller of ReflectConstruct
> > You can just pass 'null'
> 
> s/null/undefined/ - sorry!

Added basic support for Reflect.construct --- doesn't seem to currently be
possible to test the new.target behaviour, though.

[arv (Not doing code reviews), 2015-03-03 14:25:52]

You should be able to test new.target by doing something like.

function C() {
  this[9] = 1;
}

var o = Reflect.construct(C, [], Array);
assertSame(C.prototype, o.__proto__);
assertSame(10, o.length);

https://codereview.chromium.org/913073003/diff/60001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/913073003/diff/60001/src/runtime.js#newcode478
src/runtime.js:478: if (length > 0x800000) {
maybe add a const/macro for this?

https://codereview.chromium.org/913073003/diff/60001/test/mjsunit/harmony/ref...
File test/mjsunit/harmony/reflect-apply.js (right):

https://codereview.chromium.org/913073003/diff/60001/test/mjsunit/harmony/ref...
test/mjsunit/harmony/reflect-apply.js:30:
assertInstanceof(Reflect.apply(returnThis, "str", []), String);
Maybe check typeof and Object.getPrototypeOf instead of relying on instanceof
which would pass even if there was an extra object on the prototype chain.

[caitp (gmail), 2015-03-03 15:37:56]

On 2015/03/03 14:25:52, arv wrote:
> You should be able to test new.target by doing something like.
> 
> function C() {
>   this[9] = 1;
> }
> 
> var o = Reflect.construct(C, [], Array);
> assertSame(C.prototype, o.__proto__);
> assertSame(10, o.length);

Well, this test makes sense, but doesn't actually work currently. The object is
created with the right prototype (for Array), but setting the element does not
appear to affect the length, as though the Array length setter is never invoked.
That's really weird =)

Anyways I'll at least assert that it has the right prototype

[caitp (gmail), 2015-03-03 15:53:49]

https://codereview.chromium.org/913073003/diff/60001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/913073003/diff/60001/src/runtime.js#newcode478
src/runtime.js:478: if (length > 0x800000) {
On 2015/03/03 14:25:52, arv wrote:
> maybe add a const/macro for this?

Replaced the magic number with a macro, hows that?

https://codereview.chromium.org/913073003/diff/60001/test/mjsunit/harmony/ref...
File test/mjsunit/harmony/reflect-apply.js (right):

https://codereview.chromium.org/913073003/diff/60001/test/mjsunit/harmony/ref...
test/mjsunit/harmony/reflect-apply.js:30:
assertInstanceof(Reflect.apply(returnThis, "str", []), String);
On 2015/03/03 14:25:52, arv wrote:
> Maybe check typeof and Object.getPrototypeOf instead of relying on instanceof
> which would pass even if there was an extra object on the prototype chain.

Done.

[caitp (gmail), 2015-03-04 03:29:39]

@arv re: chat on IRC, I've tried the last rebased patchset with 1604bd4 reverted
and without, all tests pass regardless. I think the newTarget testing is
probably adequate like this (although more is always good too, I guess).

If it looks okay to you guys, I'll start writing ports tomorrow

[arv (Not doing code reviews), 2015-03-04 09:45:01]

https://codereview.chromium.org/913073003/diff/120001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/913073003/diff/120001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2930: // Shouldn't InitializeExperimentalGlobal() happen
before
Dmitry fixed this I believe.

https://codereview.chromium.org/913073003/diff/120001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/913073003/diff/120001/src/runtime.js#newcode490
src/runtime.js:490: var ctorOk = IS_SPEC_FUNCTION(this) && %IsConstructor(this);
This runtime function makes me sad. I see that you have a todo in
Runtime_IsConstructor...

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

https://codereview.chromium.org/913073003/diff/120001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1232: static void
Generate_ConstructHelper(MacroAssembler* masm) {
Can the code in here be better shared with Generate_ApplyHelper?

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/reflect-construct.js (right):

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
also check length and Array.isArray to ensure we created an exotic array object.

[caitp (gmail), 2015-03-04 13:24:15]

https://codereview.chromium.org/913073003/diff/120001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/913073003/diff/120001/src/bootstrapper.cc#new...
src/bootstrapper.cc:2930: // Shouldn't InitializeExperimentalGlobal() happen
before
On 2015/03/04 09:45:01, arv wrote:
> Dmitry fixed this I believe.

Fix hasn't landed yet, but yes it will be changed when it does

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

https://codereview.chromium.org/913073003/diff/120001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1232: static void
Generate_ConstructHelper(MacroAssembler* masm) {
On 2015/03/04 09:45:01, arv wrote:
> Can the code in here be better shared with Generate_ApplyHelper?

It was my first approach, but it made things really, really hard to follow. It
might be better to factor the shared bits into smaller functions

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/reflect-construct.js (right):

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
On 2015/03/04 09:45:01, arv wrote:
> also check length and Array.isArray to ensure we created an exotic array
object.

That's not exactly correct:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycreatefromc...
--- It's an ordinary object with the Array prototype.

[arv (Not doing code reviews), 2015-03-04 13:30:33]

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/reflect-construct.js (right):

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
On 2015/03/04 13:24:14, caitp wrote:
> On 2015/03/04 09:45:01, arv wrote:
> > also check length and Array.isArray to ensure we created an exotic array
> object.
> 
> That's not exactly correct:
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycreatefromc...
> --- It's an ordinary object with the Array prototype.

You are right.

Maybe a more interesting test then would be `Reflect.construct(Array, [5],
Constructor)`.

[caitp (gmail), 2015-03-04 13:54:56]

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/reflect-construct.js (right):

https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
On 2015/03/04 13:30:33, arv wrote:
> On 2015/03/04 13:24:14, caitp wrote:
> > On 2015/03/04 09:45:01, arv wrote:
> > > also check length and Array.isArray to ensure we created an exotic array
> > object.
> > 
> > That's not exactly correct:
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycreatefromc...
> > --- It's an ordinary object with the Array prototype.
> 
> You are right.
> 
> Maybe a more interesting test then would be `Reflect.construct(Array, [5],
> Constructor)`.

Hmm, we seem to be doing something wrong though. If `target` is a builtin
function, the result ends up being an exotic <<whatever>>, regardless of what
`newTarget` is, even though it gets the right prototype. I don't think that's
correct.

[caitp (gmail), 2015-03-04 13:59:28]

On 2015/03/04 13:54:56, caitp wrote:
>
https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
> File test/mjsunit/harmony/reflect-construct.js (right):
> 
>
https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
> test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
> On 2015/03/04 13:30:33, arv wrote:
> > On 2015/03/04 13:24:14, caitp wrote:
> > > On 2015/03/04 09:45:01, arv wrote:
> > > > also check length and Array.isArray to ensure we created an exotic array
> > > object.
> > > 
> > > That's not exactly correct:
> > >
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycreatefromc...
> > > --- It's an ordinary object with the Array prototype.
> > 
> > You are right.
> > 
> > Maybe a more interesting test then would be `Reflect.construct(Array, [5],
> > Constructor)`.
> 
> Hmm, we seem to be doing something wrong though. If `target` is a builtin
> function, the result ends up being an exotic <<whatever>>, regardless of what
> `newTarget` is, even though it gets the right prototype. I don't think that's
> correct.

wait, I'm wrong. Yeah, adding some extra tests

[caitp (gmail), 2015-03-04 14:01:01]

On 2015/03/04 13:59:28, caitp wrote:
> On 2015/03/04 13:54:56, caitp wrote:
> >
>
https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
> > File test/mjsunit/harmony/reflect-construct.js (right):
> > 
> >
>
https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
> > test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
> > On 2015/03/04 13:30:33, arv wrote:
> > > On 2015/03/04 13:24:14, caitp wrote:
> > > > On 2015/03/04 09:45:01, arv wrote:
> > > > > also check length and Array.isArray to ensure we created an exotic
array
> > > > object.
> > > > 
> > > > That's not exactly correct:
> > > >
> > >
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycreatefromc...
> > > > --- It's an ordinary object with the Array prototype.
> > > 
> > > You are right.
> > > 
> > > Maybe a more interesting test then would be `Reflect.construct(Array, [5],
> > > Constructor)`.
> > 
> > Hmm, we seem to be doing something wrong though. If `target` is a builtin
> > function, the result ends up being an exotic <<whatever>>, regardless of
what
> > `newTarget` is, even though it gets the right prototype. I don't think
that's
> > correct.
> 
> wait, I'm wrong. Yeah, adding some extra tests

wait, I'm not wrong. It's so weird! It's like builtin functions set their own
prototype when they're called, I guess

[caitp (gmail), 2015-03-04 14:01:02]

On 2015/03/04 13:59:28, caitp wrote:
> On 2015/03/04 13:54:56, caitp wrote:
> >
>
https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
> > File test/mjsunit/harmony/reflect-construct.js (right):
> > 
> >
>
https://codereview.chromium.org/913073003/diff/120001/test/mjsunit/harmony/re...
> > test/mjsunit/harmony/reflect-construct.js:51: assertSame(1, O[9]);
> > On 2015/03/04 13:30:33, arv wrote:
> > > On 2015/03/04 13:24:14, caitp wrote:
> > > > On 2015/03/04 09:45:01, arv wrote:
> > > > > also check length and Array.isArray to ensure we created an exotic
array
> > > > object.
> > > > 
> > > > That's not exactly correct:
> > > >
> > >
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinarycreatefromc...
> > > > --- It's an ordinary object with the Array prototype.
> > > 
> > > You are right.
> > > 
> > > Maybe a more interesting test then would be `Reflect.construct(Array, [5],
> > > Constructor)`.
> > 
> > Hmm, we seem to be doing something wrong though. If `target` is a builtin
> > function, the result ends up being an exotic <<whatever>>, regardless of
what
> > `newTarget` is, even though it gets the right prototype. I don't think
that's
> > correct.
> 
> wait, I'm wrong. Yeah, adding some extra tests

wait, I'm not wrong. It's so weird! It's like builtin functions set their own
prototype when they're called, I guess

[caitp (gmail), 2015-03-04 14:44:11]

So Runtime_NewObjectHelper creates an object using the initial map of whatever
the constructor was, and then __maybe__ sets __proto__ to the prototype of the
original constructor. That doesn't seem right to me, but I'm sure there are good
reasons for doing this, maybe it's okay to leave it as-is for now?

[Dmitry Lomov (no reviews), 2015-03-04 15:24:09]

On 2015/03/04 14:44:11, caitp wrote:
> So Runtime_NewObjectHelper creates an object using the initial map of whatever
> the constructor was, and then __maybe__ sets __proto__ to the prototype of the
> original constructor. That doesn't seem right to me, but I'm sure there are
good
> reasons for doing this, maybe it's okay to leave it as-is for now?

This is absolutely correct.

  Reflect.construct(Array, [], C)

invokes Array.[[Construct]] trap with [] for argumentsList and C for new Target.
For Arrays this constructs an exotic object (Array) with __proto__ being
C.prototype.

This is what your CL does

[caitp (gmail), 2015-03-04 15:43:00]

On 2015/03/04 15:24:09, Dmitry Lomov (chromium) wrote:
> On 2015/03/04 14:44:11, caitp wrote:
> > So Runtime_NewObjectHelper creates an object using the initial map of
whatever
> > the constructor was, and then __maybe__ sets __proto__ to the prototype of
the
> > original constructor. That doesn't seem right to me, but I'm sure there are
> good
> > reasons for doing this, maybe it's okay to leave it as-is for now?
> 
> This is absolutely correct.
> 
>   Reflect.construct(Array, [], C)
> 
> invokes Array.[[Construct]] trap with [] for argumentsList and C for new
Target.
> For Arrays this constructs an exotic object (Array) with __proto__ being
> C.prototype.
> 
> This is what your CL does

Right, but the actual behaviour is not quite correct.

Here's a sample run:

```
(lldb) r --harmony-reflect
Process 17689 launched: '/Users/caitp/google/v8/out/x64.debug/d8' (x86_64)
V8 version 4.3.0 (candidate) [console: dumb]
d8> function A() {}
undefined
d8> function B() {}
undefined
d8> B.prototype.name = "B"
"B"
d8> Reflect.construct(A, [], B).name
"B"
d8> Reflect.construct(A, [], B) instanceof B
true
d8> Reflect.construct(A, [], B) instanceof A
false
d8> function C() {}
undefined
d8> Reflect.construct(A, [], C) instanceof A
true
d8> Reflect.construct(A, [], C) instanceof C
false
d8> 
```

Notice that if I add a property to B's prototype, then we take the path where we
do set the prototype to the right value. However, when newTarget is `C`, which
has not touched its prototype at all, the prototype is incorrect.

[Dmitry Lomov (no reviews), 2015-03-04 15:54:46]

On 2015/03/04 15:43:00, caitp wrote:
> On 2015/03/04 15:24:09, Dmitry Lomov (chromium) wrote:
> > On 2015/03/04 14:44:11, caitp wrote:
> > > So Runtime_NewObjectHelper creates an object using the initial map of
> whatever
> > > the constructor was, and then __maybe__ sets __proto__ to the prototype of
> the
> > > original constructor. That doesn't seem right to me, but I'm sure there
are
> > good
> > > reasons for doing this, maybe it's okay to leave it as-is for now?
> > 
> > This is absolutely correct.
> > 
> >   Reflect.construct(Array, [], C)
> > 
> > invokes Array.[[Construct]] trap with [] for argumentsList and C for new
> Target.
> > For Arrays this constructs an exotic object (Array) with __proto__ being
> > C.prototype.
> > 
> > This is what your CL does
> 
> Right, but the actual behaviour is not quite correct.
> 
> Here's a sample run:
> 
> ```
> (lldb) r --harmony-reflect
> Process 17689 launched: '/Users/caitp/google/v8/out/x64.debug/d8' (x86_64)
> V8 version 4.3.0 (candidate) [console: dumb]
> d8> function A() {}
> undefined
> d8> function B() {}
> undefined
> d8> B.prototype.name = "B"
> "B"
> d8> Reflect.construct(A, [], B).name
> "B"
> d8> Reflect.construct(A, [], B) instanceof B
> true
> d8> Reflect.construct(A, [], B) instanceof A
> false
> d8> function C() {}
> undefined
> d8> Reflect.construct(A, [], C) instanceof A
> true
> d8> Reflect.construct(A, [], C) instanceof C
> false
> d8> 
> ```
> 
> Notice that if I add a property to B's prototype, then we take the path where
we
> do set the prototype to the right value. However, when newTarget is `C`, which
> has not touched its prototype at all, the prototype is incorrect.
Ah, I see. It is indeed a bug (can be fixed later; you cannot repro that with
classes since for classes we always set .prototype)

[caitp (gmail), 2015-03-11 17:54:47]

dslomov@ do you have an opinion on what Erik was saying, about sharing more code
between ApplyHelper and ConstructHelper?

I personally felt that it made it much harder to read, and the only thing that
can really be shared is the looping over the arguments array.

If there's no opinion, I'll start working on ports so that it can land

[Dmitry Lomov (no reviews), 2015-03-12 11:44:13]

On 2015/03/11 17:54:47, caitp wrote:
> dslomov@ do you have an opinion on what Erik was saying, about sharing more
code
> between ApplyHelper and ConstructHelper?
> 
> I personally felt that it made it much harder to read, and the only thing that
> can really be shared is the looping over the arguments array.
> 
> If there's no opinion, I'll start working on ports so that it can land

The arguments copying loop is big and scary - if we could share that that would
be a win I think.

[caitp (gmail), 2015-03-12 14:44:24]

On 2015/03/12 11:44:13, Dmitry Lomov (chromium) wrote:
> On 2015/03/11 17:54:47, caitp wrote:
> > dslomov@ do you have an opinion on what Erik was saying, about sharing more
> code
> > between ApplyHelper and ConstructHelper?
> > 
> > I personally felt that it made it much harder to read, and the only thing
that
> > can really be shared is the looping over the arguments array.
> > 
> > If there's no opinion, I'll start working on ports so that it can land
> 
> The arguments copying loop is big and scary - if we could share that that
would
> be a win I think.

It's not really that much code, I've put up a version where the looping is
shared though. I would not look at the inter-diff because the rebase was
included in that patchset, but it should be pretty easy to see it.

If that looks alright, I'll start working on some ports

[Dmitry Lomov (no reviews), 2015-03-13 09:39:09]

This is much nice, thanks! A couple of comments

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

https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1057: const int kIndexOffset,
These are not constants anymore, name them indexOffset, argumentsOffset etc.

https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1061: __ movp(key, Operand(rbp, kIndexOffset));
Receiver and key must be LoadDescriptor::{Reeceiver,Name}Register, since you use
a KeyedLoadIC below.
Remove 'receiver' and 'key' arguments

https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1280: // Check the stack for overflow. We are not trying
to catch
Please share the stack overflow check code between this and PrepareApply as well

[caitp (gmail), 2015-03-13 15:10:37]

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

https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1280: // Check the stack for overflow. We are not trying
to catch
On 2015/03/13 09:39:09, Dmitry Lomov (chromium) wrote:
> Please share the stack overflow check code between this and PrepareApply as
well

I notice that the stack overflow check is running before pushing arguments (and
possibly newTarget) to the stack, so the check is really off by 1 or 2 pointers.
Do we care about that?

[Dmitry Lomov (no reviews), 2015-03-13 16:40:03]

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

https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1280: // Check the stack for overflow. We are not trying
to catch
On 2015/03/13 15:10:37, caitp wrote:
> On 2015/03/13 09:39:09, Dmitry Lomov (chromium) wrote:
> > Please share the stack overflow check code between this and PrepareApply as
> well
> 
> I notice that the stack overflow check is running before pushing arguments
(and
> possibly newTarget) to the stack, so the check is really off by 1 or 2
pointers.
> Do we care about that?

We can overshoot, so just make it 2 pointers always

[caitp (gmail), 2015-03-13 18:22:43]

On 2015/03/13 16:40:03, Dmitry Lomov (chromium) wrote:
> https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc
> File src/x64/builtins-x64.cc (right):
> 
>
https://codereview.chromium.org/913073003/diff/180001/src/x64/builtins-x64.cc...
> src/x64/builtins-x64.cc:1280: // Check the stack for overflow. We are not
trying
> to catch
> On 2015/03/13 15:10:37, caitp wrote:
> > On 2015/03/13 09:39:09, Dmitry Lomov (chromium) wrote:
> > > Please share the stack overflow check code between this and PrepareApply
as
> > well
> > 
> > I notice that the stack overflow check is running before pushing arguments
> (and
> > possibly newTarget) to the stack, so the check is really off by 1 or 2
> pointers.
> > Do we care about that?
> 
> We can overshoot, so just make it 2 pointers always

ia32/arm/arm64 ports are done --- it might be more like 4 pointers since limit
and index for iterating over the arguments object are pushed too. If it's not
exploitable to cause a stack overflow with Push instructions, it's probably fine
to leave it

[Dmitry Lomov (no reviews), 2015-03-16 09:42:25]

looking good, just a couple of comments on bootstrapper

https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
src/bootstrapper.cc:1726: Handle<JSGlobalObject> global(JSGlobalObject::cast(
This has to be under if(FLAG_harmony_reflect)

https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
src/bootstrapper.cc:2100: // Install references to functions of the Reflect
object
This should be under FLAG_harmony_reflect. Move it to
IntitializeGlobal_harmony_reflect.

[caitp (gmail), 2015-03-16 13:08:01]

https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
src/bootstrapper.cc:1726: Handle<JSGlobalObject> global(JSGlobalObject::cast(
On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> This has to be under if(FLAG_harmony_reflect) 

Acknowledged.

https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
src/bootstrapper.cc:2100: // Install references to functions of the Reflect
object
On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> This should be under FLAG_harmony_reflect. Move it to
> IntitializeGlobal_harmony_reflect.

I want these functions to be exposed to the runtime whether Reflect is on or not
(because I'd like to be able to use them for spread-calls, rather than the
runtime fn)

[caitp (gmail), 2015-03-16 13:16:21]

On 2015/03/16 13:08:01, caitp wrote:
> https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
> File src/bootstrapper.cc (right):
> 
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> src/bootstrapper.cc:1726: Handle<JSGlobalObject> global(JSGlobalObject::cast(
> On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > This has to be under if(FLAG_harmony_reflect) 
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> src/bootstrapper.cc:2100: // Install references to functions of the Reflect
> object
> On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > This should be under FLAG_harmony_reflect. Move it to
> > IntitializeGlobal_harmony_reflect.
> 
> I want these functions to be exposed to the runtime whether Reflect is on or
not
> (because I'd like to be able to use them for spread-calls, rather than the
> runtime fn)

Updated InitializeGlobal_harmony_reflect() --- I'd prefer that the references
for builtins are set up either way, but one thing that might work is just adding
a `|| FLAG_harmony_spreadcalls` or something later, I guess

[Dmitry Lomov (no reviews), 2015-03-18 10:49:39]

On 2015/03/16 13:16:21, caitp wrote:
> On 2015/03/16 13:08:01, caitp wrote:
> > https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
> > File src/bootstrapper.cc (right):
> > 
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > src/bootstrapper.cc:1726: Handle<JSGlobalObject>
global(JSGlobalObject::cast(
> > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > This has to be under if(FLAG_harmony_reflect) 
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > src/bootstrapper.cc:2100: // Install references to functions of the Reflect
> > object
> > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > This should be under FLAG_harmony_reflect. Move it to
> > > IntitializeGlobal_harmony_reflect.
> > 
> > I want these functions to be exposed to the runtime whether Reflect is on or
> not
> > (because I'd like to be able to use them for spread-calls, rather than the
> > runtime fn)
> 
> Updated InitializeGlobal_harmony_reflect() --- I'd prefer that the references
> for builtins are set up either way, but one thing that might work is just
adding
> a `|| FLAG_harmony_spreadcalls` or something later, I guess

Sorry, I do not follow: your latest patch does not seem to move 'intrenal
builtin' properties setting to IntitalizeGlobal_harmony_reflect

[caitp (gmail), 2015-03-18 13:07:26]

On 2015/03/18 10:49:39, Dmitry Lomov (chromium) wrote:
> On 2015/03/16 13:16:21, caitp wrote:
> > On 2015/03/16 13:08:01, caitp wrote:
> > > https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
> > > File src/bootstrapper.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > > src/bootstrapper.cc:1726: Handle<JSGlobalObject>
> global(JSGlobalObject::cast(
> > > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > > This has to be under if(FLAG_harmony_reflect) 
> > > 
> > > Acknowledged.
> > > 
> > >
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > > src/bootstrapper.cc:2100: // Install references to functions of the
Reflect
> > > object
> > > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > > This should be under FLAG_harmony_reflect. Move it to
> > > > IntitializeGlobal_harmony_reflect.
> > > 
> > > I want these functions to be exposed to the runtime whether Reflect is on
or
> > not
> > > (because I'd like to be able to use them for spread-calls, rather than the
> > > runtime fn)
> > 
> > Updated InitializeGlobal_harmony_reflect() --- I'd prefer that the
references
> > for builtins are set up either way, but one thing that might work is just
> adding
> > a `|| FLAG_harmony_spreadcalls` or something later, I guess
> 
> Sorry, I do not follow: your latest patch does not seem to move 'intrenal
> builtin' properties setting to IntitalizeGlobal_harmony_reflect

I said, I think it would be preferable if the builtins did not require a flag to
access those --- they're not exposed without the flag, but builtins should be
able to use them.

Very well, making the change

[caitp (gmail), 2015-03-18 13:31:40]

On 2015/03/18 13:07:26, caitp wrote:
> On 2015/03/18 10:49:39, Dmitry Lomov (chromium) wrote:
> > On 2015/03/16 13:16:21, caitp wrote:
> > > On 2015/03/16 13:08:01, caitp wrote:
> > > >
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
> > > > File src/bootstrapper.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > > > src/bootstrapper.cc:1726: Handle<JSGlobalObject>
> > global(JSGlobalObject::cast(
> > > > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > > > This has to be under if(FLAG_harmony_reflect) 
> > > > 
> > > > Acknowledged.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > > > src/bootstrapper.cc:2100: // Install references to functions of the
> Reflect
> > > > object
> > > > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > > > This should be under FLAG_harmony_reflect. Move it to
> > > > > IntitializeGlobal_harmony_reflect.
> > > > 
> > > > I want these functions to be exposed to the runtime whether Reflect is
on
> or
> > > not
> > > > (because I'd like to be able to use them for spread-calls, rather than
the
> > > > runtime fn)
> > > 
> > > Updated InitializeGlobal_harmony_reflect() --- I'd prefer that the
> references
> > > for builtins are set up either way, but one thing that might work is just
> > adding
> > > a `|| FLAG_harmony_spreadcalls` or something later, I guess
> > 
> > Sorry, I do not follow: your latest patch does not seem to move 'intrenal
> > builtin' properties setting to IntitalizeGlobal_harmony_reflect
> 
> I said, I think it would be preferable if the builtins did not require a flag
to
> access those --- they're not exposed without the flag, but builtins should be
> able to use them.
> 
> Very well, making the change

Actually, it seems to be problematic to make this change, unless the snapshot is
turned off.

[caitp (gmail), 2015-03-18 13:48:41]

On 2015/03/18 13:31:40, caitp wrote:
> On 2015/03/18 13:07:26, caitp wrote:
> > On 2015/03/18 10:49:39, Dmitry Lomov (chromium) wrote:
> > > On 2015/03/16 13:16:21, caitp wrote:
> > > > On 2015/03/16 13:08:01, caitp wrote:
> > > > >
> https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc
> > > > > File src/bootstrapper.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > > > > src/bootstrapper.cc:1726: Handle<JSGlobalObject>
> > > global(JSGlobalObject::cast(
> > > > > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > > > > This has to be under if(FLAG_harmony_reflect) 
> > > > > 
> > > > > Acknowledged.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/913073003/diff/270016/src/bootstrapper.cc#new...
> > > > > src/bootstrapper.cc:2100: // Install references to functions of the
> > Reflect
> > > > > object
> > > > > On 2015/03/16 09:42:24, Dmitry Lomov (chromium) wrote:
> > > > > > This should be under FLAG_harmony_reflect. Move it to
> > > > > > IntitializeGlobal_harmony_reflect.
> > > > > 
> > > > > I want these functions to be exposed to the runtime whether Reflect is
> on
> > or
> > > > not
> > > > > (because I'd like to be able to use them for spread-calls, rather than
> the
> > > > > runtime fn)
> > > > 
> > > > Updated InitializeGlobal_harmony_reflect() --- I'd prefer that the
> > references
> > > > for builtins are set up either way, but one thing that might work is
just
> > > adding
> > > > a `|| FLAG_harmony_spreadcalls` or something later, I guess
> > > 
> > > Sorry, I do not follow: your latest patch does not seem to move 'intrenal
> > > builtin' properties setting to IntitalizeGlobal_harmony_reflect
> > 
> > I said, I think it would be preferable if the builtins did not require a
flag
> to
> > access those --- they're not exposed without the flag, but builtins should
be
> > able to use them.
> > 
> > Very well, making the change
> 
> Actually, it seems to be problematic to make this change, unless the snapshot
is
> turned off.

Moved it to InitializeGlobal_harmony_reflect() so that it's never in the
snapshot, but that might be problematic later

[Dmitry Lomov (no reviews), 2015-03-19 09:43:02]

lgtm

[caitp (gmail), 2015-03-19 13:19:59]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-03-19 13:20:09]

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

[caitp (gmail), 2015-03-19 14:04:43]

The CQ bit was unchecked by caitpotter88@gmail.com

[caitp (gmail), 2015-03-19 14:25:30]

On 2015/03/19 13:20:09, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/913073003/340001

there was a minor bug in the ARM version... trying that again

[caitp (gmail), 2015-03-19 14:25:37]

The CQ bit was checked by caitpotter88@gmail.com

[caitp (gmail), 2015-03-19 14:25:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dslomov@chromium.org
Link to the patchset: https://codereview.chromium.org/913073003/#ps360001
(title: "Fix arm")

[commit-bot: I haz the power, 2015-03-19 14:25:50]

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

[commit-bot: I haz the power, 2015-03-19 14:47:26]

Committed patchset #20 (id:360001)

[commit-bot: I haz the power, 2015-03-19 14:47:40]

Patchset 20 (id:??) landed as
https://crrev.com/d21fd15467e16f185e511dbfbaeef7caddfe804a
Cr-Commit-Position: refs/heads/master@{#27316}