Fix VM bug with try-catch inside of try-finally.

Fix VM bug with try-catch inside of try-finally.

There were two exception handlers with the same index added to the code so that only the one added
last was executed. In case of an exception that means that a re-throw may be omitted, causing invalid
control flow.

Also, add assertion to ensure unique try-index for exception handlers.

BUG=#25333
R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/e9968ca5ff695d4b88e924ddac3302c8ddbe6746

[Florian Schneider, 2016-01-06 16:21:51]

fschneider@google.com changed reviewers:
+ hausner@google.com

[Ivan Posva, 2016-01-06 16:35:47]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2016-01-06 16:35:47]

DBC -ip

https://codereview.chromium.org/1569523002/diff/20001/tests/language/try_fina...
File tests/language/try_finally_regress_25333_test.dart (right):

https://codereview.chromium.org/1569523002/diff/20001/tests/language/try_fina...
tests/language/try_finally_regress_25333_test.dart:18: }
Please add some state verifying that the catch clause was actually executed and
not skipped entirely.

https://codereview.chromium.org/1569523002/diff/20001/tests/language/try_fina...
tests/language/try_finally_regress_25333_test.dart:27: }
ditto.

[Florian Schneider, 2016-01-06 16:40:07]

https://codereview.chromium.org/1569523002/diff/20001/tests/language/try_fina...
File tests/language/try_finally_regress_25333_test.dart (right):

https://codereview.chromium.org/1569523002/diff/20001/tests/language/try_fina...
tests/language/try_finally_regress_25333_test.dart:18: }
On 2016/01/06 16:35:47, Ivan Posva wrote:
> Please add some state verifying that the catch clause was actually executed
and
> not skipped entirely.

Done.

https://codereview.chromium.org/1569523002/diff/20001/tests/language/try_fina...
tests/language/try_finally_regress_25333_test.dart:27: }
On 2016/01/06 16:35:47, Ivan Posva wrote:
> ditto.

Done.

[hausner, 2016-01-06 17:11:59]

LGTM but I don't understand why the new rethrow_clause ast is necessary. It
seems to be equivalent to the finally clause. Can you explain that (in a code
comment)?

Thank you for fixing this.

https://codereview.chromium.org/1569523002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1569523002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9804: rethrow_clause = EnsureFinallyClause(
Does this call to EnsureFinallyClause() return a different result than the the
one we already have computed above? It looks to me that finally_clause and
rethrow_clause will contain the same AST.

If that is true, then all TryCatchNode nodes contain the same value for the
finally and rethrow clause. Why do you need to have a separate rethrow_clause
ast?

[Florian Schneider, 2016-01-06 17:37:26]

I'll add the comment to the code as well.

https://codereview.chromium.org/1569523002/diff/20001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1569523002/diff/20001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:9804: rethrow_clause = EnsureFinallyClause(
On 2016/01/06 17:11:59, hausner wrote:
> Does this call to EnsureFinallyClause() return a different result than the the
> one we already have computed above? It looks to me that finally_clause and
> rethrow_clause will contain the same AST.
> 
> If that is true, then all TryCatchNode nodes contain the same value for the
> finally and rethrow clause. Why do you need to have a separate rethrow_clause
> ast?

The AST will be the same (like for inlined-finally-nodes), but
it will not share the same try-indices - i.e. AllocateTryIndex will return a
fresh one if the finally clause contains a try-catch (as in the test).

[Florian Schneider, 2016-01-06 17:42:00]

I should add: The flowgraph builder emits two copies of a finally-block if the
try-block has a normal exit (i.e. does not end in throw/return etc. One for the
exception-case, one for the non-exception case.

[hausner, 2016-01-06 17:44:32]

Thank you for the explanation. It would be good to have it in the code as a
comment.

[Florian Schneider, 2016-01-06 17:50:58]

Description was changed from

==========
Fix VM bug with try-catch inside of try-finally.

There were two exception handlers with the same index added to the code so that
only the one added
last was executed. In case of an exception that means that a re-throw may be
omitted, causing invalid
control flow.

Also, add assertion to ensure unique try-index for exception handlers.

BUG=#25333
==========

to

==========
Fix VM bug with try-catch inside of try-finally.

There were two exception handlers with the same index added to the code so that
only the one added
last was executed. In case of an exception that means that a re-throw may be
omitted, causing invalid
control flow.

Also, add assertion to ensure unique try-index for exception handlers.

BUG=#25333
R=hausner@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e9968ca5ff695d4b88e924ddac3302c8ddbe6746
==========

[Florian Schneider, 2016-01-06 17:50:59]

Committed patchset #4 (id:60001) manually as
e9968ca5ff695d4b88e924ddac3302c8ddbe6746 (presubmit successful).