Implement deletion of breakpoints

Implement deletion of breakpoints

- deletion of breakpoints
- better cleanup of debugger resources
Committed: https://code.google.com/p/dart/source/detail?r=3470

[siva, 2012-01-20 21:26:28]

lgtm

http://codereview.chromium.org/9271008/diff/4002/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:66: 
Do we also need a Dart_DeleteAllBreakpoints function?

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:322: RegisterBreakpoint(bpt);
Should the register happen after the code patching?

Currently the code patching function does not return errors but if we decide
that it needs to return errors later, we would have an inconsistent state.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:349: bpt->pc());
Both newly set breakpoints and existing breakpoints will be reported the same
way, do you think there is value in differentiating them?

It will be useful when you implement single stepping (setting temporary
breakpoints for single stepping and a real breakpoint already exists at that
location).

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:497: bool Debugger::IsRegisteredBreakpoint(Breakpoint*
bpt) {
Where is this function used?

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:513: }
There is an assertion that breakpoints_ is not NULL and a check that if
breakpoints == NULL return. Is the assertion needed?

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:526: // Unlink curr_bpt from list and delete the
breakpoint.
Can we roll the two delete points you have into the loop?

Breakpoint* prev_bpt = NULL;
Breakpoint* curr_bpt = breakpoints_;
while (curr_bpt != NULL) {
  if (bpt == curr_bpt) {
    if (prev_bpt == NULL) {
      breakpoints_ = curr_bpt->next();
    } else {
      prev_bpt->set_next(curr_bpt->next();
    }
    ...
    ...
  }
  ...
  ...
}

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger_api_impl...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:90: breakpoint_hit = true;
Is this setting needed?

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:145: // breakpoint is removed by the
handler, so we excpect the breakpoint
expect

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:268: // requires a handle zone. We must set up a temporary
zone because
The comment seems hard to read, is "We shutting down the debugger requires a
handle zone" required?

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:270: if (true) {
Why not just
{
  Zone zone(this);
  ...
  ...
}

[hausner, 2012-01-20 22:20:32]

Thank you for the quick review.

http://codereview.chromium.org/9271008/diff/4002/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:66: 
On 2012/01/20 21:26:29, asiva wrote:
> Do we also need a Dart_DeleteAllBreakpoints function?

I don't have a good understanding of what we really need. It's easy to do, but I
try to keep the API small until I have a better understanding. I'll put that
idea on the list of potential API calls and revisit with Anton and Pavel.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:322: RegisterBreakpoint(bpt);
On 2012/01/20 21:26:29, asiva wrote:
> Should the register happen after the code patching?
> 
> Currently the code patching function does not return errors but if we decide
> that it needs to return errors later, we would have an inconsistent state.

Done.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:349: bpt->pc());
On 2012/01/20 21:26:29, asiva wrote:
> Both newly set breakpoints and existing breakpoints will be reported the same
> way, do you think there is value in differentiating them?
> 
> It will be useful when you implement single stepping (setting temporary
> breakpoints for single stepping and a real breakpoint already exists at that
> location).

Added a todo to determine later. The idea sounds right.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:497: bool Debugger::IsRegisteredBreakpoint(Breakpoint*
bpt) {
On 2012/01/20 21:26:29, asiva wrote:
> Where is this function used?

Not used at the moment. At first I wanted to make it an error if an API function
is called on a breakpoint that is not registered, but then decided to just make
that a no-op.

Deleted the function.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:513: }
On 2012/01/20 21:26:29, asiva wrote:
> There is an assertion that breakpoints_ is not NULL and a check that if
> breakpoints == NULL return. Is the assertion needed?

Similar to the above comment: Most likely it is an error if anyone tries to
remove a breakpoint when none is set. I thought it might be good to catch this
in a debug build, but leave it as a no-op in release.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:526: // Unlink curr_bpt from list and delete the
breakpoint.
On 2012/01/20 21:26:29, asiva wrote:
> Can we roll the two delete points you have into the loop?
> 
> Breakpoint* prev_bpt = NULL;
> Breakpoint* curr_bpt = breakpoints_;
> while (curr_bpt != NULL) {
>   if (bpt == curr_bpt) {
>     if (prev_bpt == NULL) {
>       breakpoints_ = curr_bpt->next();
>     } else {
>       prev_bpt->set_next(curr_bpt->next();
>     }
>     ...
>     ...
>   }
>   ...
>   ...
> }

Ok, done. I thought the code would be easier to understand if the two cases are
separated.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger_api_impl...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:90: breakpoint_hit = true;
On 2012/01/20 21:26:29, asiva wrote:
> Is this setting needed?
No, not really. Removed.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:145: // breakpoint is removed by the
handler, so we excpect the breakpoint
On 2012/01/20 21:26:29, asiva wrote:
> expect

Done.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:268: // requires a handle zone. We must set up a temporary
zone because
On 2012/01/20 21:26:29, asiva wrote:
> The comment seems hard to read, is "We shutting down the debugger requires a
> handle zone" required?
Leftover junk from previous version of comment. Fixed.

http://codereview.chromium.org/9271008/diff/4002/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:270: if (true) {
On 2012/01/20 21:26:29, asiva wrote:
> Why not just
> {
>   Zone zone(this);
>   ...
>   ...
> }

Done.