More accurately record an end position for default parameters in arrows

More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer. This means
that we get accurate-enough positions for the initializers in the eager
parsing step to get the same answers for hole-check-elimination that we
will later during ParseLazy.

In the included test case, for example:

  (function() { ((s = 17, y = s) => s)(); } )();
                        ^2     ^1

The old code would generate a hole check when trying to load
|s| for assignment to |y| (because it treated the closing parentheses
pointed to by "^1" as the "initialization position" of |s|).

The new code uses the comma pointed to by "^2" as the initialization
position of |s|. Since that occurs textually before the load of |s|,
full-codegen knows it can avoid the hole check.

BUG=v8:4908
LOG=n

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

[adamk, 2016-04-20 00:35:32]

adamk@chromium.org changed reviewers:
+ littledan@chromium.org, nikolaos@chromium.org

[nickie, 2016-04-20 11:01:41]

I will not pretend that I understand why this fixes the problem in v8:4908, but
I confirm that it does.  It now does not crash with all the tests that I tried. 
My comments are on a different front.

https://codereview.chromium.org/1900343002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1900343002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3920: if (parameters->Arity() >= Code::kMaxArguments) {
I'm not sure if exactly Code::kMaxArguments (65535) should be allowed or not. 
If yes, then the ">=" should be a ">" here.  I used the following to test this
and it failed:

    python -c 'print "print(((" + ",".join("a"+str(i) for i in range(2**16-1)) +
")=>42)())"' > foo.js
    out/Release/d8 foo.js

https://codereview.chromium.org/1900343002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3925: 
I believe that putting these five lines here does not do the trick.  Where it
was (line 3832), it was checked on each recursive call of
ParseArrowFunctionFormalParameters.  Now, whether that was good or bad is a
different story.  (Or even, the idea of having a recursive
ParseArrowFunctionFormalParameters that overflows the stack in Debug mode before
reaching kMaxArguments.)  I believe that these five lines should go after the
call to ParseArrowFunctionFormalParameters, before line 3929.

[adamk, 2016-04-20 18:48:46]

Description was changed from

==========
More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer.

BUG=v8:4908
LOG=n
==========

to

==========
More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer. This means
that we get accurate-enough positions for the initializers in the eager
parsing step to get the same answers for hole-check-elimination that we
will later during ParseLazy.

In the included test case, for example:

  (function() { ((s = 17, y = s) => s)(); } )();
                        ^2     ^1

The old code would generate a hole check when trying to load
|s| for assignment to |y| (because it treated the closing parentheses
pointed to by "^1" as the "initialization position" of |s|).

The new code uses the comma pointed to by "^2" as the initialization
position of |s|. Since that occurs textually before the load of |s|,
full-codegen knows it can avoid the hole check.

BUG=v8:4908
LOG=n
==========

[adamk, 2016-04-20 18:51:45]

I've tried to add more to the CL description explaining what's going on in this
fix. Also handled Niko's comments.

https://codereview.chromium.org/1900343002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/1900343002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3920: if (parameters->Arity() >= Code::kMaxArguments) {
On 2016/04/20 11:01:41, nickie wrote:
> I'm not sure if exactly Code::kMaxArguments (65535) should be allowed or not. 
> If yes, then the ">=" should be a ">" here.  I used the following to test this
> and it failed:
> 
>     python -c 'print "print(((" + ",".join("a"+str(i) for i in range(2**16-1))
+
> ")=>42)())"' > foo.js
>     out/Release/d8 foo.js

Indeed, ">" is the comparison used elsewhere. Will fix as part of this patch,
though I do think it's somewhat academic.

https://codereview.chromium.org/1900343002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:3925: 
On 2016/04/20 11:01:41, nickie wrote:
> I believe that putting these five lines here does not do the trick.  Where it
> was (line 3832), it was checked on each recursive call of
> ParseArrowFunctionFormalParameters.  Now, whether that was good or bad is a
> different story.  (Or even, the idea of having a recursive
> ParseArrowFunctionFormalParameters that overflows the stack in Debug mode
before
> reaching kMaxArguments.)  I believe that these five lines should go after the
> call to ParseArrowFunctionFormalParameters, before line 3929.

Ah, I misunderstood what the Arity() function did. Moved after the call to
ParseArrowFunctionFormalParameters.

[Dan Ehrenberg, 2016-04-20 19:55:55]

I don't see any problems in this code, but I wonder if we can get around this
issue in a more comprehensive way. Could we just mark the parameter scope as
non-linear (Scope::SetNonlinear) to omit any hole check elimination? This would
be the surest way to make sure that positions (which still could change with
this patch, but hopefully just in an inconsequential way) don't affect the
outcome of what fullcodegen produces.

[Dan Ehrenberg, 2016-04-20 19:56:25]

On 2016/04/20 at 19:55:55, Dan Ehrenberg wrote:
> I don't see any problems in this code, but I wonder if we can get around this
issue in a more comprehensive way. Could we just mark the parameter scope as
non-linear (Scope::SetNonlinear) to omit any hole check elimination? This would
be the surest way to make sure that positions (which still could change with
this patch, but hopefully just in an inconsequential way) don't affect the
outcome of what fullcodegen produces.

The obvious downside is that performance may become worse, however.

[adamk, 2016-04-20 19:59:24]

On 2016/04/20 19:56:25, Dan Ehrenberg wrote:
> On 2016/04/20 at 19:55:55, Dan Ehrenberg wrote:
> > I don't see any problems in this code, but I wonder if we can get around
this
> issue in a more comprehensive way. Could we just mark the parameter scope as
> non-linear (Scope::SetNonlinear) to omit any hole check elimination? This
would
> be the surest way to make sure that positions (which still could change with
> this patch, but hopefully just in an inconsequential way) don't affect the
> outcome of what fullcodegen produces.
> 
> The obvious downside is that performance may become worse, however.

If we were to do something like that, I would want to rename the "nonlinear" bit
to be more explicit about its purpose ("SetNoHoleCheckElimination" is the
strawman).

Is there a reason you'd prefer that approach to this one? You call it "more
comprehensive", but it just sounds more conservative to me.

[Dan Ehrenberg, 2016-04-20 20:07:17]

On 2016/04/20 at 19:59:24, adamk wrote:
> On 2016/04/20 19:56:25, Dan Ehrenberg wrote:
> > On 2016/04/20 at 19:55:55, Dan Ehrenberg wrote:
> > > I don't see any problems in this code, but I wonder if we can get around
this
> > issue in a more comprehensive way. Could we just mark the parameter scope as
> > non-linear (Scope::SetNonlinear) to omit any hole check elimination? This
would
> > be the surest way to make sure that positions (which still could change with
> > this patch, but hopefully just in an inconsequential way) don't affect the
> > outcome of what fullcodegen produces.
> > 
> > The obvious downside is that performance may become worse, however.
> 
> If we were to do something like that, I would want to rename the "nonlinear"
bit to be more explicit about its purpose ("SetNoHoleCheckElimination" is the
strawman).
> 
> Is there a reason you'd prefer that approach to this one? You call it "more
comprehensive", but it just sounds more conservative to me.

I like the idea of the name change.

Agreed, it's just more conservative. The circumstantial reasoning in favor of
this kind of change is based around the fact that this is an area of code that's
on its second bug fix, and I have a general, unsubstantiated worry it might not
be the last. You explained to me offline that the pos still won't necessarily be
equal in the first and second parse, as comments can separate the comma from the
end of the definition. Probably that doesn't do anything, but I am just worried
by the interaction of the various systems.

[adamk, 2016-04-20 20:09:52]

On 2016/04/20 20:07:17, Dan Ehrenberg wrote:
> On 2016/04/20 at 19:59:24, adamk wrote:
> > On 2016/04/20 19:56:25, Dan Ehrenberg wrote:
> > > On 2016/04/20 at 19:55:55, Dan Ehrenberg wrote:
> > > > I don't see any problems in this code, but I wonder if we can get around
> this
> > > issue in a more comprehensive way. Could we just mark the parameter scope
as
> > > non-linear (Scope::SetNonlinear) to omit any hole check elimination? This
> would
> > > be the surest way to make sure that positions (which still could change
with
> > > this patch, but hopefully just in an inconsequential way) don't affect the
> > > outcome of what fullcodegen produces.
> > > 
> > > The obvious downside is that performance may become worse, however.
> > 
> > If we were to do something like that, I would want to rename the "nonlinear"
> bit to be more explicit about its purpose ("SetNoHoleCheckElimination" is the
> strawman).
> > 
> > Is there a reason you'd prefer that approach to this one? You call it "more
> comprehensive", but it just sounds more conservative to me.
> 
> I like the idea of the name change.
> 
> Agreed, it's just more conservative. The circumstantial reasoning in favor of
> this kind of change is based around the fact that this is an area of code
that's
> on its second bug fix, and I have a general, unsubstantiated worry it might
not
> be the last. You explained to me offline that the pos still won't necessarily
be
> equal in the first and second parse, as comments can separate the comma from
the
> end of the definition. Probably that doesn't do anything, but I am just
worried
> by the interaction of the various systems.

How about we give this approach a shot and leave the "disable hole checks for
this scope" in our back pocket if more bugs crop up? I've definitely seen a
steady decrease of bugs in this area, and this fix is _strictly_ an improvement
on the old code (while comments and whitespace can still cause variation among
compilations, it's strictly closer to the right values than it is without this
patch), while also defeating the only related failing test case I know of.

[Dan Ehrenberg, 2016-04-20 20:44:32]

lgtm

[adamk, 2016-04-20 20:46:26]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-04-20 20:46:37]

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

[commit-bot: I haz the power, 2016-04-20 20:48:39]

Description was changed from

==========
More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer. This means
that we get accurate-enough positions for the initializers in the eager
parsing step to get the same answers for hole-check-elimination that we
will later during ParseLazy.

In the included test case, for example:

  (function() { ((s = 17, y = s) => s)(); } )();
                        ^2     ^1

The old code would generate a hole check when trying to load
|s| for assignment to |y| (because it treated the closing parentheses
pointed to by "^1" as the "initialization position" of |s|).

The new code uses the comma pointed to by "^2" as the initialization
position of |s|. Since that occurs textually before the load of |s|,
full-codegen knows it can avoid the hole check.

BUG=v8:4908
LOG=n
==========

to

==========
More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer. This means
that we get accurate-enough positions for the initializers in the eager
parsing step to get the same answers for hole-check-elimination that we
will later during ParseLazy.

In the included test case, for example:

  (function() { ((s = 17, y = s) => s)(); } )();
                        ^2     ^1

The old code would generate a hole check when trying to load
|s| for assignment to |y| (because it treated the closing parentheses
pointed to by "^1" as the "initialization position" of |s|).

The new code uses the comma pointed to by "^2" as the initialization
position of |s|. Since that occurs textually before the load of |s|,
full-codegen knows it can avoid the hole check.

BUG=v8:4908
LOG=n
==========

[commit-bot: I haz the power, 2016-04-20 20:48:40]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-22 19:13:50]

Description was changed from

==========
More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer. This means
that we get accurate-enough positions for the initializers in the eager
parsing step to get the same answers for hole-check-elimination that we
will later during ParseLazy.

In the included test case, for example:

  (function() { ((s = 17, y = s) => s)(); } )();
                        ^2     ^1

The old code would generate a hole check when trying to load
|s| for assignment to |y| (because it treated the closing parentheses
pointed to by "^1" as the "initialization position" of |s|).

The new code uses the comma pointed to by "^2" as the initialization
position of |s|. Since that occurs textually before the load of |s|,
full-codegen knows it can avoid the hole check.

BUG=v8:4908
LOG=n
==========

to

==========
More accurately record an end position for default parameters in arrows

Our previous over-conservative answer caused us to emit hole checks in
full-codegen when eagerly parsing but not when lazily parsing.

With this patch, we use the positions of the BinaryOperations making up
the parameter list (which are the positions of the commas) to determine
the appropriate "end position" for each parameter's initializer. This means
that we get accurate-enough positions for the initializers in the eager
parsing step to get the same answers for hole-check-elimination that we
will later during ParseLazy.

In the included test case, for example:

  (function() { ((s = 17, y = s) => s)(); } )();
                        ^2     ^1

The old code would generate a hole check when trying to load
|s| for assignment to |y| (because it treated the closing parentheses
pointed to by "^1" as the "initialization position" of |s|).

The new code uses the comma pointed to by "^2" as the initialization
position of |s|. Since that occurs textually before the load of |s|,
full-codegen knows it can avoid the hole check.

BUG=v8:4908
LOG=n

Committed: https://crrev.com/e96cbdcdd611fe55d558439ce3854ebe1adfdb1a
Cr-Commit-Position: refs/heads/master@{#35678}
==========

[commit-bot: I haz the power, 2016-04-22 19:13:51]

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