Save exception and stack trace variables in async catch clauses and use them

Save exception and stack trace variables in async catch clauses and use them
in rethrow statements (fix issue 22523).

R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44001

[regis, 2015-02-24 19:17:53]

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

[Ivan Posva, 2015-02-24 20:52:38]

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

[Ivan Posva, 2015-02-24 20:52:39]

DBC -ip

https://codereview.chromium.org/956583003/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/956583003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:4063:
ASSERT(!catch_block->stacktrace_var().is_captured());
Why is the fact that ':saved_try_context_var' cannot be captures not asserted
here as well?

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

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8222: void Parser::AddSavedExceptionAndStacktraceToScope(
These variables are being added unconditionally here. You should assert that we
are in a async/sync*/async* body closure.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8232: saved_exception_var->set_is_captured();
These variables should be automatically captured as is the rest of the
variables. If not we need to know why they are not being captured by the rest of
the system.
These variables also should be marked as final.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8999: excp_var =
scope->LookupVariable(Symbols::SavedExceptionVar(), false);
Why is this not LocalLookupVariable?

[hausner, 2015-02-24 20:57:32]

LGTM w/comments.

Does this fix any of the crasher bugs where we try to write to a null object?

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

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:5982: ASSERT(exception_var != NULL);
Maybe it makes sense to pull this assert out of the if block, now that we use
the exception_var outside of the if.

Ditto for ASSERT(stack_trace_var != NULL) below.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8200: ASSERT(added_to_scope);  // TODO(regis): Why do we
check below but not here?
FWIW, I don't know either.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8232: saved_exception_var->set_is_captured();
Capturing explicitly should not be necessary. After the function is parsed,
there is a recursive pass over all scopes to capture all variables. I think that
would also capture these two variables.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8999: excp_var =
scope->LookupVariable(Symbols::SavedExceptionVar(), false);
On 2015/02/24 20:52:39, Ivan Posva wrote:
> Why is this not LocalLookupVariable?

I think Ivan is right, this should only look up in the innermost scope.
Otherwise you might find the saved exception var of an outer try-catch. Compare
to the else branch below.

[regis, 2015-02-24 22:05:13]

Thanks!

https://codereview.chromium.org/956583003/diff/1/runtime/vm/flow_graph_builde...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/956583003/diff/1/runtime/vm/flow_graph_builde...
runtime/vm/flow_graph_builder.cc:4063:
ASSERT(!catch_block->stacktrace_var().is_captured());
On 2015/02/24 20:52:39, Ivan Posva wrote:
> Why is the fact that ':saved_try_context_var' cannot be captures not asserted
> here as well?

No idea.
Assert added.

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

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:5982: ASSERT(exception_var != NULL);
On 2015/02/24 20:57:31, hausner wrote:
> Maybe it makes sense to pull this assert out of the if block, now that we use
> the exception_var outside of the if.
> 
> Ditto for ASSERT(stack_trace_var != NULL) below.

Done. Note that it was asserted in the constructor of CatchClauseNode below.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8200: ASSERT(added_to_scope);  // TODO(regis): Why do we
check below but not here?
On 2015/02/24 20:57:31, hausner wrote:
> FWIW, I don't know either.

I get it now.
The error can only be reported below when the name of the exception param is
reused for the stack trace param.
I have added a comment.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8222: void Parser::AddSavedExceptionAndStacktraceToScope(
On 2015/02/24 20:52:39, Ivan Posva wrote:
> These variables are being added unconditionally here. You should assert that
we
> are in a async/sync*/async* body closure.

Done.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8232: saved_exception_var->set_is_captured();
On 2015/02/24 20:52:39, Ivan Posva wrote:
> These variables should be automatically captured as is the rest of the
> variables. If not we need to know why they are not being captured by the rest
of
> the system.
> These variables also should be marked as final.

Removed the explicit capturing and marked as final.

https://codereview.chromium.org/956583003/diff/1/runtime/vm/parser.cc#newcode...
runtime/vm/parser.cc:8999: excp_var =
scope->LookupVariable(Symbols::SavedExceptionVar(), false);
On 2015/02/24 20:57:31, hausner wrote:
> On 2015/02/24 20:52:39, Ivan Posva wrote:
> > Why is this not LocalLookupVariable?
> 
> I think Ivan is right, this should only look up in the innermost scope.
> Otherwise you might find the saved exception var of an outer try-catch.
Compare
> to the else branch below.

As explained by the comments (obviously not clearly enough), the saved exception
var and saved stack trace var are looked for in the catch scope, not in the try
scope as for the vars of the else part. The try scope is available, but not the
catch scope. It's why I have to start from the current scope. There is no danger
to find these vars in an outer scope, since they are declared unconditionally.
I have updated the comments.

[regis, 2015-02-24 22:05:42]

Committed patchset #2 (id:20001) manually as r44001 (presubmit successful).