Store the scope info in catch contexts

Store the scope info in catch contexts

Since the extension field is already used for the catch name, store a
ContextExtension there instead.

In the future, this will allow for chaining ScopeInfos together, so we
no longer need a context chain for lazy parsing / compilation.

BUG=v8:5215
R=bmeurer@chromium.org,neis@chromium.org,marja@chromium.org

Committed: https://crrev.com/9b6ff3a8f698eeef5f24d5403709ca6fb3251914
Cr-Commit-Position: refs/heads/master@{#39164}

[jochen (gone - plz use gerrit), 2016-09-01 14:33:16]

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

[commit-bot: I haz the power, 2016-09-01 14:33:28]

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

[jochen (gone - plz use gerrit), 2016-09-01 14:35:48]

ptal

[commit-bot: I haz the power, 2016-09-01 14:57:07]

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

[commit-bot: I haz the power, 2016-09-01 14:57:08]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-02 07:39:35]

(I read through this CL, but I can't really tell if it's correct or not.
Deferring the review to people who know.)

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h#...
src/compiler/js-operator.h:191: Handle<String> const catch_name_;
This is an uncommon way to do const, a more common would be "const
Handle<String>".

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h#...
src/compiler/js-operator.h:202: std::ostream& operator<<(std::ostream&,
CreateCatchContextParameters const&);
What's up with this style?

[jochen (gone - plz use gerrit), 2016-09-02 08:27:15]

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h#...
src/compiler/js-operator.h:191: Handle<String> const catch_name_;
On 2016/09/02 at 07:39:35, marja wrote:
> This is an uncommon way to do const, a more common would be "const
Handle<String>".

Just copying what the other operators in this file do

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h#...
src/compiler/js-operator.h:202: std::ostream& operator<<(std::ostream&,
CreateCatchContextParameters const&);
On 2016/09/02 at 07:39:35, marja wrote:
> What's up with this style?

you mean like missing parameter names?

[neis, 2016-09-02 09:09:46]

lgtm, didn't look at compiler

[jochen (gone - plz use gerrit), 2016-09-02 09:19:40]

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

[commit-bot: I haz the power, 2016-09-02 09:19:44]

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

[jochen (gone - plz use gerrit), 2016-09-02 09:20:28]

jochen@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[jochen (gone - plz use gerrit), 2016-09-02 09:20:29]

+mstarzinger for all the things

Georg/Marja, I also hooked up the now available ScopeInfo to the CATCH_SCOPE
scope when deserializing scope chains. ptal

[marja, 2016-09-02 09:34:55]

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/2302013002/diff/1/src/compiler/js-operator.h#...
src/compiler/js-operator.h:202: std::ostream& operator<<(std::ostream&,
CreateCatchContextParameters const&);
On 2016/09/02 08:27:15, jochen wrote:
> On 2016/09/02 at 07:39:35, marja wrote:
> > What's up with this style?
> 
> you mean like missing parameter names?

No, I meant CreateCatchContextParameters const& instead of const
CreateCatchContextParameters&, but this file seems indeed to be some kind of an
exception to the coding style :/

[commit-bot: I haz the power, 2016-09-02 09:46:58]

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

[commit-bot: I haz the power, 2016-09-02 09:46:59]

Dry run: This issue passed the CQ dry run.

[marja, 2016-09-02 09:55:47]

ast/ LGTM

[rmcilroy, 2016-09-05 09:56:08]

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

[rmcilroy, 2016-09-05 09:56:08]

interpreter/ LGTM

[Michael Starzinger, 2016-09-05 11:35:29]

LGTM with comments ans nits to address.

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
src/compiler/access-builder.cc:747: FieldAccess
AccessBuilder::ForContextExtensionScopeInfo() {
nit: Likewise, lets move it up to before "elements" to the other "fields".

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
src/compiler/access-builder.cc:750: Type::Any(), MachineType::AnyTagged(),      
    kFullWriteBarrier};
nit: Lets use Type::OtherInternal instead.

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
src/compiler/access-builder.h:214: static FieldAccess
ForContextExtensionScopeInfo();
nit: Please move up to below the other "fields" to make "elements" stay at the
bottom of this file.

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-create-...
File src/compiler/js-create-lowering.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-create-...
src/compiler/js-create-lowering.cc:859: Node* arguments = aa.Finish();
nit: s/arguments/extension/

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:181: // Defines the name and ScopeInfo for a new
catch context.
nit: ... "This is used as a parameter by the JSCreateCatchContext operator."

https://codereview.chromium.org/2302013002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/src/factory.cc#newcode827
src/factory.cc:827: Handle<ContextExtension> extension =
NewContextExtension(scope_info, name);
Allocating before fully initializing the context (i.e. after the set_map but
before the other setters) looks dangerous. Please move this up to before even
allocating the FixedArray (i.e. to before line 824 above).

https://codereview.chromium.org/2302013002/diff/20001/test/unittests/compiler...
File test/unittests/compiler/js-create-lowering-unittest.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/test/unittests/compiler...
test/unittests/compiler/js-create-lowering-unittest.cc:198: Handle<ScopeInfo>
scope_info(
nit: Would factory()->NewScopeInfo(1) also work here?

[jochen (gone - plz use gerrit), 2016-09-05 11:42:45]

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
File src/compiler/access-builder.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
src/compiler/access-builder.cc:747: FieldAccess
AccessBuilder::ForContextExtensionScopeInfo() {
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> nit: Likewise, lets move it up to before "elements" to the other "fields".

done

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
src/compiler/access-builder.cc:750: Type::Any(), MachineType::AnyTagged(),      
    kFullWriteBarrier};
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> nit: Lets use Type::OtherInternal instead.

done

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
File src/compiler/access-builder.h (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/access-bui...
src/compiler/access-builder.h:214: static FieldAccess
ForContextExtensionScopeInfo();
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> nit: Please move up to below the other "fields" to make "elements" stay at the
bottom of this file.

done.

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-create-...
File src/compiler/js-create-lowering.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-create-...
src/compiler/js-create-lowering.cc:859: Node* arguments = aa.Finish();
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> nit: s/arguments/extension/

done

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-operator.h
File src/compiler/js-operator.h (right):

https://codereview.chromium.org/2302013002/diff/20001/src/compiler/js-operato...
src/compiler/js-operator.h:181: // Defines the name and ScopeInfo for a new
catch context.
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> nit: ... "This is used as a parameter by the JSCreateCatchContext operator."

done

https://codereview.chromium.org/2302013002/diff/20001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/src/factory.cc#newcode827
src/factory.cc:827: Handle<ContextExtension> extension =
NewContextExtension(scope_info, name);
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> Allocating before fully initializing the context (i.e. after the set_map but
before the other setters) looks dangerous. Please move this up to before even
allocating the FixedArray (i.e. to before line 824 above).

done

https://codereview.chromium.org/2302013002/diff/20001/test/unittests/compiler...
File test/unittests/compiler/js-create-lowering-unittest.cc (right):

https://codereview.chromium.org/2302013002/diff/20001/test/unittests/compiler...
test/unittests/compiler/js-create-lowering-unittest.cc:198: Handle<ScopeInfo>
scope_info(
On 2016/09/05 at 11:35:29, Michael Starzinger wrote:
> nit: Would factory()->NewScopeInfo(1) also work here?

done

[jochen (gone - plz use gerrit), 2016-09-05 11:42:51]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-09-05 11:42:51]

The patchset sent to the CQ was uploaded after l-g-t-m from neis@chromium.org
Link to the patchset: https://codereview.chromium.org/2302013002/#ps40001
(title: "updates")

[commit-bot: I haz the power, 2016-09-05 11:42:57]

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-09-05 12:10:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-05 12:10:57]

Description was changed from

==========
Store the scope info in catch contexts

Since the extension field is already used for the catch name, store a
ContextExtension there instead.

In the future, this will allow for chaining ScopeInfos together, so we
no longer need a context chain for lazy parsing / compilation.

BUG=v8:5215
R=bmeurer@chromium.org,neis@chromium.org,marja@chromium.org
==========

to

==========
Store the scope info in catch contexts

Since the extension field is already used for the catch name, store a
ContextExtension there instead.

In the future, this will allow for chaining ScopeInfos together, so we
no longer need a context chain for lazy parsing / compilation.

BUG=v8:5215
R=bmeurer@chromium.org,neis@chromium.org,marja@chromium.org

Committed: https://crrev.com/9b6ff3a8f698eeef5f24d5403709ca6fb3251914
Cr-Commit-Position: refs/heads/master@{#39164}
==========

[commit-bot: I haz the power, 2016-09-05 12:10:58]

Patchset 3 (id:??) landed as
https://crrev.com/9b6ff3a8f698eeef5f24d5403709ca6fb3251914
Cr-Commit-Position: refs/heads/master@{#39164}