Implement ES6 rest parameters

Implement ES6 rest parameters

LOG=Y
BUG=v8:2159
R=dslomov@chromium.org, arv@chromium.org, marja@chromium.org

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

[caitp (gmail), 2014-12-23 19:37:24]

I'm making progress on this --- It's not ready to land, but it would be nice to
get some pointers on a few things.

The following known issues are present:

1) Rest parameter construction is broken when optimized (doesn't crash, just
doesn't do anything), and does not trigger deoptimization (mainly because I'm
not quite sure how to do that yet)

2) Rest parameter construction is always a runtime function, although I can
probably implement a fast mode in full-codegen shortly

3) Constructing the rest parameter array mutates the parameters on the stack,
which causes `arguments` to report an incorrect value. (I am not sure what the
best way around this is, since I'd prefer not to always allocate a context for
functions with rest parameters)

4) Parsing rest parameters does not work at all for arrow functions, I'm not
sure the best approach to fixing this. My gut feeling is that
ParseAssignmentExpression would check for a "..." token before doing any work,
and annotate the resulting expression to indicate that it is a rest/spread
expression --- but it would be good to get some pointers for that

[caitp (gmail), 2014-12-23 19:39:03]

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

[bnoordhuis, 2014-12-23 21:51:15]

ben@strongloop.com changed reviewers:
+ ben@strongloop.com

[bnoordhuis, 2014-12-23 21:51:16]

https://codereview.chromium.org/816913003/diff/1/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/816913003/diff/1/src/x64/full-codegen-x64.cc#...
src/x64/full-codegen-x64.cc:254: int offset = num_parameters * kPointerSize;
I think this needs to be kRegisterSize in order to work on x32.  This is
probably not the only place in this file that does it wrong, though.  Maybe uses
of kPointerSize in stack offset calculations should be replaced with a new
kStackSlotSize constant.

[caitp (gmail), 2015-01-01 14:25:20]

I've written a document explaining the current issues with this CL as I see
them, it's shared with the reviewer email addresses

https://docs.google.com/document/d/1P3hUEehXHUXPAuJZMqhJ0kA_-lEjdUquchhv4OPB6...

I have some ideas about how to get this working correctly, just want to get some
feedback on those ideas first.

[caitp (gmail), 2015-01-09 02:33:23]

On 2015/01/01 14:25:20, caitp wrote:
> I've written a document explaining the current issues with this CL as I see
> them, it's shared with the reviewer email addresses
> 
>
https://docs.google.com/document/d/1P3hUEehXHUXPAuJZMqhJ0kA_-lEjdUquchhv4OPB6...
> 
> I have some ideas about how to get this working correctly, just want to get
some
> feedback on those ideas first.

added support for arrow functions, but like I said before, it makes parsing a
bit too liberal. This is fine once the spread operator is implemented, but for
now it would be good to throw in non-arrow-function cases. Can't think of a fast
way to do that, otoh.

[marja, 2015-01-09 15:18:48]

Sorry, I didn't get a chance to have a look at this CL yet but I can do that
next week (the parsing part).

[marja, 2015-01-09 15:31:54]

On 2015/01/09 15:18:48, marja wrote:
> Sorry, I didn't get a chance to have a look at this CL yet but I can do that
> next week (the parsing part).

Actually, for that, or, before I do that, can you add some more tests in
test-parsing.cc, especially for error cases, afaic your test only tests one
arrow func error case but I suppose there's more? (Makes the parsing part easier
to review when I can see quickly which error cases we need to prepare for.)
Also, what's up with the commented-out stuff in the test?

And don't we now need to check the ellipses-ness of expressions in many more
places? So that we don't accidentally start allowing ...[someexpression] in
places where we shouldn't? What's your plan for solving it?

(I'm okay with splitting this CL and landing the arrow function part
separately.)

[caitp (gmail), 2015-01-09 15:40:38]

On 2015/01/09 15:31:54, marja wrote:
> On 2015/01/09 15:18:48, marja wrote:
> > Sorry, I didn't get a chance to have a look at this CL yet but I can do that
> > next week (the parsing part).
> 
> Actually, for that, or, before I do that, can you add some more tests in
> test-parsing.cc, especially for error cases, afaic your test only tests one
> arrow func error case but I suppose there's more? (Makes the parsing part
easier
> to review when I can see quickly which error cases we need to prepare for.)
> Also, what's up with the commented-out stuff in the test?

Yeah, I'll add some parsing tests

> And don't we now need to check the ellipses-ness of expressions in many more
> places? So that we don't accidentally start allowing ...[someexpression] in
> places where we shouldn't? What's your plan for solving it?
>
> (I'm okay with splitting this CL and landing the arrow function part
> separately.)

The easiest thing is to just wait until the spread operator is supported, before
supporting arrow functions, and land rest params for arrow functions as part of
that CL (because, in that case, the parsing won't be too liberal anymore).

[caitp (gmail), 2015-01-09 15:48:55]

On 2015/01/09 15:40:38, caitp wrote:
> On 2015/01/09 15:31:54, marja wrote:
> > On 2015/01/09 15:18:48, marja wrote:
> > > Sorry, I didn't get a chance to have a look at this CL yet but I can do
that
> > > next week (the parsing part).
> > 
> > Actually, for that, or, before I do that, can you add some more tests in
> > test-parsing.cc, especially for error cases, afaic your test only tests one
> > arrow func error case but I suppose there's more? (Makes the parsing part
> easier
> > to review when I can see quickly which error cases we need to prepare for.)
> > Also, what's up with the commented-out stuff in the test?
> 
> Yeah, I'll add some parsing tests
> 
> > And don't we now need to check the ellipses-ness of expressions in many more
> > places? So that we don't accidentally start allowing ...[someexpression] in
> > places where we shouldn't? What's your plan for solving it?
> >
> > (I'm okay with splitting this CL and landing the arrow function part
> > separately.)
> 
> The easiest thing is to just wait until the spread operator is supported,
before
> supporting arrow functions, and land rest params for arrow functions as part
of
> that CL (because, in that case, the parsing won't be too liberal anymore).

On the other hand, it may not be "too harmful" to just parse liberally for a
while, before spread is supported.

Anywhere where the token isn't really allowed to happen, it's going to be an
unexpected token error anyways.

[arv (Not doing code reviews), 2015-01-09 17:01:59]

Great start.

https://codereview.chromium.org/816913003/diff/80001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/816913003/diff/80001/src/ast.h#newcode405
src/ast.h:405: void set_ellipsis(int location) {
This is a bit strange. Normally we would introduce a new ast node for this.

https://codereview.chromium.org/816913003/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/816913003/diff/80001/src/compiler.cc#newcode608
src/compiler.cc:608: int parameter_count = lit->parameter_count();
Maybe add something to FunctionLiteral that returns the static function length
based on rest (now) and default (future) params.

https://codereview.chromium.org/816913003/diff/80001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/816913003/diff/80001/src/parser.cc#newcode3508
src/parser.cc:3508: bool CheckAndDeclareArrowParameter(ParserTraits* traits,
Expression* expression,
I would be totally fine with not supporting rest in arrow functions at this
point... but it is good that we keep these in mind.

https://codereview.chromium.org/816913003/diff/80001/src/parser.cc#newcode3539
src/parser.cc:3539: bool is_rest_param = *is_rest = expression->has_ellipsis();
If/when we add real SpreadExpression AST nodes... `expression->IsSpread()`
maybe?

https://codereview.chromium.org/816913003/diff/80001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/816913003/diff/80001/src/preparser.h#newcode2780
src/preparser.h:2780: // arw malformed.
typo

https://codereview.chromium.org/816913003/diff/80001/src/runtime/runtime-scop...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/816913003/diff/80001/src/runtime/runtime-scop...
src/runtime/runtime-scopes.cc:504: Object** parameters,
Why not Arguments here?

https://codereview.chromium.org/816913003/diff/80001/src/runtime/runtime-scop...
src/runtime/runtime-scopes.cc:512: elements->set(i, *--parameters);
I think this would be cleaner as *args[i + rest_index] or something like that.

https://codereview.chromium.org/816913003/diff/80001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/816913003/diff/80001/src/scanner.cc#newcode602
src/scanner.cc:602: if (c0_ == '.') {
Do we want to do the flag check here?

https://codereview.chromium.org/816913003/diff/80001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/816913003/diff/80001/src/scopes.h#newcode373
src/scopes.h:373: return nullptr;
Maybe use DCHECK instead of returning nullptr?

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
File test/mjsunit/harmony/rest-params.js (right):

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
test/mjsunit/harmony/rest-params.js:7: function strict_test(a, b, ...c) {
strict_test -> strictTest

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
test/mjsunit/harmony/rest-params.js:10: assertTrue(Array.isArray(c));
maybe check Object.getPrototypeOf too?

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
test/mjsunit/harmony/rest-params.js:12: var expected_length = arguments.length
>= 3 ? arguments.length - 2 : 0;
no underscores

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
test/mjsunit/harmony/rest-params.js:16: //assertEquals(c[j++], arguments[i]);
?

Is this not working yet?

[caitp (gmail), 2015-01-09 17:10:08]

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
File test/mjsunit/harmony/rest-params.js (right):

https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
test/mjsunit/harmony/rest-params.js:16: //assertEquals(c[j++], arguments[i]);
On 2015/01/09 17:01:59, arv wrote:
> ?
> 
> Is this not working yet?

I thought I uncommented this, guess I forgot.

The issue here is, arguments[...] accesses look for the value on the stack, for
whatever reason. So, since the value on the stack was replaced with an array, it
gives the wrong value for the index of the rest parameter.

The document I posted earlier covers a few ways to work around this, I'm just
not sure what is the right approach to take here.

[caitp (gmail), 2015-01-15 18:54:57]

On 2015/01/09 17:10:08, caitp wrote:
>
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
> File test/mjsunit/harmony/rest-params.js (right):
> 
>
https://codereview.chromium.org/816913003/diff/80001/test/mjsunit/harmony/res...
> test/mjsunit/harmony/rest-params.js:16: //assertEquals(c[j++], arguments[i]);
> On 2015/01/09 17:01:59, arv wrote:
> > ?
> > 
> > Is this not working yet?
> 
> I thought I uncommented this, guess I forgot.
> 
> The issue here is, arguments[...] accesses look for the value on the stack,
for
> whatever reason. So, since the value on the stack was replaced with an array,
it
> gives the wrong value for the index of the rest parameter.
> 
> The document I posted earlier covers a few ways to work around this, I'm just
> not sure what is the right approach to take here.

solved the one issue with arguments values being clobbered.

Function arity is reported incorrectly under some circumstances, and it's hard
to see why (doesn't seem to happen from full-codegen tests) --- also, I find
that I hit an assertion somewhere if I do something like `function(a, b, ...c) {
return { args: [].slice.call(arguments), rest: c }; }`, it's hard to make this
work right :>

[marja, 2015-01-15 19:41:24]

I started having another look, but then I realized the arrow funcs are not split
out yet and the parsing part is probably not supposed to be correct yet. (After
this thing is removed, the parser part is very thin anyway and looks good.)

https://codereview.chromium.org/816913003/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/816913003/diff/140001/src/preparser.h#newcode...
src/preparser.h:2273: Consume(Token::ELLIPSIS);
So this part is going to go away, right, otherwise we'd allow var x = ...y; ?

[caitp (gmail), 2015-01-15 19:42:44]

https://codereview.chromium.org/816913003/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/816913003/diff/140001/src/preparser.h#newcode...
src/preparser.h:2273: Consume(Token::ELLIPSIS);
On 2015/01/15 19:41:24, marja wrote:
> So this part is going to go away, right, otherwise we'd allow var x = ...y; ?

err, this was supposed to be gone already! it will be gone soon at any rate

[caitp (gmail), 2015-01-20 19:23:48]

> Add a little bit of TF support

I admittedly have no idea what I'm doing here, but this change seems to make
things work with --always-opt --turbo-filter=* in strict mode. Unfortunately,
sloppy mode is terribly broken (lines like `[].slice.call(arguments)` in
conjunction with rest params seem to cause issues in sloppy mode when optimized,
as well as just creating sloppy arguments themselves).

So, it's a good start, still trying to figure that out :< some pointers would be
great, because it's not even clear from reading the code how
BuildArgumentsObject() and the similar method for building rest parameters even
end up in the graph.

[caitp (gmail), 2015-01-20 19:40:51]

On 2015/01/20 19:23:48, caitp wrote:
> > Add a little bit of TF support
> 
> I admittedly have no idea what I'm doing here, but this change seems to make
> things work with --always-opt --turbo-filter=* in strict mode. Unfortunately,
> sloppy mode is terribly broken (lines like `[].slice.call(arguments)` in
> conjunction with rest params seem to cause issues in sloppy mode when
optimized,
> as well as just creating sloppy arguments themselves).
> 
> So, it's a good start, still trying to figure that out :< some pointers would
be
> great, because it's not even clear from reading the code how
> BuildArgumentsObject() and the similar method for building rest parameters
even
> end up in the graph.

**to clarify, "it's not clear to me, from reading the code, how nodes generated
by BuildArgumentsObject() and my new BuildRestArgumentsArray() end up in the
graph, but I suppose Graph::NewNode() implicitly inserts them or something. At
any rate, I'm having trouble finding the root cause of the sloppy mode issues.

[arv (Not doing code reviews), 2015-01-20 20:08:59]

One thing that should make things simpler is that we should create an
unmapped/strict arguments object in the presences of rest parameters. Therefore
the alias trouble that you are running into for sloppy functions will go away.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-createunmappedargume...
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions...

https://codereview.chromium.org/816913003/diff/200001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/816913003/diff/200001/src/ast.h#newcode403
src/ast.h:403: void set_ellipsis(int location) {
I don't think this approach is going to work. Once we have default params and
destructuring we really need an AST node here.

[caitp (gmail), 2015-01-20 20:11:07]

https://codereview.chromium.org/816913003/diff/200001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/816913003/diff/200001/src/ast.h#newcode403
src/ast.h:403: void set_ellipsis(int location) {
On 2015/01/20 20:08:59, arv wrote:
> I don't think this approach is going to work. Once we have default params and
> destructuring we really need an AST node here.

you're right --- I have just been amending commits, so there was no simple way
to revert the arrow function support, so this is just another bit of it left
over. So this should be good, I'll add another patchset to get rid of it :>

[caitp (gmail), 2015-01-21 04:44:58]

Good things:
- bailout of hydrogen if rest parameters are present
- basic implementation in turbofan that seems to work well
- try to always produce strict/unmapped arguments objects when rest params used
- make sure function length is always set correctly

Bad things:
- there is a distinction between "rest arguments used" and "rest arguments in
formal parameters" --- there probably doesn't need to be such a distinction, and
working around it is kind of awkward
- for whatever reason, under certain conditions, sloppy arguments are still
produced (resulting in some failing tests which try to assert that the arguments
object is unmapped) --- however during manual testing (with various optimization
flags) it seems to work correctly
- the whole "is_simple_parameter_list()" thing is very messy and probably shows
up in more places than is really needed

but, it's getting there :> I think all tests should pass once unmapped arguments
are being generated in all cases, just need to figure out why they're not.

[arv (Not doing code reviews), 2015-01-21 16:26:29]

It would also be nice to provide a test to make sure we do not alias the
arguments.

function f(x, ...xs) {
  // assert not strict
  arguments[0] = 1;
  assertEquals(3, x);
  arguments[1] = 2;
  assertArrayEquals([4, 5], xs);
}
f(3, 4, 5);

https://codereview.chromium.org/816913003/diff/240001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/816913003/diff/240001/src/code-stubs.h#newcod...
src/code-stubs.h:1618: virtual CallInterfaceDescriptor
GetCallInterfaceDescriptor() OVERRIDE {
we no longer use virtual when override is present.

https://codereview.chromium.org/816913003/diff/240001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2040: // This hsould never lazy deopt, so it
is fine to send invalid bailout id.
typo

https://codereview.chromium.org/816913003/diff/240001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/816913003/diff/240001/src/objects-inl.h#newco...
src/objects-inl.h:6169: return shared()->scope_info()->IsSimpleParameterList();
or

shared()->is_simple_parameter_list();

https://codereview.chromium.org/816913003/diff/240001/src/preparser.cc
File src/preparser.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/preparser.cc#newcod...
src/preparser.cc:875: is_rest = peek() == Token::ELLIPSIS &&
allow_harmony_rest_params();
Code duplication of preparser/parser makes me sad.

No need to fix though...

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:494: DCHECK(callee->is_simple_parameter_list());
Should be

DCHECK(!callee->is_simple_parameter_list());

If you run the test in debug mode you might be able to find the case where you
end up with a sloppy arg where you expect a strict arg.

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:513: parameters -= rest_index;
I still think this would be cleaner if you didn't do pointer arithmetics.

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:547: // Determine parameter location on the stack
and dispatch on language mode.
" dispatch on language mode"

What does that mean?

https://codereview.chromium.org/816913003/diff/240001/src/scanner.cc
File src/scanner.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/scanner.cc#newcode602
src/scanner.cc:602: if (c0_ == '.') {
We could do the feature flag check here as well?

https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc#newcode185
src/scopes.cc:185: rest_parameter_ = nullptr;
NULL for consistency?

https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc#newcode1309
src/scopes.cc:1309: if (rest_parameter_ && !MustAllocate(rest_parameter_)) {
I'm curious. What is going on here?

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:867: // Patch the arguments.length and the parameters
pointer.
Maybe we should refactor this to not duplicate this code?

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:873: StandardFrameConstants::kCallerSPOffset));
wrong indentation

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:883: /*(__ movp(rdx, rcx);
Remove?

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:894: // __ Allocate(rcx, rax, rdx, rbx, &runtime,
TAG_OBJECT);
remove?

https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:237: int rest_index;
Is rest_index -1 if not set? Maybe do the test for that before creating
rest_param?

https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:251: Operand(rbp,
StandardFrameConstants::kCallerSPOffset + offset));
indentation

https://codereview.chromium.org/816913003/diff/240001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/rest-params.js (right):

https://codereview.chromium.org/816913003/diff/240001/test/mjsunit/harmony/re...
test/mjsunit/harmony/rest-params.js:40: function sloppySlowTest(a, a, ...c) {
This should be a SyntaxError.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions...

Maybe file a bug so we don't forget about this?

[caitp (gmail), 2015-01-21 18:17:55]

thanks, great comments =)

https://codereview.chromium.org/816913003/diff/240001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/816913003/diff/240001/src/code-stubs.h#newcod...
src/code-stubs.h:1618: virtual CallInterfaceDescriptor
GetCallInterfaceDescriptor() OVERRIDE {
On 2015/01/21 16:26:28, arv wrote:
> we no longer use virtual when override is present.

Acknowledged.

https://codereview.chromium.org/816913003/diff/240001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2040: // This hsould never lazy deopt, so it
is fine to send invalid bailout id.
On 2015/01/21 16:26:28, arv wrote:
> typo

Acknowledged.

https://codereview.chromium.org/816913003/diff/240001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/816913003/diff/240001/src/objects-inl.h#newco...
src/objects-inl.h:6169: return shared()->scope_info()->IsSimpleParameterList();
On 2015/01/21 16:26:28, arv wrote:
> or
> 
> shared()->is_simple_parameter_list();

Acknowledged.

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
File src/runtime/runtime-scopes.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:513: parameters -= rest_index;
On 2015/01/21 16:26:28, arv wrote:
> I still think this would be cleaner if you didn't do pointer arithmetics.

I think it's easier to read actually, otherwise it's like `parameters[arithmetic
producing a negative index]` which is really confusing.

https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-sco...
src/runtime/runtime-scopes.cc:547: // Determine parameter location on the stack
and dispatch on language mode.
On 2015/01/21 16:26:28, arv wrote:
> " dispatch on language mode"
> 
> What does that mean?

The frame iterator stuff was copied from NewArguments (along with the comment),
it's talking about strict mode --- but yeah it doesn't really apply here

https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc#newcode1309
src/scopes.cc:1309: if (rest_parameter_ && !MustAllocate(rest_parameter_)) {
On 2015/01/21 16:26:28, arv wrote:
> I'm curious. What is going on here?

MustAllocate() will return true if eval() is used, or if the variable is bound
to a VariableProxy, which I believe should happen whenever it is referenced
somewhere in the scope (but I may be wrong about that). So the idea is, if it
doesn't get used, don't allocate it.

This is part of the "there is a distinction between "rest arguments used" and
"rest arguments in formal parameters" --- there probably doesn't need to be such
a distinction, and working around it is kind of awkward" that I mentioned in the
comment.

It might not be worth worrying about this yet.

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:867: // Patch the arguments.length and the parameters
pointer.
On 2015/01/21 16:26:28, arv wrote:
> Maybe we should refactor this to not duplicate this code?

it would be nice, but it's hard as there's no guarantee the arguments access
stub was used/will be used

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:873: StandardFrameConstants::kCallerSPOffset));
On 2015/01/21 16:26:28, arv wrote:
> wrong indentation

Acknowledged.

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:883: /*(__ movp(rdx, rcx);
On 2015/01/21 16:26:28, arv wrote:
> Remove?

I was trying to figure out a way to allocate and set up the maps from here, but
yeah for now I guess it can just always call runtime.

https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:894: // __ Allocate(rcx, rax, rdx, rbx, &runtime,
TAG_OBJECT);
On 2015/01/21 16:26:28, arv wrote:
> remove?

Acknowledged.

https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:251: Operand(rbp,
StandardFrameConstants::kCallerSPOffset + offset));
On 2015/01/21 16:26:28, arv wrote:
> indentation

Acknowledged.

https://codereview.chromium.org/816913003/diff/240001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/rest-params.js (right):

https://codereview.chromium.org/816913003/diff/240001/test/mjsunit/harmony/re...
test/mjsunit/harmony/rest-params.js:40: function sloppySlowTest(a, a, ...c) {
On 2015/01/21 16:26:28, arv wrote:
> This should be a SyntaxError.
> 
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions...
> 
> Maybe file a bug so we don't forget about this?

hmm you're right. It should be a syntax error, I'll fix it up in this CL

[caitp (gmail), 2015-01-22 01:42:22]

So, there's only one problem left really, that I can see.

The generated code seems to always try to use the rest index of the first
function with rest parameters, for instance:

```
function a(...args) {
  if (args.length !== 3) throw "wrong length (a)!";
  return JSON.stringify(args);
}

function b(a, ...args) {
  if (args.length !== 2) throw "wrong length (b)!";
  return JSON.stringify(args);
}

print(a(1,2,3)); // works as expected
print(b(1,2,3)); // throws, because `b` thinks that its rest index is still 0,
when really it should be 1
```

I'm not sure what the cause for this is, since during code generation the stub
seems to be invoked with the right index each time.

I've added new tests to try and figure this out and make sure it doesn't break
again

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

On 2015/01/22 01:42:22, caitp wrote:
> So, there's only one problem left really, that I can see.
> 
> The generated code seems to always try to use the rest index of the first
> function with rest parameters, for instance:
> 
> ```
> function a(...args) {
>   if (args.length !== 3) throw "wrong length (a)!";
>   return JSON.stringify(args);
> }
> 
> function b(a, ...args) {
>   if (args.length !== 2) throw "wrong length (b)!";
>   return JSON.stringify(args);
> }
> 
> print(a(1,2,3)); // works as expected
> print(b(1,2,3)); // throws, because `b` thinks that its rest index is still 0,
> when really it should be 1
> ```
> 
> I'm not sure what the cause for this is, since during code generation the stub
> seems to be invoked with the right index each time.
> 
> I've added new tests to try and figure this out and make sure it doesn't break
> again

of course, the issue is that the code stub is cached, and always looked up with
an identical key... fixed by encoding rest_index in minor_key_. All tests
passing :>

[caitp (gmail), 2015-01-22 02:11:10]

On 2015/01/22 02:02:03, caitp wrote:
> On 2015/01/22 01:42:22, caitp wrote:
> > So, there's only one problem left really, that I can see.
> > 
> > The generated code seems to always try to use the rest index of the first
> > function with rest parameters, for instance:
> > 
> > ```
> > function a(...args) {
> >   if (args.length !== 3) throw "wrong length (a)!";
> >   return JSON.stringify(args);
> > }
> > 
> > function b(a, ...args) {
> >   if (args.length !== 2) throw "wrong length (b)!";
> >   return JSON.stringify(args);
> > }
> > 
> > print(a(1,2,3)); // works as expected
> > print(b(1,2,3)); // throws, because `b` thinks that its rest index is still
0,
> > when really it should be 1
> > ```
> > 
> > I'm not sure what the cause for this is, since during code generation the
stub
> > seems to be invoked with the right index each time.
> > 
> > I've added new tests to try and figure this out and make sure it doesn't
break
> > again
> 
> of course, the issue is that the code stub is cached, and always looked up
with
> an identical key... fixed by encoding rest_index in minor_key_. All tests
> passing :>

...mostly passing, at any rate...

[caitp (gmail), 2015-01-22 02:54:50]

On 2015/01/22 02:11:10, caitp wrote:
> On 2015/01/22 02:02:03, caitp wrote:
> > On 2015/01/22 01:42:22, caitp wrote:
> > > So, there's only one problem left really, that I can see.
> > > 
> > > The generated code seems to always try to use the rest index of the first
> > > function with rest parameters, for instance:
> > > 
> > > ```
> > > function a(...args) {
> > >   if (args.length !== 3) throw "wrong length (a)!";
> > >   return JSON.stringify(args);
> > > }
> > > 
> > > function b(a, ...args) {
> > >   if (args.length !== 2) throw "wrong length (b)!";
> > >   return JSON.stringify(args);
> > > }
> > > 
> > > print(a(1,2,3)); // works as expected
> > > print(b(1,2,3)); // throws, because `b` thinks that its rest index is
still
> 0,
> > > when really it should be 1
> > > ```
> > > 
> > > I'm not sure what the cause for this is, since during code generation the
> stub
> > > seems to be invoked with the right index each time.
> > > 
> > > I've added new tests to try and figure this out and make sure it doesn't
> break
> > > again
> > 
> > of course, the issue is that the code stub is cached, and always looked up
> with
> > an identical key... fixed by encoding rest_index in minor_key_. All tests
> > passing :>
> 
> ...mostly passing, at any rate...

all of the non-rest-params failures were fixed with the last patch-set.

only 2 unexpected failures from `make check` now for me:

```
=== mjsunit/harmony/rest-params ===                                            
============ Stress 1/5 ============
============ Stress 2/5 ============
============ Stress 3/5 ============
============ Stress 4/5 ============
============ Stress 5/5 ============
test/mjsunit/harmony/rest-params.js:0: TypeError: Cannot read property
'constructor' of undefined
// Copyright 2014 the V8 project authors. All rights reserved.
^
TypeError: Cannot read property 'constructor' of undefined
    at strictTest (test/mjsunit/harmony/rest-params.js:21:24)
    at testRestParamsStrictMode (test/mjsunit/harmony/rest-params.js:57:3)
    at test/mjsunit/harmony/rest-params.js:65:3
Command: out/x64.release/d8 --test --random-seed=-293474262 --stress-opt
--always-opt --nohard-abort --nodead-code-elimination --nofold-constants
--harmony-rest-parameters --harmony-arrow-functions test/mjsunit/mjsunit.js
test/mjsunit/harmony/rest-params.js

=== mjsunit/harmony/rest-params ===                                            
test/mjsunit/mjsunit.js:178: Failure: expected <Object()> found <3>
    throw new MjsUnitAssertionError(message);
    ^
Error
    at new MjsUnitAssertionError (test/mjsunit/mjsunit.js:31:16)
    at fail (test/mjsunit/mjsunit.js:178:11)
    at assertEquals (test/mjsunit/mjsunit.js:248:7)
    at sloppyTest (test/mjsunit/harmony/rest-params.js:40:33)
    at testRestParamssloppyMode (test/mjsunit/harmony/rest-params.js:101:3)
    at test/mjsunit/harmony/rest-params.js:107:3
Command: out/x64.release/d8 --test --random-seed=-293474262
--turbo-deoptimization --turbo-filter=* --always-opt --nohard-abort
--nodead-code-elimination --nofold-constants --harmony-rest-parameters
--harmony-arrow-functions test/mjsunit/mjsunit.js
test/mjsunit/harmony/rest-params.js
[02:48|% 100|+ 27016|-   6]: Done 
```

4 other errors from intl402, but I always get those. So this just looks like a
few more opt/deopt hiccups.

[caitp (gmail), 2015-01-22 16:45:10]

All working, except for the rest-params stress-opt test which fails (I guess the
bailout doesn't work in that case? I'm not sure how I'd ignore the failure for
that in mjsunit.status, though)

It's probably time to start cleaning up the patch and make sure it fits in
nicely with the rest of v8, so comments welcome when folks have time

[arv (Not doing code reviews), 2015-01-22 21:33:18]

I have no suggestions at this time.

Dmitry, care to have a look?

[caitp (gmail), 2015-01-23 15:56:17]

Patchset #1 (id:1) has been deleted

[caitp (gmail), 2015-01-23 15:56:33]

Patchset #1 (id:20001) has been deleted

[caitp (gmail), 2015-01-23 15:56:47]

Patchset #1 (id:40001) has been deleted

[caitp (gmail), 2015-01-23 15:57:03]

Patchset #1 (id:60001) has been deleted

[caitp (gmail), 2015-01-23 15:57:17]

Patchset #1 (id:80001) has been deleted

[caitp (gmail), 2015-01-23 15:57:33]

Patchset #1 (id:100001) has been deleted

[caitp (gmail), 2015-01-23 15:57:49]

Patchset #1 (id:120001) has been deleted

[caitp (gmail), 2015-01-23 15:58:09]

Patchset #2 (id:160001) has been deleted

[caitp (gmail), 2015-01-23 15:58:57]

Patchset #1 (id:140001) has been deleted

[caitp (gmail), 2015-01-23 15:59:04]

Patchset #1 (id:180001) has been deleted

[caitp (gmail), 2015-01-23 15:59:17]

Patchset #1 (id:200001) has been deleted

[caitp (gmail), 2015-01-23 15:59:29]

Patchset #1 (id:220001) has been deleted

[caitp (gmail), 2015-01-23 15:59:40]

Patchset #1 (id:240001) has been deleted

[caitp (gmail), 2015-01-23 15:59:48]

Patchset #1 (id:260001) has been deleted

[caitp (gmail), 2015-01-23 15:59:58]

Patchset #1 (id:280001) has been deleted

[caitp (gmail), 2015-01-23 16:00:13]

Patchset #1 (id:300001) has been deleted

[caitp (gmail), 2015-01-23 16:00:22]

Patchset #1 (id:320001) has been deleted

[caitp (gmail), 2015-01-27 17:27:30]

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

[caitp (gmail), 2015-01-27 17:28:29]

CC adamk@ to bug an extra person on this (no pressure, if/when you have time)

[caitp (gmail), 2015-01-28 19:00:29]

PTAL cleaned up a bit and added some ports --- still breaks on the stress opt
tests, because rest params aren't set up in crankshaft (not quite sure how to
ignore that specific test).

[Michael Starzinger, 2015-01-29 20:50:50]

The TurboFan part looks OK to me, didn't look at the rest. To answer your
question: The runtime call you added is wired into the graph in two ways.

1) Implicitly through an effect-dependency, this is done implicitly by
GraphBuilder::NewNode() as you suspected. Every runtime call produces an
"effect" that subsequent nodes will then depend on.

2) The call to BuildVariableAssignment() either simulates a store to a
stack-allocated variable and keeps track of the node in
GraphBuilder::Environment or it actually produces a JSStoreContext node for
context-allocated variables.

In general nodes in the graph are only alive if they are reachable from "end"
somehow. If some nodes turn out not to be reachable from "end" then they just
fall off the graph and that is working as intended.

[arv (Not doing code reviews), 2015-01-30 03:53:22]

This patch is getting pretty big. Could you split the parser into its own CL.
That part is pretty non controversial and it might make it simpler to track down
the remaining issues.

https://codereview.chromium.org/816913003/diff/510001/src/mips/code-stubs-mip...
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/816913003/diff/510001/src/mips/code-stubs-mip...
src/mips/code-stubs-mips.cc:2015: void
RestParamAccessStub::GenerateNew(MacroAssembler* masm) {
You don't need to do mips. Only ia32, x64, arm and arm64. The rest are optional
and gets ported by other people. They will of course be happy if you do these.

https://codereview.chromium.org/816913003/diff/510001/src/scopes.cc
File src/scopes.cc (right):

https://codereview.chromium.org/816913003/diff/510001/src/scopes.cc#newcode462
src/scopes.cc:462: DCHECK(rest_parameter_ == NULL);
DCHECK_EQ

[caitp (gmail), 2015-01-30 04:11:13]

On 2015/01/30 03:53:22, arv wrote:
> This patch is getting pretty big. Could you split the parser into its own CL.
> That part is pretty non controversial and it might make it simpler to track
down
> the remaining issues.
> 

I'll have that up tonight

>
https://codereview.chromium.org/816913003/diff/510001/src/mips/code-stubs-mip...
> File src/mips/code-stubs-mips.cc (right):
> 
>
https://codereview.chromium.org/816913003/diff/510001/src/mips/code-stubs-mip...
> src/mips/code-stubs-mips.cc:2015: void
> RestParamAccessStub::GenerateNew(MacroAssembler* masm) {
> You don't need to do mips. Only ia32, x64, arm and arm64. The rest are
optional
> and gets ported by other people. They will of course be happy if you do these.
> 

The codegen stuff is all really baseline and basic (just wrappers for a runtime
call) so they will probably find some work to make them better... I can remove
the mips stuff if it makes it easier to review

> https://codereview.chromium.org/816913003/diff/510001/src/scopes.cc
> File src/scopes.cc (right):
> 
> https://codereview.chromium.org/816913003/diff/510001/src/scopes.cc#newcode462
> src/scopes.cc:462: DCHECK(rest_parameter_ == NULL);
> DCHECK_EQ

Acknowledged

[arv (Not doing code reviews), 2015-01-30 04:23:27]

On 2015/01/30 04:11:13, caitp wrote:

>
https://codereview.chromium.org/816913003/diff/510001/src/mips/code-stubs-mip...
> > src/mips/code-stubs-mips.cc:2015: void
> > RestParamAccessStub::GenerateNew(MacroAssembler* masm) {
> > You don't need to do mips. Only ia32, x64, arm and arm64. The rest are
> optional
> > and gets ported by other people. They will of course be happy if you do
these.
> > 
> 
> The codegen stuff is all really baseline and basic (just wrappers for a
runtime
> call) so they will probably find some work to make them better... I can remove
> the mips stuff if it makes it easier to review

No need to remove it.

[caitp (gmail), 2015-01-30 15:26:41]

Patchset #10 (id:510001) has been deleted

[caitp (gmail), 2015-02-02 14:56:23]

Patchset #10 (id:530001) has been deleted

[arv (Not doing code reviews), 2015-02-05 22:50:58]

This looks very solid to me. I'll try to apply to patch locally and see if I can
repro the stress-opt issue.

https://codereview.chromium.org/816913003/diff/570001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/rest-params.js (right):

https://codereview.chromium.org/816913003/diff/570001/test/mjsunit/harmony/re...
test/mjsunit/harmony/rest-params.js:5: // Flags: --harmony-rest-parameters
--harmony-arrow-functions
remove arrow flag for now?

[caitp (gmail), 2015-02-06 02:00:02]

Patchset #11 (id:570001) has been deleted

[caitp (gmail), 2015-02-06 02:01:22]

On 2015/02/05 22:50:58, arv wrote:
> This looks very solid to me. I'll try to apply to patch locally and see if I
can
> repro the stress-opt issue.
> 
>
https://codereview.chromium.org/816913003/diff/570001/test/mjsunit/harmony/re...
> File test/mjsunit/harmony/rest-params.js (right):
> 
>
https://codereview.chromium.org/816913003/diff/570001/test/mjsunit/harmony/re...
> test/mjsunit/harmony/rest-params.js:5: // Flags: --harmony-rest-parameters
> --harmony-arrow-functions
> remove arrow flag for now?

done

[Dmitry Lomov (no reviews), 2015-02-11 00:30:50]

Just a couple of typos, still reviewing

https://codereview.chromium.org/816913003/diff/610001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/816913003/diff/610001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:248: Comment cmnt(masm_, "[ Allocate rest
parametere array");
Typo

https://codereview.chromium.org/816913003/diff/610001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/816913003/diff/610001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:216: 
Stray edit

[caitp (gmail), 2015-02-11 21:05:54]

https://codereview.chromium.org/816913003/diff/610001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/816913003/diff/610001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:248: Comment cmnt(masm_, "[ Allocate rest
parametere array");
On 2015/02/11 00:30:50, Dmitry Lomov (chromium) wrote:
> Typo

Oops! Fixed

https://codereview.chromium.org/816913003/diff/610001/src/x64/full-codegen-x6...
File src/x64/full-codegen-x64.cc (right):

https://codereview.chromium.org/816913003/diff/610001/src/x64/full-codegen-x6...
src/x64/full-codegen-x64.cc:216: 
On 2015/02/11 00:30:50, Dmitry Lomov (chromium) wrote:
> Stray edit

Fixed.

[arv (Not doing code reviews), 2015-02-12 21:48:43]

Are there still issues with this CL?

https://codereview.chromium.org/816913003/diff/630001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/816913003/diff/630001/src/objects.h#newcode4338
src/objects.h:4338: class IsSimpleParameterListField : public BitField<bool, 13,
1> {};
14 or AsmFunctionField::kNext

[caitp (gmail), 2015-02-12 22:12:04]

It still fails the 5th stress-opt run (rest parameter is undefined rather than
an array).

If you add --turbo-filter=* to the command line, it passes beautifully, so it
seems like on the 5th run, instead of aborting compilation, it just keeps going
and never assigns the rest parameter.

I've tried adding a simple crankshaft implementation just to make it pass that,
but it doesn't seem to like adding new instructions at that point. It would be
better if it can get around having no crankshaft implementation at all

https://codereview.chromium.org/816913003/diff/630001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/816913003/diff/630001/src/objects.h#newcode4338
src/objects.h:4338: class IsSimpleParameterListField : public BitField<bool, 13,
1> {};
On 2015/02/12 21:48:43, arv wrote:
> 14 or AsmFunctionField::kNext

Oops, fixed! I'm going with the kNext option, seems less error-prone

[caitp (gmail), 2015-02-13 14:28:51]

PTAL --- I've added a few lines to call AbortOptimization() in compiler.cc if a
scope has a rest parameter, it fixes the stress-opt failure. So, I think
everything is okay now

[caitp (gmail), 2015-02-13 14:32:57]

On 2015/02/13 14:28:51, caitp wrote:
> PTAL --- I've added a few lines to call AbortOptimization() in compiler.cc if
a
> scope has a rest parameter, it fixes the stress-opt failure. So, I think
> everything is okay now

erk, I'm wrong... the line I added is wrong, but weirdly if I correct it, it
still fails. investigating further :(

[caitp (gmail), 2015-02-13 15:09:52]

Patchset #15 (id:670001) has been deleted

[caitp (gmail), 2015-02-13 15:35:54]

On 2015/02/13 14:32:57, caitp wrote:
> On 2015/02/13 14:28:51, caitp wrote:
> > PTAL --- I've added a few lines to call AbortOptimization() in compiler.cc
if
> a
> > scope has a rest parameter, it fixes the stress-opt failure. So, I think
> > everything is okay now
> 
> erk, I'm wrong... the line I added is wrong, but weirdly if I correct it, it
> still fails. investigating further :(

weirdly, using set_dont_optimize_reason() from the parser seems to fix the
stress-opt issue without breaking anything else.

If there are any pointers as to why crankshaft complains if I try to add
additional instructions in SetUpScope(), it might be possible to enable some
optimizations and make it slightly less bad

[Dmitry Lomov (no reviews), 2015-02-13 16:27:14]

lgtm, I have one question

https://codereview.chromium.org/816913003/diff/710001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/816913003/diff/710001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:285: if (is_strict(language_mode()) ||
!is_simple_parameter_list()) {
Is this just temporary and we later add support for sloppy args with rest
parameters?

[caitp (gmail), 2015-02-13 16:30:45]

https://codereview.chromium.org/816913003/diff/710001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/816913003/diff/710001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:285: if (is_strict(language_mode()) ||
!is_simple_parameter_list()) {
On 2015/02/13 16:27:14, Dmitry Lomov (chromium) wrote:
> Is this just temporary and we later add support for sloppy args with rest
> parameters?

This is specced (FunctionDeclarationInstantiation, 9.2.13) --- always create
unmapped arguments object IsSimpleParameterList() is false

```
21. If argumentsObjectNeeded is true, then
    a. If strict is true or if simpleParameterList is false, then
        i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
    b. Else,
        i. NOTE mapped argument object is only provided for non-strict functions
that don’t have a rest parameter, any parameter default value initializers, or
any destructured parameters .
        ii. Let ao be CreateMappedArgumentsObject(func, formals, argumentsList,
env).
```

[Dmitry Lomov (no reviews), 2015-02-13 16:41:09]

https://codereview.chromium.org/816913003/diff/710001/src/arm64/full-codegen-...
File src/arm64/full-codegen-arm64.cc (right):

https://codereview.chromium.org/816913003/diff/710001/src/arm64/full-codegen-...
src/arm64/full-codegen-arm64.cc:285: if (is_strict(language_mode()) ||
!is_simple_parameter_list()) {
On 2015/02/13 16:30:45, caitp wrote:
> On 2015/02/13 16:27:14, Dmitry Lomov (chromium) wrote:
> > Is this just temporary and we later add support for sloppy args with rest
> > parameters?
> 
> This is specced (FunctionDeclarationInstantiation, 9.2.13) --- always create
> unmapped arguments object IsSimpleParameterList() is false
> 
> ```
> 21. If argumentsObjectNeeded is true, then
>     a. If strict is true or if simpleParameterList is false, then
>         i. Let ao be CreateUnmappedArgumentsObject(argumentsList).
>     b. Else,
>         i. NOTE mapped argument object is only provided for non-strict
functions
> that don’t have a rest parameter, any parameter default value initializers, or
> any destructured parameters .
>         ii. Let ao be CreateMappedArgumentsObject(func, formals,
argumentsList,
> env).
> ```

Ah, thanks!

[arv (Not doing code reviews), 2015-02-13 16:56:44]

LGTM

Maybe you can add a test that triggers lazy parsing too? See
https://codereview.chromium.org/923683002/diff/20001/test/mjsunit/harmony/cla...

https://codereview.chromium.org/816913003/diff/710001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/816913003/diff/710001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:1158: // Check if the calling frame is an arguments
adaptor frame.
Out of scope for this CL but I think this code is repeated in a few places now.
We should refactor it.

https://codereview.chromium.org/816913003/diff/710001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/816913003/diff/710001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:849: //
remove empty line

[caitp (gmail), 2015-02-13 17:25:58]

New patchsets have been uploaded after l-g-t-m from
dslomov@chromium.org,arv@chromium.org

[arv (Not doing code reviews), 2015-02-13 17:49:34]

Still LGTM

[caitp (gmail), 2015-02-13 20:13:00]

New patchsets have been uploaded after l-g-t-m from arv@chromium.org

[caitp (gmail), 2015-02-13 22:17:39]

On 2015/02/13 17:49:34, arv wrote:
> Still LGTM

had to make one fixup to make turbofan happy, passing locally

[caitp (gmail), 2015-02-13 22:24:09]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-02-13 22:24:51]

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

[commit-bot: I haz the power, 2015-02-13 22:31:56]

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

[commit-bot: I haz the power, 2015-02-13 22:31:58]

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

[caitp (gmail), 2015-02-13 22:33:18]

woo typos :<

[caitp (gmail), 2015-02-13 23:37:04]

The CQ bit was checked by caitpotter88@gmail.com

[commit-bot: I haz the power, 2015-02-13 23:37:11]

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

[commit-bot: I haz the power, 2015-02-14 00:15:06]

Committed patchset #20 (id:790001)

[commit-bot: I haz the power, 2015-02-14 00:15:28]

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