Implement deletion of breakpoints

runtime/vm/debugger.cc

Index: runtime/vm/debugger.cc
===================================================================
--- runtime/vm/debugger.cc	(revision 3444)
+++ runtime/vm/debugger.cc	(working copy)
@@ -25,6 +25,7 @@
     : function_(func.raw()),
       pc_desc_index_(pc_desc_index),
       pc_(0),
+      saved_bytes_(0),
       line_number_(-1),
       next_(NULL) {
   Code& code = Code::Handle(func.code());
@@ -230,6 +231,21 @@
 }
 
 
+Debugger::~Debugger() {
+  ASSERT(breakpoints_ == NULL);
+}
+
+
+void Debugger::Shutdown() {
+  while (breakpoints_ != NULL) {
+    Breakpoint* bpt = breakpoints_;
+    breakpoints_ = breakpoints_->next();
+    UnsetBreakpoint(bpt);
+    delete bpt;
+  }
+}
+
+
 bool Debugger::IsActive() {
   // TODO(hausner): The code generator uses this function to prevent
   // generation of optimized code when Dart code is being debugged.
@@ -297,14 +313,32 @@
     PcDescriptors::Kind kind = desc.DescriptorKind(i);
     Breakpoint* bpt = NULL;
     if (kind == PcDescriptors::kIcCall) {
+      bpt = GetBreakpoint(desc.PC(i));
+      if (bpt != NULL) {
+        // There is an existing breakpoint at this token position.
+        break;
+      }
+      bpt = new Breakpoint(target_function, i);
+      RegisterBreakpoint(bpt);

[siva, 2012/01/20 21:26:29]

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.

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

Done.

+      String& func_name = String::Handle();
+      int num_args, num_named_args;
+      CodePatcher::GetInstanceCallAt(desc.PC(i),
+          &func_name, &num_args, &num_named_args, &bpt->saved_bytes_);
       CodePatcher::PatchInstanceCallAt(
           desc.PC(i), StubCode::BreakpointDynamicEntryPoint());
-      bpt = new Breakpoint(target_function, i);
     } else if (kind == PcDescriptors::kOther) {
       if ((desc.TokenIndex(i) > 0) && CodePatcher::IsDartCall(desc.PC(i))) {
+        bpt = GetBreakpoint(desc.PC(i));
+        if (bpt != NULL) {
+          // There is an existing breakpoint at this token position.
+          break;
+        }
+        bpt = new Breakpoint(target_function, i);
+        RegisterBreakpoint(bpt);
+        Function& func = Function::Handle();
+        CodePatcher::GetStaticCallAt(desc.PC(i), &func, &bpt->saved_bytes_);
         CodePatcher::PatchStaticCallAt(
           desc.PC(i), StubCode::BreakpointStaticEntryPoint());
-        bpt = new Breakpoint(target_function, i);
       }
     }
     if (bpt != NULL) {
@@ -314,7 +348,6 @@
                   bpt->LineNumber(),
                   bpt->pc());

[siva, 2012/01/20 21:26:29]

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).

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

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

       }
-      AddBreakpoint(bpt);
       return bpt;
     }
   }
@@ -322,6 +355,22 @@
 }
 
 
+void Debugger::UnsetBreakpoint(Breakpoint* bpt) {
+  const Function& func = Function::Handle(bpt->function());
+  const Code& code = Code::Handle(func.code());
+  PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
+  intptr_t desc_index = bpt->pc_desc_index();
+  ASSERT(desc_index < desc.Length());
+  ASSERT(bpt->pc() == desc.PC(desc_index));
+  PcDescriptors::Kind kind = desc.DescriptorKind(desc_index);
+  if (kind == PcDescriptors::kIcCall) {
+    CodePatcher::PatchInstanceCallAt(desc.PC(desc_index), bpt->saved_bytes_);
+  } else {
+    CodePatcher::PatchStaticCallAt(desc.PC(desc_index), bpt->saved_bytes_);
+  }
+}
+
+
 Breakpoint* Debugger::SetBreakpointAtEntry(const Function& target_function) {
   ASSERT(!target_function.IsNull());
   return SetBreakpoint(target_function, target_function.token_index());
@@ -445,7 +494,63 @@
 }
 
 
-void Debugger::AddBreakpoint(Breakpoint* bpt) {
+bool Debugger::IsRegisteredBreakpoint(Breakpoint* bpt) {

[siva, 2012/01/20 21:26:29]

Where is this function used?

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

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.

+  Breakpoint* known_bpt = this->breakpoints_;
+  while (known_bpt != NULL) {
+    if (bpt == known_bpt) {
+      return true;
+    }
+    known_bpt = known_bpt->next();
+  }
+  return false;
+}
+
+
+void Debugger::RemoveBreakpoint(Breakpoint* bpt) {
+  ASSERT(breakpoints_ != NULL);
+  if (breakpoints_ == NULL) {
+    return;
+  }

[siva, 2012/01/20 21:26:29]

There is an assertion that breakpoints_ is not NULL and a check that if
breakpoints == NULL return. Is the assertion needed?

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

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.

+  if (bpt == breakpoints_) {
+    breakpoints_ = breakpoints_->next();
+    UnsetBreakpoint(bpt);
+    delete bpt;
+    return;
+  }
+  // There is at least one breakpoint in the list, and the one we're
+  // looking for is not at the head of the list.
+  Breakpoint* prev_bpt = this->breakpoints_;
+  Breakpoint* curr_bpt = prev_bpt->next();
+  while (curr_bpt != NULL) {
+    if (bpt == curr_bpt) {
+      // Unlink curr_bpt from list and delete the breakpoint.

[siva, 2012/01/20 21:26:29]

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();
    }
    ...
    ...
  }
  ...
  ...
}

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

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

+      prev_bpt->set_next(curr_bpt->next());
+      UnsetBreakpoint(bpt);
+      delete bpt;
+      return;
+    }
+    prev_bpt = curr_bpt;
+    curr_bpt = curr_bpt->next();
+  }
+  // bpt is not a registered breakpoint, nothing to do.
+}
+
+
+Breakpoint* Debugger::GetBreakpointByFunction(const Function& func,
+                                              intptr_t token_index) {
+  Breakpoint* bpt = this->breakpoints_;
+  while (bpt != NULL) {
+    if ((bpt->function() == func.raw()) &&
+        (bpt->token_index() == token_index)) {
+      return bpt;
+    }
+    bpt = bpt->next();
+  }
+  return NULL;
+}
+
+
+void Debugger::RegisterBreakpoint(Breakpoint* bpt) {
   ASSERT(bpt->next() == NULL);
   bpt->set_next(this->breakpoints_);
   this->breakpoints_ = bpt;