Debugger step 2

runtime/vm/debugger.cc

Index: runtime/vm/debugger.cc
===================================================================
--- runtime/vm/debugger.cc	(revision 1987)
+++ runtime/vm/debugger.cc	(working copy)
@@ -4,6 +4,7 @@
 
 #include "vm/debugger.h"
 
+#include "vm/code_index_table.h"
 #include "vm/code_patcher.h"
 #include "vm/compiler.h"
 #include "vm/flags.h"
@@ -11,6 +12,7 @@
 #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"
 
@@ -21,36 +23,163 @@
 DEFINE_FLAG(charp, bpt, NULL, "Debug breakpoint at <func>");
 
 
-class Breakpoint {
- public:
-  Breakpoint(const Function& func, intptr_t pc_desc_index, uword pc)
-      : function_(func.raw()),
-        pc_desc_index_(pc_desc_index),
-        pc_(pc),
-        next_(NULL) {
+
+Breakpoint::Breakpoint(const Function& func, intptr_t pc_desc_index)
+    : function_(func.raw()),
+      pc_desc_index_(pc_desc_index),
+      token_index_(0),
+      pc_(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->token_index_ != 0);

[siva, 2011/12/05 19:21:44]

Storing the pc_ and token_index_ in this structure seems to un-normalize the
data somewhat. Why is it not sufficient to just store the func object and
pc_desc_index? Are you worried that the cost of accessing the pc_ and
token_index_ using the pc_descriptors array would be very high?

[hausner, 2011/12/05 21:11:23]

I tried to cache some data so it would not have to be computed each time. (Even
just getting the token index requires allocating handles.) Also, I have to get
the code and pc descriptors anyway to get the PC. Why duplicate that code
pattern in an accessor for token_index?

If you feel strongly I can remove token_index_ though.

+}
+
+
+RawScript* Breakpoint::SourceCode() {
+  const Function& func = Function::Handle(this->function_);
+  const Class& cls = Class::Handle(func.owner());
+  return cls.script();
+}
+
+
+RawString* Breakpoint::SourceUrl() {
+  const Script& script = Script::Handle(this->SourceCode());
+  return script.url();
+}
+
+
+intptr_t Breakpoint::LineNumber() {
+  // Compute line number lazily since it causes scanning of the script.
+  if (this->line_number_ < 0) {
+    const Script& script = Script::Handle(this->SourceCode());
+    intptr_t ignore_column;
+    script.GetTokenLocation(this->token_index_,
+                            &this->line_number_, &ignore_column);
   }
+  return this->line_number_;
+}
 
-  RawFunction* function() const { return function_; }
-  uword pc() const { return pc_; }
 
-  void set_next(Breakpoint* value) { next_ = value; }
-  Breakpoint* next() const { return this->next_; }
+void Breakpoint::VisitObjectPointers(ObjectPointerVisitor* visitor) {
+  visitor->VisitPointer(reinterpret_cast<RawObject**>(&function_));
+}
 
-  void VisitObjectPointers(ObjectPointerVisitor* visitor) {
-    visitor->VisitPointer(reinterpret_cast<RawObject**>(&function_));
+
+ActivationFrame::ActivationFrame(uword pc)
+    : pc_(pc),
+      function_(Function::null()),
+      token_index_(-1),
+      line_number_(-1) {
+}
+
+
+RawFunction* ActivationFrame::DartFunction() {
+  if (function_ == Function::null()) {
+    ASSERT(Isolate::Current() != NULL);
+    CodeIndexTable* code_index_table = Isolate::Current()->code_index_table();
+    ASSERT(code_index_table != NULL);
+    function_ = code_index_table->LookupFunction(pc_);
   }
+  return function_;
+}
 
- private:
-  RawFunction* function_;
-  intptr_t pc_desc_index_;
-  uword pc_;
-  Breakpoint* next_;
-  DISALLOW_COPY_AND_ASSIGN(Breakpoint);
-};
 
+RawString* ActivationFrame::SourceUrl() {
+  const Script& script = Script::Handle(SourceScript());
+  return script.url();
+}
 
+
+RawScript* ActivationFrame::SourceScript() {
+  const Function& func = Function::Handle(DartFunction());
+  const Class& cls = Class::Handle(func.owner());
+  return cls.script();
+}
+
+
+intptr_t ActivationFrame::TokenIndex() {
+  if (token_index_ < 0) {
+    const Function& func = Function::Handle(DartFunction());
+    Code& code = Code::Handle(func.code());
+    ASSERT(!code.IsNull());
+    PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
+    for (int i = 0; i < desc.Length(); i++) {
+      if (desc.PC(i) == pc_) {
+        token_index_ = desc.TokenIndex(i);
+        break;
+      }
+    }
+    ASSERT(token_index_ >= 0);
+  }
+  return token_index_;
+}
+
+
+intptr_t ActivationFrame::LineNumber() {
+  // Compute line number lazily since it causes scanning of the script.
+  if (line_number_ < 0) {
+    const Script& script = Script::Handle(SourceScript());
+    intptr_t ignore_column;
+    script.GetTokenLocation(TokenIndex(), &line_number_, &ignore_column);
+  }

[siva, 2011/12/05 19:21:44]

The line number computation code is duplicated here and above maybe it can be
factored into a single function intptr_t ComputeLineNumber(script, token_index);

[hausner, 2011/12/05 21:11:23]

I noticed that too, but I could really only factor out the call to
GetTokenLocation. I'm not sure that's really an abstraction. In a way,
GetTokenLocation is the factored-out code here.

+  return line_number_;
+}
+
+
+RawArray* ActivationFrame::Variables() {
+  UNIMPLEMENTED();
+}
+
+
+RawInstance* ActivationFrame::Value(const String& variable_name) {
+  UNIMPLEMENTED();
+}
+
+
+char* ActivationFrame::ToCString() {
+  const char* kFormat = "Function: '%s%s%s' url: '%s' line: %d";
+
+  Function& func = Function::Handle(DartFunction());
+  String& func_name = String::Handle(func.name());
+  Class& func_class = Class::Handle(func.owner());
+  String& class_name = String::Handle(func_class.Name());
+  String& url = String::Handle(SourceUrl());
+  intptr_t line = LineNumber();
+
+  intptr_t len = OS::SNPrint(NULL, 0, kFormat,
+                             class_name.ToCString(),
+                             func_class.IsTopLevel() ? "" : ".",
+                             func_name.ToCString(),
+                             url.ToCString(),
+                             line);
+  len++;  // String terminator.
+  char* chars = reinterpret_cast<char*>(
+      Isolate::Current()->current_zone()->Allocate(len));
+  OS::SNPrint(chars, len, kFormat,
+              class_name.ToCString(),
+              func_class.IsTopLevel() ? "" : ".",
+              func_name.ToCString(),
+              url.ToCString(),
+              line);
+  return chars;
+}
+
+
+void StackTrace::AddActivation(ActivationFrame* frame) {
+  this->trace_.Add(frame);
+}
+
+
 Debugger::Debugger()
     : initialized_(false),
+      bp_handler_(NULL),
       breakpoints_(NULL) {
 }
 
@@ -87,7 +216,6 @@
 }
 
 
-
 void Debugger::SetBreakpointAtEntry(const String& class_name,
                                     const String& function_name) {
   Function& func = Function::Handle();
@@ -97,8 +225,7 @@
     func = ResolveFunction(class_name, function_name);
   }
   if (func.IsNull()) {
-    OS::Print("could not find function '%s' in class '%s'\n",
-      function_name.ToCString(), class_name.ToCString());
+    OS::Print("could not find function '%s'\n", function_name.ToCString());
     return;
   }
   if (!func.HasCode()) {
@@ -109,27 +236,37 @@
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   for (int i = 0; i < desc.Length(); i++) {
     PcDescriptors::Kind kind = desc.DescriptorKind(i);
+    Breakpoint* bpt = NULL;
     if (kind == PcDescriptors::kIcCall) {
-      OS::Print("patching dynamic call at %p\n", desc.PC(i));
       CodePatcher::PatchInstanceCallAt(
           desc.PC(i), StubCode::BreakpointDynamicEntryPoint());
-      AddBreakpoint(new Breakpoint(func, i, desc.PC(i)));
-      return;
+      bpt = new Breakpoint(func, i);
     } else if (kind == PcDescriptors::kOther) {
       if (CodePatcher::IsDartCall(desc.PC(i))) {
-        OS::Print("patching static call at %p\n", desc.PC(i));
         CodePatcher::PatchStaticCallAt(
             desc.PC(i), StubCode::BreakpointStaticEntryPoint());
-        AddBreakpoint(new Breakpoint(func, i, desc.PC(i)));
-        return;
+        bpt = new Breakpoint(func, i);
       }
     }
+    if (bpt != NULL) {
+      OS::Print("Setting breakpoint at '%s' line %d  (PC %p)\n",
+          String::Handle(bpt->SourceUrl()).ToCString(),
+          bpt->LineNumber(),
+          bpt->pc());
+      AddBreakpoint(bpt);
+      return;
+    }
   }
   OS::Print("no breakpoint location found in function '%s'\n",
          function_name.ToCString());
 }
 
 
+void Debugger::SetBreakpointHandler(BreakpointHandler* handler) {
+  bp_handler_ = handler;
+}
+
+
 void Debugger::VisitObjectPointers(ObjectPointerVisitor* visitor) {
   ASSERT(visitor != NULL);
   Breakpoint* bpt = this->breakpoints_;
@@ -140,6 +277,43 @@
 }
 
 
+static void DefaultBreakpointHandler(Breakpoint* bpt, StackTrace* stack) {
+  for (int i = 0; i < stack->Length(); i++) {

[siva, 2011/12/05 19:21:44]

stack->Length() returns an intptr_t so should i also be an intptr_t?

[hausner, 2011/12/05 21:11:23]

Done.

+    OS::Print("   %d. %s\n",
+              i + 1, stack->ActivationFrameAt(i)->ToCString());
+  }
+}
+
+
+void Debugger::BreakpointCallback() {
+  ASSERT(initialized_);
+  DartFrameIterator iterator;
+  DartFrame* frame = iterator.NextFrame();
+  Function& func = Function::Handle();
+  ASSERT(frame != NULL);
+  Breakpoint* bpt = GetBreakpoint(frame->pc());
+  ASSERT(bpt != NULL);
+  OS::Print(">>> Breakpoint at %s:%d (Address %p)\n",
+      bpt ? String::Handle(bpt->SourceUrl()).ToCString() : "?",
+      bpt ? bpt->LineNumber() : 0,
+      frame->pc());
+  StackTrace* stack_trace = new StackTrace(8);
+  while (frame != NULL) {
+    ASSERT(frame->IsValid());
+    ASSERT(frame->IsDartFrame());
+    ActivationFrame* activation = new ActivationFrame(frame->pc());
+    stack_trace->AddActivation(activation);
+    frame = iterator.NextFrame();
+  }
+
+  if (bp_handler_ != NULL) {
+    (*bp_handler_)(bpt, stack_trace);
+  } else {
+    DefaultBreakpointHandler(bpt, stack_trace);
+  }

[siva, 2011/12/05 19:21:44]

Why not register DefaultBreakpointHandler as the default handler in
Debugger::Initialize() and
ASSERT(bp_handler_ != NULL); here.

[hausner, 2011/12/05 21:11:23]

Yes. Done.
 

+}
+
+
 void Debugger::Initialize(Isolate* isolate) {
   if (initialized_) {
     return;