Fix another missing source location VM stacktrace with async code.

Fix another missing source location VM stacktrace with async code.

The transformed AST for await now uses the token position from the original await node.

BUG=dartbug.com/22009
R=hausner@google.com, iposva@google.com

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

[Florian Schneider, 2015-02-12 11:20:09]

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

[hausner, 2015-02-12 17:18:44]

This may fix the stack trace issue, but changes (and most likely breaks) the
debugger experience. All the nodes that now have a source position are now safe
points for the debugger, i.e. they are candidates to set breakpoints and if the
user single-steps, the debugger stops at most of these nodes now. A mild side
effect would be that it may take 10 or so "step" commands to get over an await.
An uglier side effect is that the user may no longer be able to stop at a
location that corresponds to user-visible code, see comment below.

As we create more and more syntactic sugar that gets translated to invisible
dart code, this issue becomes a bigger problem. What we really need is a way to
separate token position from whether or not the debugger should stop there. In
the meantime, can't we live wit stack traces that have no line info in some
frames?

I'm not comfortable giving an LGTM at this point because of the unintended
fallouts I'd expect in the debugging experience. Ivan can override this if he
thinks it's the right thing.

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

https://codereview.chromium.org/922603002/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:153: new (Z) SequenceNode(node->token_pos(),
is_not_future_scope);
The reason why all these nodes that have no equivalent source code position are
marked with kNoSourcePos is so that the debugger does not set breakpoints at
meaningless locations, and does not stop there when stepping.

https://codereview.chromium.org/922603002/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:249: node->token_pos(), result_param));
This is a good example of code that will most likely have a very low compiled
code address. When the user attempts to set a breakpoint in the line that
contains node->token_pos(), this may be the place the debugger stops instead of
stopping at a place that reflects user-visible code.

[nweiz, 2015-02-12 18:43:51]

DBC

On 2015/02/12 17:18:44, hausner wrote:
> As we create more and more syntactic sugar that gets translated to invisible
> dart code, this issue becomes a bigger problem. What we really need is a way
to
> separate token position from whether or not the debugger should stop there. In
> the meantime, can't we live wit stack traces that have no line info in some
> frames?

Looking more at Kevin's example, it seems like traces without position
information are always in a stack chain where there's position information
elsewhere. In fact, since they're representing "invisible" stack frames anyway,
they're not particularly useful to have in the stack chain at all, so I'll
probably just have the stack_trace package filter them out by default.

tl;dr: Yes, we can live with these for the time being. Also, when they are
fixed, having some sort of visible marker for "non-debuggable frame" would also
be helpful, so the stack_trace package can know to omit those if necessary.

[Florian Schneider, 2015-02-13 10:21:06]

Doesn't the debugger step forward until reaching the next token position anyway?

On the other hand, how does setting a breakpoint at an await expression work
now, if the node's token postion is never recorde?

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

https://codereview.chromium.org/922603002/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:249: node->token_pos(), result_param));
On 2015/02/12 17:18:44, hausner wrote:
> This is a good example of code that will most likely have a very low compiled
> code address. When the user attempts to set a breakpoint in the line that
> contains node->token_pos(), this may be the place the debugger stops instead
of
> stopping at a place that reflects user-visible code.

I can only assign the tokenposition to the call instructions here if that helps.
Unfortunately we don't have any tests that would catch any regression in
debugger experience for async code.

[Florian Schneider, 2015-02-13 10:24:43]

New patchsets have been uploaded after l-g-t-m from hausner@google.com

[Florian Schneider, 2015-02-13 10:25:28]

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

https://codereview.chromium.org/922603002/diff/1/runtime/vm/ast_transformer.c...
runtime/vm/ast_transformer.cc:189: new(Z) InstanceCallNode(node->token_pos(),
To fix the bug it would be enough to record it here, and I _think_ that the
await node's token position should be recorded somewhere anyway.

[Florian Schneider, 2015-02-13 10:33:38]

On 2015/02/12 18:43:51, nweiz wrote:
> DBC
> 
> On 2015/02/12 17:18:44, hausner wrote:
> > As we create more and more syntactic sugar that gets translated to invisible
> > dart code, this issue becomes a bigger problem. What we really need is a way
> to
> > separate token position from whether or not the debugger should stop there.
In
> > the meantime, can't we live wit stack traces that have no line info in some
> > frames?
> 
> Looking more at Kevin's example, it seems like traces without position
> information are always in a stack chain where there's position information
> elsewhere. In fact, since they're representing "invisible" stack frames
anyway,
> they're not particularly useful to have in the stack chain at all, so I'll
> probably just have the stack_trace package filter them out by default.
> 
> tl;dr: Yes, we can live with these for the time being. Also, when they are
> fixed, having some sort of visible marker for "non-debuggable frame" would
also
> be helpful, so the stack_trace package can know to omit those if necessary.

@nweiz: I noticed that as well. The position was only missing at the implicit
Future.then invociaton stemming from the await expression. The implicit
completeError invocation has a position at the end of the async-expression
assigned.

[Ivan Posva, 2015-02-18 07:43:12]

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

[Ivan Posva, 2015-02-18 07:43:12]

LGTM with only calls directly associated with the await expression having a
valid token index.

-Ivan

[Florian Schneider, 2015-02-18 14:48:50]

On 2015/02/18 07:43:12, Ivan Posva wrote:
> LGTM with only calls directly associated with the await expression having a
> valid token index.
> 
> -Ivan

Thanks. I'll land the smallest possible fix for the issue (see patch #2).

[Florian Schneider, 2015-02-18 14:50:42]

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