[parser] Fix bug in destructuring binding for catch.

[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern.  This lead to
full-codegen eliminating crucial hole checks when reading such variables
inside the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178
Committed: https://crrev.com/4a4f7175dfdc246f1ed864cd0b86ccb6e837dee3
Cr-Commit-Position: refs/heads/master@{#37569}

[neis, 2016-07-06 12:33:03]

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

[commit-bot: I haz the power, 2016-07-06 12:33:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[neis, 2016-07-06 12:33:47]

Description was changed from

==========
[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern itself.  This lead to
full-codegen eliminating crucial hole checks when reading such variables inside
the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178
==========

to

==========
[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern.  This lead to
full-codegen eliminating crucial hole checks when reading such variables
inside the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178
==========

[commit-bot: I haz the power, 2016-07-06 12:59:38]

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

[commit-bot: I haz the power, 2016-07-06 12:59:38]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-07-06 18:17:41]

lgtm

Interestingly, this seems to use the same strategy (put the initializer position
at the end of the pattern) as parsing parameters did before
https://codereview.chromium.org/1900343002 . Maybe it amounts to the same as
afterwards, as there is only one "parameter" here. Seems valid in this case,
especially since there is no eager/lazy parsing concern, but I wonder if we
should just disable fullcodegen hole check elimination in general (by marking
the catch scope non-linear) when we run into these sorts of bugs.

[neis, 2016-07-07 07:02:54]

On 2016/07/06 18:17:41, Dan Ehrenberg wrote:
> lgtm
> 
> Interestingly, this seems to use the same strategy (put the initializer
position
> at the end of the pattern) as parsing parameters did before
> https://codereview.chromium.org/1900343002 . Maybe it amounts to the same as
> afterwards, as there is only one "parameter" here. Seems valid in this case,
> especially since there is no eager/lazy parsing concern, but I wonder if we
> should just disable fullcodegen hole check elimination in general (by marking
> the catch scope non-linear) when we run into these sorts of bugs.

Interesting, I wasn't aware of that CL and I didn't know about linear scopes
either. I'll wait for Adam to chime in, since he wrote that CL and the code that
I changed.

BTW, strangely, only full-codegen seems to use this position-based hole check
optimization.

[Dan Ehrenberg, 2016-07-07 07:15:09]

lgtm

Adam is OOO through next Wednesday. I think this change is good for now and we
can revisit when he gets back.

[neis, 2016-07-07 07:26:48]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-07-07 07:26:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 07:29:36]

Description was changed from

==========
[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern.  This lead to
full-codegen eliminating crucial hole checks when reading such variables
inside the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178
==========

to

==========
[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern.  This lead to
full-codegen eliminating crucial hole checks when reading such variables
inside the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178
==========

[commit-bot: I haz the power, 2016-07-07 07:29:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-07 07:31:25]

Description was changed from

==========
[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern.  This lead to
full-codegen eliminating crucial hole checks when reading such variables
inside the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178
==========

to

==========
[parser] Fix bug in destructuring binding for catch.

For variables introduced as part of a catch pattern, we used to set their
"initializer position" to the beginning of the pattern.  This lead to
full-codegen eliminating crucial hole checks when reading such variables
inside the pattern itself.

R=adamk@chromium.org, littledan@chromium.org
BUG=v8:5178

Committed: https://crrev.com/4a4f7175dfdc246f1ed864cd0b86ccb6e837dee3
Cr-Commit-Position: refs/heads/master@{#37569}
==========

[commit-bot: I haz the power, 2016-07-07 07:31:26]

Patchset 1 (id:??) landed as
https://crrev.com/4a4f7175dfdc246f1ed864cd0b86ccb6e837dee3
Cr-Commit-Position: refs/heads/master@{#37569}

[adamk, 2016-07-12 21:15:23]

Since this doesn't have the lazy/eager parsing problem of my earlier patch, I
don't think this is dangerous, but I would like a bit more test coverage.

I agree that this is one more strike against full-codegen hole check
elimination, but not sure how much investigation it's worth.

https://codereview.chromium.org/2123953002/diff/1/test/mjsunit/regress/regres...
File test/mjsunit/regress/regress-5178.js (right):

https://codereview.chromium.org/2123953002/diff/1/test/mjsunit/regress/regres...
test/mjsunit/regress/regress-5178.js:6: try { throw {} } catch({a=b, b}) { a+b }
Can you also add a test case where we do a hole check but it passes? Something
like:

try { throw {a: 42} } catch ({a, b = a}) { assertEquals(42, b) }