Fix stepping over await statements

Fix stepping over await statements

When async-stepping over an await statement that follows a previous
async-step command, ensure that the synthetic breakpoint at the
beginning of the async closure is not accidentally deleted.

Removing a test that depends on minute details of the internal
implementation of async-step commands.

Adding new test that checks for multiple consecutive async-step
commands.

Note that debugger.cc seems to contain a memory leak. Breakpoint
objects are not deleted (deallocated) when RemoveBreakpoint()
is called. Will address this in a separate CL.

BUG=#27238
R=johnmccutchan@google.com

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

[hausner, 2016-09-21 23:31:30]

Description was changed from

==========
Fix stepping over await statements

When async-stepping over an await statement that follows a previous
async-step command, ensure that the synthetic breakpoint at the
beginning of the async closure is not accidentally deleted.

Removing a test that depends on minute details of the internal
implementation of async-step commands.

Adding new test that checks for multiple consecutive async-step
commands.

Note that debugger.cc seems to contain a memory leak. Breakpoint
objects are not deleted (deallocated) when RemoveBreakpoint()
is called. Will address this in a separate CL.

BUG=#27238
==========

to

==========
Fix stepping over await statements

When async-stepping over an await statement that follows a previous
async-step command, ensure that the synthetic breakpoint at the
beginning of the async closure is not accidentally deleted.

Removing a test that depends on minute details of the internal
implementation of async-step commands.

Adding new test that checks for multiple consecutive async-step
commands.

Note that debugger.cc seems to contain a memory leak. Breakpoint
objects are not deleted (deallocated) when RemoveBreakpoint()
is called. Will address this in a separate CL.

BUG=#27238
==========

[hausner, 2016-09-21 23:31:30]

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

[Cutch, 2016-09-21 23:43:46]

I'm wondering if we can preserve the old breakpoint behaviour while still fixing
this bug. See my comments.

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

https://codereview.chromium.org/2366463002/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:382: // If the debugger is paused after stepping over an
await statement,
I wonder if we could just do the following in this block:

if (for_over_await ....) {
  RemoveBreakpoint(bpt->id());
  remove_synthetic_async_breakpoint = false;   /* this controls whether or not
synthetic_async_breakpoint gets removed after the stepping */
  bpt = NULL;
}

...

This way we still create a new synthetic breakpoint.

https://codereview.chromium.org/2366463002/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2727: // that we are paused at that breakpoint and then
delete it after continuing.
remove_synthetic_async_breakpoint = true;

https://codereview.chromium.org/2366463002/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2729: if (synthetic_async_breakpoint_ != NULL) {
if (remove_synthetic_async_breakpoint && (synthetic_async_breakpoint_ != NULL))
{
}

[hausner, 2016-09-21 23:59:29]

See comment/question

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

https://codereview.chromium.org/2366463002/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:382: // If the debugger is paused after stepping over an
await statement,
On 2016/09/21 23:43:45, Cutch wrote:
> I wonder if we could just do the following in this block:
> 
> if (for_over_await ....) {
>   RemoveBreakpoint(bpt->id());
>   remove_synthetic_async_breakpoint = false;   /* this controls whether or not
> synthetic_async_breakpoint gets removed after the stepping */
>   bpt = NULL;
> }
> 
> ...
> 
> This way we still create a new synthetic breakpoint.

Why is it important to create a new synthetic BP each time? Also, I think 
synthetic_async_breakpoint_ should be set to NULL if the breakpoint is refers to
is deleted.

[Cutch, 2016-09-22 15:49:07]

On 2016/09/21 23:59:29, hausner wrote:
> See comment/question
> 
> https://codereview.chromium.org/2366463002/diff/20001/runtime/vm/debugger.cc
> File runtime/vm/debugger.cc (right):
> 
>
https://codereview.chromium.org/2366463002/diff/20001/runtime/vm/debugger.cc#...
> runtime/vm/debugger.cc:382: // If the debugger is paused after stepping over
an
> await statement,
> On 2016/09/21 23:43:45, Cutch wrote:
> > I wonder if we could just do the following in this block:
> > 
> > if (for_over_await ....) {
> >   RemoveBreakpoint(bpt->id());
> >   remove_synthetic_async_breakpoint = false;   /* this controls whether or
not
> > synthetic_async_breakpoint gets removed after the stepping */
> >   bpt = NULL;
> > }
> > 
> > ...
> > 
> > This way we still create a new synthetic breakpoint.
> 
> Why is it important to create a new synthetic BP each time? Also, I think 
> synthetic_async_breakpoint_ should be set to NULL if the breakpoint is refers
to
> is deleted.

If there are multiple step-over-awaits in flight the BP id can be used to
differentiate them.

[hausner, 2016-09-22 15:54:55]

Maybe we should discuss in person tomorrow.

I don't see how there can be multiple async step-over in flight in the same
closure. (The breakpoint is unique per closure.)

[hausner, 2016-09-22 18:32:43]

Here is a fix that creates a new distinctive breakpoint (with new id) for each
async step-over, deleting the previous breakpoint after the first async-step
resumes.

[Cutch, 2016-09-22 18:36:12]

On 2016/09/22 18:32:43, hausner wrote:
> Here is a fix that creates a new distinctive breakpoint (with new id) for each
> async step-over, deleting the previous breakpoint after the first async-step
> resumes.

Thanks! With this change does the old step_over_await test start working again?

[Cutch, 2016-09-22 18:40:43]

I was thinking about your comment about how there couldn't be multiple
breakpoints for the same closure and I think you're right. But then I started to
wonder about recursive calls into an async function and how the debugger might
get confused if we stepped over different await statements in different
activations of the same function...

[hausner, 2016-09-22 20:03:00]

Yes, the old step_over_await_test runs with this version of the change. However,
I think that test is too brittle, and partially wrong. It only passes because
LINE_C and LINE_D are set to off-by-one line numbers. I would not keep that test
as-is.

[Cutch, 2016-09-22 20:28:05]

lgtm

[hausner, 2016-09-22 21:01:24]

Thank you.

[hausner, 2016-09-22 21:06:27]

Description was changed from

==========
Fix stepping over await statements

When async-stepping over an await statement that follows a previous
async-step command, ensure that the synthetic breakpoint at the
beginning of the async closure is not accidentally deleted.

Removing a test that depends on minute details of the internal
implementation of async-step commands.

Adding new test that checks for multiple consecutive async-step
commands.

Note that debugger.cc seems to contain a memory leak. Breakpoint
objects are not deleted (deallocated) when RemoveBreakpoint()
is called. Will address this in a separate CL.

BUG=#27238
==========

to

==========
Fix stepping over await statements

When async-stepping over an await statement that follows a previous
async-step command, ensure that the synthetic breakpoint at the
beginning of the async closure is not accidentally deleted.

Removing a test that depends on minute details of the internal
implementation of async-step commands.

Adding new test that checks for multiple consecutive async-step
commands.

Note that debugger.cc seems to contain a memory leak. Breakpoint
objects are not deleted (deallocated) when RemoveBreakpoint()
is called. Will address this in a separate CL.

BUG=#27238
R=johnmccutchan@google.com

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

[hausner, 2016-09-22 21:06:29]

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