[interpreter] Implement handling of try-finally constructs.

[interpreter] Implement handling of try-finally constructs.

This models function local control flow through try-finally constructs
using a token dispatch mechanism. All paths through the finally block
are assigned a token, at the end of the finally block a switch construct
dispatches according to this token.

R=oth@chromium.org,rmcilroy@chromium.org
BUG=v8:4674
LOG=n

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

[Michael Starzinger, 2016-01-21 12:40:32]

Note that I still need to adapt the bytecode generator tests to the changes of
how try-finally is being modeled. But otherwise this is ready for a first round
of comments.

[oth, 2016-01-21 14:06:44]

This is great. Looks good to go when the test changes are ready.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:159: SwitchBuilder dispatch(builder(),
static_cast<int>(deferred_.size() + 1));
Comment on the +1.

[Michael Starzinger, 2016-01-21 14:24:53]

Thanks. Addressed comments. Need one more tiny fix in a separate CL, then we are
ready to go with this one. :)

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:159: SwitchBuilder dispatch(builder(),
static_cast<int>(deferred_.size() + 1));
On 2016/01/21 14:06:44, oth wrote:
> Comment on the +1.

Done.

[rmcilroy, 2016-01-21 14:30:49]

Looks great, just nits and bytecode generator tests.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:133: // generates a new dispatch token
that identifies one particular path.
Add a comment that this expects the result to be in the accumulator.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:137:
builder()->StoreAccumulatorInRegister(result_register_);
nit - newline above here

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:143: // the finally-block has been entered
through the exception handler.
Ditto (comment that exception should be in accumulator)

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:167:
dispatch.DefaultAt(static_cast<int>(deferred_.size()));
I'm guessing the default case here only for the fall-through (-1) case? Could
you add a comment for this.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:297: USE(try_catch_builder_);
nit - add a TODO here for now?

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1144: try_control_builder.BeginFinally();
nit - newline before BeginFinally (I think it would be clearer to have the block
of statements be: 

 try_control_builder.BeginFinally() 
 VisitClearMessageObject();
 //...
 Visit(stmt->finally_block());
 try_control_builder.EndFinally();

https://codereview.chromium.org/1613443002/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1613443002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2065: "var a = 1; try { a = 2; throw
23; } finally { throw 42; };",
Could you add some tests for break / continue as well?

[Michael Starzinger, 2016-01-22 10:18:11]

PTAL. Comments addressed. This should be ready to go now.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:133: // generates a new dispatch token
that identifies one particular path.
On 2016/01/21 14:30:49, rmcilroy wrote:
> Add a comment that this expects the result to be in the accumulator.

Done.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:137:
builder()->StoreAccumulatorInRegister(result_register_);
On 2016/01/21 14:30:49, rmcilroy wrote:
> nit - newline above here

Done.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:143: // the finally-block has been entered
through the exception handler.
On 2016/01/21 14:30:49, rmcilroy wrote:
> Ditto (comment that exception should be in accumulator)

Done.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:167:
dispatch.DefaultAt(static_cast<int>(deferred_.size()));
On 2016/01/21 14:30:49, rmcilroy wrote:
> I'm guessing the default case here only for the fall-through (-1) case? Could
> you add a comment for this.

Acknowledged. Comment already added due to Orions feedback.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:297: USE(try_catch_builder_);
On 2016/01/21 14:30:49, rmcilroy wrote:
> nit - add a TODO here for now? 

Done.

https://codereview.chromium.org/1613443002/diff/40001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1144: try_control_builder.BeginFinally();
On 2016/01/21 14:30:49, rmcilroy wrote:
> nit - newline before BeginFinally (I think it would be clearer to have the
block
> of statements be: 
> 
>  try_control_builder.BeginFinally() 
>  VisitClearMessageObject();
>  //...
>  Visit(stmt->finally_block());
>  try_control_builder.EndFinally();

Done. Also note that there is still functionality missing (i.e. message object
needs to be preserved at finally entry and restored and finally exit). So this
will be reshuffled at least one more time before I am done.

https://codereview.chromium.org/1613443002/diff/40001/test/cctest/interpreter...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1613443002/diff/40001/test/cctest/interpreter...
test/cctest/interpreter/test-interpreter.cc:2065: "var a = 1; try { a = 2; throw
23; } finally { throw 42; };",
On 2016/01/21 14:30:49, rmcilroy wrote:
> Could you add some tests for break / continue as well?

Done.

[oth, 2016-01-22 11:42:58]

lgtm, thx!

[rmcilroy, 2016-01-22 11:44:11]

lgtm

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:200: virtual bool Execute(Command command,
Statement* statement) {
/s/virtual/override (I've changed the existing ones in
https://codereview.chromium.org/1618693002/)

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:294: virtual bool Execute(Command command,
Statement* statement) {
ditto

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:326: virtual bool Execute(Command command,
Statement* statement) {
ditto

[Michael Starzinger, 2016-01-22 11:53:54]

Patchset #7 (id:120001) has been deleted

[Michael Starzinger, 2016-01-22 11:55:34]

Thanks. Preparing to land ...

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:200: virtual bool Execute(Command command,
Statement* statement) {
On 2016/01/22 11:44:11, rmcilroy wrote:
> /s/virtual/override (I've changed the existing ones in
> https://codereview.chromium.org/1618693002/)

Done.

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:294: virtual bool Execute(Command command,
Statement* statement) {
On 2016/01/22 11:44:11, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1613443002/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:326: virtual bool Execute(Command command,
Statement* statement) {
On 2016/01/22 11:44:11, rmcilroy wrote:
> ditto

Done.

[Michael Starzinger, 2016-01-22 12:41:48]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2016-01-22 12:41:48]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1613443002/#ps160001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-01-22 12:42:03]

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

[commit-bot: I haz the power, 2016-01-22 12:43:15]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-01-22 12:44:03]

Description was changed from

==========
[interpreter] Implement handling of try-finally constructs.

This models function local control flow through try-finally constructs
using a token dispatch mechanism. All paths through the finally block
are assigned a token, at the end of the finally block a switch construct
dispatches according to this token.

R=oth@chromium.org,rmcilroy@chromium.org
BUG=v8:4674
LOG=n
==========

to

==========
[interpreter] Implement handling of try-finally constructs.

This models function local control flow through try-finally constructs
using a token dispatch mechanism. All paths through the finally block
are assigned a token, at the end of the finally block a switch construct
dispatches according to this token.

R=oth@chromium.org,rmcilroy@chromium.org
BUG=v8:4674
LOG=n

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

[commit-bot: I haz the power, 2016-01-22 12:44:04]

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