Change how debugger handles source breakpoints

runtime/vm/debugger.cc

 // Copyright (c) 2012, 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 "include/dart_api.h"
 
 #include "vm/code_generator.h"
 #include "vm/code_patcher.h"
@@ skipping 32 matching lines @@
   }
 
  private:
   GrowableObjectArray* objs_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteObjectCache);
 };
 
 
 SourceBreakpoint::SourceBreakpoint(intptr_t id,
-                                   const Function& func,
-                                   intptr_t token_pos)
+                                   const Script& script,
+                                   intptr_t range_start,
+                                   intptr_t range_end)
     : id_(id),
-      function_(func.raw()),
-      token_pos_(token_pos),
-      line_number_(-1),
+      script_(script.raw()),
+      range_start_(range_start),
+      range_end_(range_end),
       is_enabled_(false),
-      next_(NULL) {
-  ASSERT(!func.IsNull());
-  ASSERT((func.token_pos() <= token_pos_) &&
-         (token_pos_ <= func.end_token_pos()));
+      next_(NULL),
+      token_pos_(-1),
+      line_number_(-1) {
+  ASSERT(id_ > 0);
+  ASSERT(!script.IsNull());
+  ASSERT((range_start_ >= 0) && (range_start_ <= range_end_));

[turnidge, 2013/12/19 22:31:59]

Should this <= be a <?  It depends whether the range is meant to include the
limit or not.

You use a < in RangeOverlaps below... should that be a <= or not?

[hausner, 2013/12/21 00:08:32]

obsolete

 }
 
 
 void SourceBreakpoint::Enable() {
   is_enabled_ = true;
   Isolate::Current()->debugger()->SyncBreakpoint(this);
 }
 
 
 void SourceBreakpoint::Disable() {
   is_enabled_ = false;
   Isolate::Current()->debugger()->SyncBreakpoint(this);
 }
 
 
-RawScript* SourceBreakpoint::SourceCode() {
-  const Function& func = Function::Handle(function_);
-  return func.script();
+void SourceBreakpoint::SetResolved(const Function& func, intptr_t token_pos) {

[turnidge, 2013/12/19 22:31:59]

What do you think of renaming "SetResolved" to "Resolve"?

[hausner, 2013/12/21 00:08:32]

I like SetResolved better. The actual resolving (computing the token position of
the safe point) happens in a different function. Here we just update the token
position and mark the breakpoint as resolved. Does this make sense?

+  ASSERT(func.script() == script_);
+  function_ = func.raw();
+  token_pos_ = token_pos;
 }
 
 
+// TODO(hausner): Get rid of library parameter. A source breakpoint location
+// does not imply a library, since the same source code can be included
+// in more than one library, e.g. the text location of mixin functions.
 void SourceBreakpoint::GetCodeLocation(
     Library* lib,
     Script* script,
-    intptr_t* pos) const {
-  const Function& func = Function::Handle(function_);
-  const Class& cls = Class::Handle(func.origin());
-  *lib = cls.library();
-  *script = func.script();
-  *pos = token_pos();
+    intptr_t* pos) {
+  *script = this->script();
+  if (IsResolved()) {
+    const Function& func = Function::Handle(function_);
+    ASSERT(!func.IsNull());
+    const Class& cls = Class::Handle(func.origin());
+    *lib = cls.library();
+    *pos = token_pos_;
+  } else {
+    *lib = Library::null();
+    *pos = range_start_;
+  }
 }
 
 
 RawString* SourceBreakpoint::SourceUrl() {
-  const Script& script = Script::Handle(SourceCode());
-  return script.url();
+  return Script::Handle(script()).url();
 }
 
 
 intptr_t SourceBreakpoint::LineNumber() {
   // Compute line number lazily since it causes scanning of the script.
+  ASSERT(IsResolved());
   if (line_number_ < 0) {
-    const Script& script = Script::Handle(SourceCode());
+    const Script& script = Script::Handle(this->script());
     script.GetTokenLocation(token_pos_, &line_number_, NULL);
   }
   return line_number_;
 }
 
 
-void SourceBreakpoint::set_function(const Function& func) {
-  function_ = func.raw();
-}
-
-
 void SourceBreakpoint::VisitObjectPointers(ObjectPointerVisitor* visitor) {
+  visitor->VisitPointer(reinterpret_cast<RawObject**>(&script_));
   visitor->VisitPointer(reinterpret_cast<RawObject**>(&function_));
 }
 
 
-void SourceBreakpoint::PrintToJSONStream(JSONStream* stream) const {
+void SourceBreakpoint::PrintToJSONStream(JSONStream* stream) {
   Isolate* isolate = Isolate::Current();
 
   JSONObject jsobj(stream);
   jsobj.AddProperty("type", "Breakpoint");
 
   jsobj.AddProperty("id", id());
   jsobj.AddProperty("enabled", IsEnabled());
-
-  const Function& func = Function::Handle(function());
-  jsobj.AddProperty("resolved", func.HasCode());
+  jsobj.AddProperty("resolved", IsResolved());
 
   Library& library = Library::Handle(isolate);
   Script& script = Script::Handle(isolate);
   intptr_t token_pos;
   GetCodeLocation(&library, &script, &token_pos);
   {
     JSONObject location(&jsobj, "location");
     location.AddProperty("type", "Location");
-    location.AddProperty("libId", library.index());
 
     const String& url = String::Handle(script.url());
     location.AddProperty("script", url.ToCString());
     location.AddProperty("tokenPos", token_pos);
   }
 }
 
 
 void CodeBreakpoint::VisitObjectPointers(ObjectPointerVisitor* visitor) {
   visitor->VisitPointer(reinterpret_cast<RawObject**>(&function_));
@@ skipping 70 matching lines @@
   return chars;
 }
 
 
 bool Debugger::HasBreakpoint(const Function& func) {
   if (!func.HasCode()) {
     // If the function is not compiled yet, just check whether there
     // is a user-defined latent breakpoint.
     SourceBreakpoint* sbpt = src_breakpoints_;
     while (sbpt != NULL) {
-      if (func.raw() == sbpt->function()) {
+      if (sbpt->IsResolved() && (func.raw() == sbpt->function())) {

[turnidge, 2013/12/19 22:31:59]

So this function has no code, has not been compiled, but the breakpoints have
been resolved?  The breakpoint is both "resolved" and "latent" (from comment)?

I don't know the states here too well, so just making sure this makes sense.  I
would have thought that a resolved breakpoint wouldn't be latent and would have
code.

[hausner, 2013/12/21 00:08:32]

Yes that seems fishy. Changed so it returns true if the breakpoint falls within
the function's token range.

         return true;
       }
       sbpt = sbpt->next_;
     }
     return false;
   }
   CodeBreakpoint* cbpt = code_breakpoints_;
   while (cbpt != NULL) {
     if (func.raw() == cbpt->function()) {
       return true;
@@ skipping 1086 matching lines @@
           (desc_token_pos <= last_token_pos) &&
           (desc.PC(i) < lowest_pc)) {
         // This descriptor is within the token position range and so
         // far has the lowest code address.
         lowest_pc = desc.PC(i);
         lowest_pc_index = i;
       }
     }
   }
   if (lowest_pc_index >= 0) {
-    // We found the the pc descriptor within the given token range that
+    // We found the pc descriptor within the given token range that
     // has the lowest execution address. This is the first possible
     // breakpoint on the line. We use this instead of the nearest
     // PC descriptor measured in token index distance.
     best_fit_index = lowest_pc_index;
   }
   if (best_fit_index >= 0) {
     return desc.TokenPos(best_fit_index);
   }
   return -1;
 }
 
 
+void Debugger::FindAllFunctions(const Script& script,

[turnidge, 2013/12/19 22:31:59]

I find the name confusing -- it seems to be trying to find *equivalent*
functions to some other function.  Would it work to change the name to something
like FindEquivalentFunctions or FindMatchingFunctions?

[hausner, 2013/12/21 00:08:32]

Name changed.

+                                intptr_t start_pos,
+                                intptr_t end_pos,
+                                GrowableObjectArray* function_list) {

[turnidge, 2013/12/19 22:31:59]

This function belongs lower in the file, nearer to FindBestFit and
SetBreakpoint.

[hausner, 2013/12/21 00:08:32]

Done.

+  Class& cls = Class::Handle(isolate_);
+  Array& functions = Array::Handle(isolate_);
+  GrowableObjectArray& closures = GrowableObjectArray::Handle(isolate_);
+  Function& function = Function::Handle(isolate_);
+
+  const ClassTable& class_table = *isolate_->class_table();
+  const intptr_t num_classes = class_table.NumCids();
+  for (intptr_t i = 1; i < num_classes; i++) {
+    if (class_table.HasValidClassAt(i)) {
+      cls = class_table.At(i);
+      // Note: we need to check the functions of this class even if
+      // the class is defined in a differenct 'script'. There could
+      // be mixin functions from the given script in this class.
+      functions = cls.functions();
+      if (!functions.IsNull()) {
+        const intptr_t num_functions = functions.Length();
+        for (intptr_t pos = 0; pos < num_functions; pos++) {
+          function ^= functions.At(pos);
+          ASSERT(!function.IsNull());
+          // Check token position first to avoid unnecessary calls
+          // to script() which allocates handles.
+          if ((function.token_pos() == start_pos)
+              && (function.end_token_pos() == end_pos)
+              && (function.script() == script.raw())) {
+            function_list->Add(function);
+            if (function.HasImplicitClosureFunction()) {
+              function = function.ImplicitClosureFunction();
+              function_list->Add(function);
+            }
+          }
+        }
+      }
+
+      closures = cls.closures();
+      if (!closures.IsNull()) {
+        const intptr_t num_closures = closures.Length();
+        for (intptr_t pos = 0; pos < num_closures; pos++) {
+          function ^= closures.At(pos);
+          ASSERT(!function.IsNull());
+          if ((function.token_pos() == start_pos)
+              && (function.end_token_pos() == end_pos)
+              && (function.script() == script.raw())) {
+            function_list->Add(function);
+            if (function.HasImplicitClosureFunction()) {
+              function = function.ImplicitClosureFunction();
+              function_list->Add(function);
+            }
+          }
+        }
+      }
+    }
+  }
+}
+
+
 void Debugger::MakeCodeBreakpointsAt(const Function& func,
-                                     intptr_t token_pos,
                                      SourceBreakpoint* bpt) {
+  ASSERT((bpt != NULL) && bpt->IsResolved());
   ASSERT(!func.HasOptimizedCode());
   Code& code = Code::Handle(func.unoptimized_code());
   ASSERT(!code.IsNull());
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   for (intptr_t i = 0; i < desc.Length(); i++) {
     intptr_t desc_token_pos = desc.TokenPos(i);
-    if ((desc_token_pos == token_pos) && IsSafePoint(desc.DescriptorKind(i))) {
+    if ((desc_token_pos == bpt->token_pos_) &&
+        IsSafePoint(desc.DescriptorKind(i))) {
       CodeBreakpoint* code_bpt = GetCodeBreakpoint(desc.PC(i));
       if (code_bpt == NULL) {
         // No code breakpoint for this code exists; create one.
         code_bpt = new CodeBreakpoint(func, i);
         RegisterCodeBreakpoint(code_bpt);
       }
       code_bpt->set_src_bpt(bpt);
+      if (bpt->IsEnabled()) {
+        code_bpt->Enable();
+      }
     }
   }
 }
 
 
-SourceBreakpoint* Debugger::SetBreakpoint(const Function& target_function,
-                                          intptr_t first_token_pos,
-                                          intptr_t last_token_pos) {
-  if ((last_token_pos < target_function.token_pos()) ||
-      (target_function.end_token_pos() < first_token_pos)) {
-    // The given token position is not within the target function.
-    return NULL;
+static void SelectBestFit(Function* best_fit, Function* func) {
+  if (best_fit->IsNull()) {
+    *best_fit = func->raw();
   }
-  intptr_t breakpoint_pos = -1;
-  Function& closure = Function::Handle(isolate_);
-  if (target_function.HasImplicitClosureFunction()) {
-    // There is a closurized version of this function.
-    closure = target_function.ImplicitClosureFunction();
+  if (func->token_pos() > best_fit->token_pos()) {
+    if (func->end_token_pos() <= best_fit->end_token_pos()) {
+      // func is contained within best_fit. Select it even if it
+      // has not been compiled yet.
+      *best_fit = func->raw();
+      if (func->HasImplicitClosureFunction()) {
+        *func = func->ImplicitClosureFunction();
+        if (func->HasCode()) {
+          *best_fit = func->raw();
+        }
+      }
+    }
+  } else if ((func->token_pos() == best_fit->token_pos())
+      && (func->end_token_pos() == best_fit->end_token_pos())
+      && func->HasCode()) {
+    // If func covers the same range, it is considered a better fit if
+    // it has been compiled.
+    *best_fit = func->raw();
   }
-  // Determine actual breakpoint location if the function or an
-  // implicit closure of the function has been compiled already.
-  if (target_function.HasCode()) {
-    DeoptimizeWorld();
-    ASSERT(!target_function.HasOptimizedCode());
-    breakpoint_pos =
-        ResolveBreakpointPos(target_function, first_token_pos, last_token_pos);
-  } else if (!closure.IsNull() && closure.HasCode()) {
-    DeoptimizeWorld();
-    ASSERT(!closure.HasOptimizedCode());
-    breakpoint_pos =
-        ResolveBreakpointPos(closure, first_token_pos, last_token_pos);
-  } else {
-    // This function has not been compiled yet. Set a pending
-    // breakpoint to be resolved later.
-    SourceBreakpoint* source_bpt =
-        GetSourceBreakpoint(target_function, first_token_pos);
-    if (source_bpt != NULL) {
-      // A pending source breakpoint for this uncompiled location
-      // already exists.
-      if (FLAG_verbose_debug) {
-        OS::Print("Pending breakpoint for uncompiled function"
-                  " '%s' at line %" Pd " already exists\n",
-                  target_function.ToFullyQualifiedCString(),
-                  source_bpt->LineNumber());
-      }
-      return source_bpt;
-    }
-    source_bpt =
-        new SourceBreakpoint(nextId(), target_function, first_token_pos);
-    RegisterSourceBreakpoint(source_bpt);
-    if (FLAG_verbose_debug) {
-      OS::Print("Registering pending breakpoint for "
-                "uncompiled function '%s' at line %" Pd "\n",
-                target_function.ToFullyQualifiedCString(),
-                source_bpt->LineNumber());
-    }
-    source_bpt->Enable();
-    return source_bpt;
-  }
-  ASSERT(breakpoint_pos != -1);
-  SourceBreakpoint* source_bpt =
-      GetSourceBreakpoint(target_function, breakpoint_pos);
-  if (source_bpt != NULL) {
-    // A source breakpoint for this location already exists.
-    return source_bpt;
-  }
-  source_bpt = new SourceBreakpoint(nextId(), target_function, breakpoint_pos);
-  RegisterSourceBreakpoint(source_bpt);
-  if (target_function.HasCode()) {
-    MakeCodeBreakpointsAt(target_function, breakpoint_pos, source_bpt);
-  }
-  if (!closure.IsNull() && closure.HasCode()) {
-    MakeCodeBreakpointsAt(closure, breakpoint_pos, source_bpt);
-  }
-  source_bpt->Enable();
-  SignalBpResolved(source_bpt);
-  return source_bpt;
 }
 
 
+// Returns true if function is at least partially overlapping
+// the given token range in a script.
+static bool RangeOverlaps(const Function& func,
+                          const Script& script,
+                          intptr_t range_start,
+                          intptr_t range_end) {
+  if ((func.end_token_pos() > range_start) && (func.token_pos() < range_end)) {
+    // Check script equality second because it allocates
+    // handles as a side effect.
+    return func.script() == script.raw();
+  }
+  return false;

[turnidge, 2013/12/19 22:31:59]

This cl contains a lot of range math that could have minor errors at the
endpoints, etc.  It looks fine to me but it is a good candidate for unit
testing.

If I were testing, I would write some small unit tests of SelectBestFit,
RangeOverlaps (building block functions) to cover edge cases.  I would then
write tests to make sure that FindBestFit works and that FindAllFunctions (or
FindEquivalentFunctions) really finds the full equivalence class of functions. 
Then I would write a few higher level tests of SetBreakpoint and make sure that
the token pos gets resolved to what I expected.

These tests could also help document why you made the heuristic choices that you
have made for future coders.

I realize this will slow you down a bit, sorry.

[hausner, 2013/12/21 00:08:32]

Range math simplified and test added (not unit test though).

+}
+
+

[turnidge, 2013/12/19 22:31:59]

Could use a comment saying what it means for one function to be a better fit,
and why.

[hausner, 2013/12/21 00:08:32]

I think the comments you suggest are inside the function, no?

+RawFunction* Debugger::FindBestFit(const Script& script,
+                                   intptr_t range_start,
+                                   intptr_t range_end) {

[turnidge, 2013/12/19 22:31:59]

Out of curiousity, could this function be static?

[hausner, 2013/12/21 00:08:32]

Yes, but then I could not use the debugger object's isolate_ field to speed up
the handle allocation.

+  Class& cls = Class::Handle(isolate_);
+  Array& functions = Array::Handle(isolate_);
+  GrowableObjectArray& closures = GrowableObjectArray::Handle(isolate_);
+  Function& function = Function::Handle(isolate_);
+  Function& best_fit = Function::Handle(isolate_);
+
+  const ClassTable& class_table = *isolate_->class_table();
+  const intptr_t num_classes = class_table.NumCids();
+  for (intptr_t i = 1; i < num_classes; i++) {
+    if (class_table.HasValidClassAt(i)) {
+      cls = class_table.At(i);
+      // Note: we need to check the functions of this class even if
+      // the class is defined in a differenct 'script'. There could
+      // be mixin functions from the given script in this class.
+      functions = cls.functions();
+      if (!functions.IsNull()) {
+        const intptr_t num_functions = functions.Length();
+        for (intptr_t pos = 0; pos < num_functions; pos++) {
+          function ^= functions.At(pos);
+          ASSERT(!function.IsNull());
+          if (RangeOverlaps(function, script, range_start, range_end)) {
+            SelectBestFit(&best_fit, &function);
+          }
+        }
+      }
+
+      closures = cls.closures();
+      if (!closures.IsNull()) {
+        const intptr_t num_closures = closures.Length();
+        for (intptr_t pos = 0; pos < num_closures; pos++) {
+          function ^= closures.At(pos);
+          ASSERT(!function.IsNull());
+          if (RangeOverlaps(function, script, range_start, range_end)) {
+            SelectBestFit(&best_fit, &function);

[turnidge, 2013/12/19 22:31:59]

If we were to change SelectBestFit to return the RawFunction* of the best fit,
the code would look like this:

best_fit = BestFit(best_fit, function);

Which would get rid of the & operators and make the code slightly more obvious
to me.

Your call.

[hausner, 2013/12/21 00:08:32]

I wanted to reuse the handles so that SelectBestFit does not have to allocate
any handles...

+          }
+        }
+      }
+    }
+  }
+  return best_fit.raw();
+}
+
+
+SourceBreakpoint* Debugger::SetBreakpoint(const Script& script,
+                                          intptr_t range_start,
+                                          intptr_t range_end) {
+  Function& func = Function::Handle(isolate_);
+  func = FindBestFit(script, range_start, range_end);
+  if (func.IsNull()) {
+    return NULL;
+  }
+  if (!func.IsNull() && func.HasCode()) {
+    // A function containing this breakpoint location has already
+    // been compiled. We can resolve the breakpoint now.
+    DeoptimizeWorld();
+    intptr_t breakpoint_pos =
+        ResolveBreakpointPos(func, range_start, range_end);
+    if (breakpoint_pos >= 0) {
+      SourceBreakpoint* bpt = GetResolvedBreakpoint(script, breakpoint_pos);
+      if (bpt != NULL) {
+        // A source breakpoint for this location already exists.
+        return bpt;
+      }
+      bpt = new SourceBreakpoint(nextId(), script, range_start, range_end);
+      bpt->SetResolved(func, breakpoint_pos);
+      RegisterSourceBreakpoint(bpt);
+      // There may be more than one function object for a given function
+      // in source code. There may be implicit closure functions, and
+      // there may be copies of mixin functions. Collect all functions whose
+      // source code range matches exactly the best fit function we
+      // found.
+      GrowableObjectArray& functions =
+          GrowableObjectArray::Handle(GrowableObjectArray::New());
+      FindAllFunctions(script,
+                       func.token_pos(),
+                       func.end_token_pos(),
+                       &functions);
+      const intptr_t num_functions = functions.Length();
+      // We must have found at least one function: func.
+      ASSERT(num_functions > 0);
+      // Create code breakpoints for all compiled functions we found.
+      for (intptr_t i = 0; i < num_functions; i++) {
+        func ^= functions.At(i);
+        if (func.HasCode()) {
+          MakeCodeBreakpointsAt(func, bpt);
+        }
+      }
+      bpt->Enable();

[turnidge, 2013/12/19 22:31:59]

If we move the call to Enable before we make the code breakpoints, does that
work too?  The code breakpoints would then be enabled as they are created
instead of being enabled later.

[hausner, 2013/12/21 00:08:32]

Yes but the call to Enable() has the side effect of going over all the code
breakpoints anyway. So if we do it here all the function objects that contain
this breakpoint will be instrumented at the same time.

+      SignalBpResolved(bpt);
+      return bpt;
+    }
+  }
+  // There is no compiled function at this range. Register an unresolved
+  // breakpoint.
+  if (FLAG_verbose_debug && !func.IsNull()) {
+    intptr_t line_number;
+    script.GetTokenLocation(range_start, &line_number, NULL);
+    OS::Print("Registering pending breakpoint for "
+              "uncompiled function '%s' at line %" Pd "\n",
+              func.ToFullyQualifiedCString(),
+              line_number);
+  }
+  SourceBreakpoint* bpt = GetUnresolvedBreakpoint(script, range_start);
+  if (bpt == NULL) {
+    bpt = new SourceBreakpoint(nextId(), script, range_start, range_end);
+  }
+  RegisterSourceBreakpoint(bpt);
+  bpt->Enable();
+  return bpt;
+}
+
+
 // Synchronize the enabled/disabled state of all code breakpoints
 // associated with the source breakpoint bpt.
 void Debugger::SyncBreakpoint(SourceBreakpoint* bpt) {
   CodeBreakpoint* cbpt = code_breakpoints_;
   while (cbpt != NULL) {
     if (bpt == cbpt->src_bpt()) {
       if (bpt->IsEnabled()) {
         cbpt->Enable();
       } else {
         cbpt->Disable();
@@ skipping 10 matching lines @@
     const Function& closure_func =
         Function::Handle(target_function.ImplicitClosureFunction());
     InstrumentForStepping(closure_func);
   }
 }
 
 
 SourceBreakpoint* Debugger::SetBreakpointAtEntry(
       const Function& target_function) {
   ASSERT(!target_function.IsNull());
-  return SetBreakpoint(target_function,
+  const Script& script = Script::Handle(target_function.script());
+  return SetBreakpoint(script,
                        target_function.token_pos(),
-                       target_function.end_token_pos());
+                       target_function.token_pos() + 1);

[turnidge, 2013/12/19 22:31:59]

If the range end is included in the range, do you need the +1 here?  Would it
work to use token_pos() as both the start and the end?

[hausner, 2013/12/21 00:08:32]

obsolete.

 }
 
 
 SourceBreakpoint* Debugger::SetBreakpointAtLine(const String& script_url,
                                                 intptr_t line_number) {
   Library& lib = Library::Handle(isolate_);
   Script& script = Script::Handle(isolate_);
   const GrowableObjectArray& libs =
       GrowableObjectArray::Handle(isolate_->object_store()->libraries());
   for (intptr_t i = 0; i < libs.Length(); i++) {
@@ skipping 19 matching lines @@
                 script_url.ToCString(), line_number);
     }
     return NULL;
   } else if (last_token_idx < 0) {
     // Line does not contain any tokens. first_token_index is the first
     // token after the given line. We check whether that token is
     // part of a function.
     last_token_idx = first_token_idx;
   }
 
-  Function& func = Function::Handle(isolate_);
-  while (first_token_idx <= last_token_idx) {
-    func = lib.LookupFunctionInScript(script, first_token_idx);
-    if (!func.IsNull()) {
-      break;
-    }
-    first_token_idx++;
+  SourceBreakpoint* bpt =
+      SetBreakpoint(script, first_token_idx, last_token_idx);
+  if ((bpt == NULL) && FLAG_verbose_debug) {
+    OS::Print("No executable code at line %" Pd " in '%s'\n",
+                line_number, script_url.ToCString());
   }
-  if (func.IsNull()) {
-    if (FLAG_verbose_debug) {
-      OS::Print("No executable code at line %" Pd " in '%s'\n",
-                line_number, script_url.ToCString());
-    }
-    return NULL;
-  }
-  if (last_token_idx < 0) {
-    // The token at first_token_index is past the requested source line.
-    // Set the breakpoint at the closest position after that line.
-    last_token_idx = func.end_token_pos();
-  }
-  return SetBreakpoint(func, first_token_idx, last_token_idx);
+  return bpt;
 }
 
 
 intptr_t Debugger::CacheObject(const Object& obj) {
   ASSERT(obj_cache_ != NULL);
   return obj_cache_->AddObject(obj);
 }
 
 
 bool Debugger::IsValidObjectId(intptr_t obj_id) {
@@ skipping 359 matching lines @@
   // This port will be used as a unique ID to represet the isolate in the
   // debugger wire protocol messages.
   isolate_id_ = isolate->main_port();
   initialized_ = true;
 
   // Signal isolate creation event.
   SignalIsolateEvent(Debugger::kIsolateCreated);
 }
 
 
-static RawFunction* GetOriginalFunction(const Function& func) {
-  if (func.IsClosureFunction()) {
-    // Local functions (closures) in mixin functions do not have
-    // an original function they were cloned from.
-    // Note: this is a problem when a breakpoint is set in a mixed-in
-    // closure. The breakpoint is linked to the closure attached to the
-    // mixin application class, not the mixin class. When the same
-    // closure is compiled for another mixin application class, we
-    // don't find the breakpoint since we'll be looking in the
-    // mixin class.
-    return func.raw();
+// Return innermost closure contained in 'function' that entirely contains
+// the given token range.
+RawFunction* Debugger::FindInnermostClosure(const Function& function,
+                                            intptr_t range_start,
+                                            intptr_t range_end) {

[turnidge, 2013/12/19 22:31:59]

Could this be static?

[hausner, 2013/12/21 00:08:32]

Yes but I wanted to used debugger->isolate_ to speed up handle allocation.

+  const Class& owner = Class::Handle(isolate_, function.Owner());
+  if (owner.closures() == GrowableObjectArray::null()) {
+    return Function::null();
   }
-  const Class& origin_class = Class::Handle(func.origin());
-  if (origin_class.is_patch()) {
-    // Patched functions from patch classes are removed from the
-    // function array of the patch class, so we will not find an
-    // original function object.
-    return func.raw();
-  }
-  const Array& functions = Array::Handle(origin_class.functions());
-  Object& orig_func = Object::Handle();
-  for (intptr_t i = 0; i < functions.Length(); i++) {
-    orig_func = functions.At(i);
-    // Function names are symbols, so we can compare the raw pointers.
-    if (func.name() == Function::Cast(orig_func).name()) {
-      return Function::Cast(orig_func).raw();
+  // Note that we need to check that the closure is in the same
+  // script as the outer function. We could have closures originating
+  // in mixin classes whose source code is contained in a differenct

[turnidge, 2013/12/19 22:31:59]

typo "differenct"

[hausner, 2013/12/21 00:08:32]

Done.

+  // script.
+  const Script& outer_origin = Script::Handle(isolate_, function.script());
+  const GrowableObjectArray& closures =
+     GrowableObjectArray::Handle(isolate_, owner.closures());
+  const intptr_t num_closures = closures.Length();
+  Function& closure = Function::Handle(isolate_);
+  Function& best_fit = Function::Handle(isolate_);
+  for (intptr_t i = 0; i < num_closures; i++) {
+    closure ^= closures.At(i);
+    if ((function.token_pos() < closure.token_pos()) &&
+        (closure.end_token_pos() < function.end_token_pos()) &&
+        (closure.token_pos() <= range_start) &&
+        (range_end <= closure.end_token_pos()) &&
+        (closure.script() == outer_origin.raw())) {
+      SelectBestFit(&best_fit, &closure);
     }
   }
-  // Uncommon case: not a mixin function.
-  ASSERT(!Class::Handle(func.Owner()).IsMixinApplication());
-  return func.raw();
+  return best_fit.raw();
 }
 
 
 void Debugger::NotifyCompilation(const Function& func) {
   if (src_breakpoints_ == NULL) {
     // Return with minimal overhead if there are no breakpoints.
     return;
   }
-  Function& lookup_function = Function::Handle(func.raw());
-  if (func.IsImplicitClosureFunction()) {
-    // If the newly compiled function is a an implicit closure (a closure that
-    // was formed by assigning a static or instance method to a function
-    // object), we need to use the closure's parent function to see whether
-    // there are any breakpoints. The parent function is the actual method on
-    // which the user sets breakpoints.
-    lookup_function = func.parent_function();
-    ASSERT(!lookup_function.IsNull());
-  }
-  if (lookup_function.Owner() != lookup_function.origin()) {
-    // This is a cloned function from a mixin class. If a breakpoint
-    // was set in this function, it is registered using the function
-    // of the origin class.
-    lookup_function = GetOriginalFunction(lookup_function);
-  }
-  SourceBreakpoint* bpt = src_breakpoints_;
-  while (bpt != NULL) {
-    if (lookup_function.raw() == bpt->function()) {
-      // Check if the breakpoint is inside a closure or local function
-      // within the newly compiled function. Note that we must look
-      // in the owner class of the function that just got compiled
-      // (i.e. func), not the owner class of the function we use to
-      // record the breakpoint (lookup_function).
-      Class& owner = Class::Handle(func.Owner());
-      Function& closure =
-          Function::Handle(owner.LookupClosureFunction(bpt->token_pos()));
-      if (!closure.IsNull() && (closure.raw() != lookup_function.raw())) {
+  // Iterate over all source breakpoints to check whether breakpoints
+  // need to be set in the newly compiled function.
+  Script& script = Script::Handle(isolate_);
+  for (SourceBreakpoint* bpt = src_breakpoints_;
+       bpt != NULL;
+       bpt = bpt->next()) {
+       script = bpt->script();
+    if (RangeOverlaps(func, script, bpt->range_start_, bpt->range_end_)) {
+      Function& inner_function = Function::Handle(isolate_);

[turnidge, 2013/12/19 22:31:59]

Since the function name doesn't say "containing the range", that should be said
in a comment here.

[hausner, 2013/12/21 00:08:32]

obsolete

+      inner_function =
+          FindInnermostClosure(func, bpt->range_start_, bpt->range_end_);
+      if (!inner_function.IsNull()) {
+        // The local function of a function we just compiled cannot
+        // be compiled already.
+        ASSERT(!inner_function.HasCode());
         if (FLAG_verbose_debug) {
-          OS::Print("Resetting pending breakpoint to function %s\n",
-                    closure.ToFullyQualifiedCString());
+          OS::Print("Pending BP remains unresolved in inner function '%s'\n",
+                    inner_function.ToFullyQualifiedCString());
         }
-        bpt->set_function(closure);
-      } else {
+        continue;
+      }
+
+      // TODO(hausner): What should we do here? Can we deoptimize the function?
+      ASSERT(!func.HasOptimizedCode());
+
+      // There is no local function within func that contains the
+      // breakpoint token range. Resolve the breakpoint if necessary
+      // and set the code breakpoints.
+      if (!bpt->IsResolved()) {
+        // Resolve source breakpoint in the newly compiled function.
+        intptr_t bp_pos =
+            ResolveBreakpointPos(func, bpt->range_start_, bpt->range_end_);
+        if (bp_pos < 0) {
+          if (FLAG_verbose_debug) {
+            OS::Print("Failed resolving breakpoint for function '%s'\n",
+                      String::Handle(func.name()).ToCString());
+          }
+          continue;
+        }
+        bpt->SetResolved(func, bp_pos);
         if (FLAG_verbose_debug) {
-          OS::Print("Enable pending breakpoint for function '%s'\n",
-                    String::Handle(lookup_function.name()).ToCString());
+          OS::Print("Resolved BP %" Pd " to pos %" Pd ", line %" Pd ", "
+                    "function '%s' (requested range %" Pd "-%" Pd ")\n",
+                    bpt->id(),
+                    bpt->token_pos(),
+                    bpt->LineNumber(),
+                    func.ToFullyQualifiedCString(),
+                    bpt->range_start_, bpt->range_end_);
         }
-        const Script& script= Script::Handle(func.script());
-        intptr_t first_pos, last_pos;
-        script.TokenRangeAtLine(bpt->LineNumber(), &first_pos, &last_pos);
-        intptr_t bp_pos =
-            ResolveBreakpointPos(func, bpt->token_pos(), last_pos);
-        bpt->set_token_pos(bp_pos);
-        MakeCodeBreakpointsAt(func, bp_pos, bpt);
         SignalBpResolved(bpt);
       }
-      bpt->Enable();  // Enables the code breakpoint as well.
+      ASSERT(bpt->IsResolved());
+      if (FLAG_verbose_debug) {
+        OS::Print("Setting breakpoint %" Pd " at line %" Pd " for %s '%s'\n",
+                  bpt->id(),
+                  bpt->LineNumber(),
+                  func.IsClosureFunction() ? "closure" : "function",
+                  String::Handle(func.name()).ToCString());
+      }
+      MakeCodeBreakpointsAt(func, bpt);

[turnidge, 2013/12/19 22:31:59]

In SetBreakpoint, you call SignalBpResolved *after* you call
MakeCodeBreakpointsAt.  Here you call SIgnalBpResolved *before* you call
MakeCodeBreakpointsAt.  Does this matter?  Is there any circumstance where the
embedder's breakpoint resolved callback might *want* to hit the newly resolved
breakpoint?

[hausner, 2013/12/21 00:08:32]

I don't think it matters. The isolate is not paused when this code runs so the
resolved event gets queued before it's sent over the wire. If you think we
should send it after all code breakpoints are made, we have to keep a list of
all BPs that got resolved as a result of this compilation and signal the events
after the list is entirely traversed.

     }
-    bpt = bpt->next();
   }
 }
 
 
 CodeBreakpoint* Debugger::GetCodeBreakpoint(uword breakpoint_address) {
   CodeBreakpoint* bpt = code_breakpoints_;
   while (bpt != NULL) {
     if (bpt->pc() == breakpoint_address) {
       return bpt;
     }
@@ skipping 73 matching lines @@
       temp_bpt->Disable();
       delete temp_bpt;
     } else {
       prev_bpt = curr_bpt;
       curr_bpt = curr_bpt->next();
     }
   }
 }
 
 
-SourceBreakpoint* Debugger::GetSourceBreakpoint(const Function& func,
-                                                intptr_t token_pos) {
+SourceBreakpoint* Debugger::GetUnresolvedBreakpoint(const Script& script,
+                                                   intptr_t range_start) {
   SourceBreakpoint* bpt = src_breakpoints_;
   while (bpt != NULL) {
-    if ((bpt->function() == func.raw()) &&
-        (bpt->token_pos() == token_pos)) {
+    if ((bpt->script_ == script.raw()) && (bpt->range_start_ == range_start)) {

[turnidge, 2013/12/19 22:31:59]

SetResolved does not clear the range_start_, does it?  Isn't it possible for
this function to return a resolved breakpoint?  What do you think of checking
IsResolved in GetUnresolvedBreakpoint and GetResolvedBreakpoint?

[hausner, 2013/12/21 00:08:32]

obsolete

       return bpt;
     }
     bpt = bpt->next();
   }
   return NULL;
 }
 
 
+SourceBreakpoint* Debugger::GetResolvedBreakpoint(const Script& script,
+                                                  intptr_t token_pos) {
+  SourceBreakpoint* bpt = src_breakpoints_;
+  while (bpt != NULL) {
+    if ((bpt->script_ == script.raw()) && (bpt->token_pos_ == token_pos)) {
+      return bpt;
+    }
+    bpt = bpt->next();
+  }
+  return NULL;
+}
+
+
 SourceBreakpoint* Debugger::GetBreakpointById(intptr_t id) {
   SourceBreakpoint* bpt = src_breakpoints_;
   while (bpt != NULL) {
     if (bpt->id() == id) {
       return bpt;
     }
     bpt = bpt->next();
   }
   return NULL;
 }
 
 
 void Debugger::RegisterSourceBreakpoint(SourceBreakpoint* bpt) {
   ASSERT(bpt->next() == NULL);
   bpt->set_next(src_breakpoints_);
   src_breakpoints_ = bpt;
 }
 
 
 void Debugger::RegisterCodeBreakpoint(CodeBreakpoint* bpt) {
   ASSERT(bpt->next() == NULL);
   bpt->set_next(code_breakpoints_);
   code_breakpoints_ = bpt;
 }
 
 }  // namespace dart