Implement deletion of breakpoints

runtime/vm/debugger.cc

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/debugger.h"
 
 #include "vm/code_index_table.h"
 #include "vm/code_patcher.h"
 #include "vm/compiler.h"
 #include "vm/flags.h"
 #include "vm/globals.h"
 #include "vm/object.h"
 #include "vm/object_store.h"
 #include "vm/os.h"
 #include "vm/stack_frame.h"
 #include "vm/stub_code.h"
 #include "vm/visitor.h"
 
 
 namespace dart {
 
 static const bool verbose = false;
 
 Breakpoint::Breakpoint(const Function& func, intptr_t pc_desc_index)
     : 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());
   ASSERT(!code.IsNull());  // Function must be compiled.
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   ASSERT(pc_desc_index < desc.Length());
   this->token_index_ = desc.TokenIndex(pc_desc_index);
   ASSERT(this->token_index_ > 0);
   this->pc_ = desc.PC(pc_desc_index);
   ASSERT(this->pc_ != 0);
@@ skipping 185 matching lines @@
 }
 
 
 Debugger::Debugger()
     : initialized_(false),
       bp_handler_(NULL),
       breakpoints_(NULL) {
 }
 
 
+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.
   // This is probably not conservative enough (we could set the first
   // breakpoint after optimized code has already been produced).
   // Long-term, we need to be able to de-optimize code.
   return breakpoints_ != NULL;
 }
 
 
@@ skipping 47 matching lines @@
   Code& code = Code::Handle(target_function.code());
   ASSERT(!code.IsNull());
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   for (int i = 0; i < desc.Length(); i++) {
     if (desc.TokenIndex(i) < token_index) {
       continue;
     }
     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) {
       if (verbose) {
         OS::Print("Setting breakpoint at '%s' line %d  (PC %p)\n",
                   String::Handle(bpt->SourceUrl()).ToCString(),
                   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;
     }
   }
   return NULL;
 }
 
 
+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());
 }
 
 
 Breakpoint* Debugger::SetBreakpointAtLine(const String& script_url,
                                           intptr_t line_number) {
   Library& lib = Library::Handle();
   Script& script = Script::Handle();
@@ skipping 103 matching lines @@
   while (bpt != NULL) {
     if (bpt->pc() == breakpoint_address) {
       return bpt;
     }
     bpt = bpt->next();
   }
   return NULL;
 }
 
 
-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;
 }
 
 
 }  // namespace dart