Support column-based breakpoints in the VM and Observatory.

runtime/vm/debugger.cc

Index: runtime/vm/debugger.cc
diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc
index 0cd16d852896199b9843d5d2687776c0c9e4dfb1..b3db87e350fee9a1d2f4f352eeb09229fb148364 100644
--- a/runtime/vm/debugger.cc
+++ b/runtime/vm/debugger.cc
@@ -65,7 +65,8 @@ class RemoteObjectCache : public ZoneAllocated {
 // Create an unresolved breakpoint in given token range and script.
 BreakpointLocation::BreakpointLocation(const Script& script,
                                        intptr_t token_pos,
-                                       intptr_t end_token_pos)
+                                       intptr_t end_token_pos,
+                                       intptr_t requested_column_number)
     : script_(script.raw()),
       url_(script.url()),
       token_pos_(token_pos),
@@ -73,15 +74,19 @@ BreakpointLocation::BreakpointLocation(const Script& script,
       is_resolved_(false),
       next_(NULL),
       conditions_(NULL),
+      requested_line_number_(-1),
+      requested_column_number_(requested_column_number),
       function_(Function::null()),
-      line_number_(-1) {
+      line_number_(-1),
+      column_number_(-1) {
   ASSERT(!script.IsNull());
   ASSERT(token_pos_ >= 0);
 }
 
 // Create a latent breakpoint at given url and line number.
 BreakpointLocation::BreakpointLocation(const String& url,
-                                       intptr_t line_number)
+                                       intptr_t requested_line_number,
+                                       intptr_t requested_column_number)
     : script_(Script::null()),
       url_(url.raw()),
       token_pos_(-1),
@@ -89,8 +94,11 @@ BreakpointLocation::BreakpointLocation(const String& url,
       is_resolved_(false),
       next_(NULL),
       conditions_(NULL),
+      requested_line_number_(requested_line_number),
+      requested_column_number_(requested_column_number),
       function_(Function::null()),
-      line_number_(line_number) {
+      line_number_(-1),
+      column_number_(-1) {
   ASSERT(line_number_ >= 0);

[hausner, 2015/09/03 00:37:50]

This assert would fire right away, since the field is initialized to -1. 

It looks like this constructor is not executed in our tests.

[turnidge, 2015/09/04 18:05:29]

Thanks.  I have added a set of tests for latent breakpoint to my CLs.

 }
 
@@ -119,7 +127,6 @@ void BreakpointLocation::SetResolved(const Function& func, intptr_t token_pos) {
   function_ = func.raw();
   token_pos_ = token_pos;
   end_token_pos_ = token_pos;
-  line_number_ = -1;  // Recalculate lazily.

[hausner, 2015/09/03 00:37:50]

Why is it not necessary to recalculate the line number once we know the
definitive location of the breakpoint?

[turnidge, 2015/09/04 18:05:29]

After my change, the line number before resolution is stored in
requested_line_number_ and the line number after resolution is stored in
line_number_.  Because the field isn't being used for both things, the field is
still -1 from the constructor call.

   is_resolved_ = true;
 }
 
@@ -150,8 +157,7 @@ void BreakpointLocation::GetCodeLocation(Library* lib,
 
 
 intptr_t BreakpointLocation::LineNumber() {
-  // Latent breakpoints must have a requested line number >= 0.
-  ASSERT(!IsLatent() || line_number_ >= 0);
+  ASSERT(IsResolved());
   // Compute line number lazily since it causes scanning of the script.
   if (line_number_ < 0) {
     const Script& script = Script::Handle(this->script());
@@ -161,6 +167,17 @@ intptr_t BreakpointLocation::LineNumber() {
 }
 
 
+intptr_t BreakpointLocation::ColumnNumber() {
+  ASSERT(IsResolved());
+  // Compute column number lazily since it causes scanning of the script.
+  if (column_number_ < 0) {
+    const Script& script = Script::Handle(this->script());
+    script.GetTokenLocation(token_pos_, &line_number_, &column_number_);
+  }
+  return column_number_;
+}
+
+
 void Breakpoint::set_bpt_location(BreakpointLocation* new_bpt_location) {
   ASSERT(bpt_location_->IsLatent());  // Only reason to move.
   bpt_location_ = new_bpt_location;
@@ -1608,7 +1625,8 @@ static intptr_t LastTokenOnLine(const TokenStream& tokens, intptr_t pos) {
 // that line, the safe point with the lowest compiled code address.

[hausner, 2015/09/03 00:37:50]

Update the comment to describe how the requested column is taken into account?

[turnidge, 2015/09/04 18:05:29]

I updated this comment.  It turns out that, given the two-pass nature of the
algorithm, my attempts at a short explanation were lossy, so I put in a pretty
large comment.

 intptr_t Debugger::ResolveBreakpointPos(const Function& func,
                                         intptr_t requested_token_pos,
-                                        intptr_t last_token_pos) {
+                                        intptr_t last_token_pos,
+                                        intptr_t requested_column) {
   ASSERT(func.HasCode());
   ASSERT(!func.HasOptimizedCode());
 
@@ -1619,6 +1637,7 @@ intptr_t Debugger::ResolveBreakpointPos(const Function& func,
     last_token_pos = func.end_token_pos();
   }
 
+  Script& script = Script::Handle(func.script());
   Code& code = Code::Handle(func.unoptimized_code());
   ASSERT(!code.IsNull());
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
@@ -1626,16 +1645,38 @@ intptr_t Debugger::ResolveBreakpointPos(const Function& func,
   // First pass: find the safe point which is closest to the beginning
   // of the given token range.
   intptr_t best_fit_pos = INT_MAX;
+  intptr_t best_column = INT_MAX;
   PcDescriptors::Iterator iter(desc, kSafepointKind);
   while (iter.MoveNext()) {
-    const intptr_t desc_token_pos = iter.TokenPos();
-    if ((desc_token_pos != Scanner::kNoSourcePos) &&
-        (desc_token_pos < best_fit_pos) &&
-        (desc_token_pos >= requested_token_pos) &&
-        (desc_token_pos <= last_token_pos)) {
-       best_fit_pos = desc_token_pos;
+    const intptr_t pos = iter.TokenPos();
+    if ((pos == Scanner::kNoSourcePos) ||
+        (pos < requested_token_pos) ||
+        (pos > last_token_pos)) {
+      // Token is not in the target range.
+      continue;
+    }
+
+    intptr_t token_start_column = -1;
+    if (requested_column >= 0) {

[hausner, 2015/09/03 00:37:50]

Do I understand this code right:

If the caller requests a column that is past the last safe point in the line,
then you will look for the first token in the next line.

Shouldn't the breakpoint then resolve to the closest column *before* the
requested one (if one exists)?

[turnidge, 2015/09/04 18:05:29]

There is a kind of logic to the current implementation -- if you ask for a
breakpoint after the last safepoint token, you are asking for a breakpoint past
the current line.

For this line:

  var x = function(y);  // long coment here

If they specify a breakpoint in the comment, then they are stopping after the
code has executed, not before.  If they specify a breakpoint at column 1000000,
they are stopping after the code has executed.

This is also kind of consistent with the current behavior of when there is no
safepoint on the current line -- when we can't satisfy the constraints, search
forward.

I can see that this might confuse people down the path, but my preference is to
keep it as-is and change it if they complain.  Is that ok?

+      intptr_t ignored = -1;
+      intptr_t token_len = -1;
+      script.GetTokenLocation(pos, &ignored, &token_start_column, &token_len);

[hausner, 2015/09/03 00:37:50]

GetTokenLocation is a really expensive operation, especially when the column
position is requested. The source text has to be rescanned. And if we don't have
the source, it will be reconstructed from the token stream.

That said, I don't know how to avoid this other than caching token info details
for a function once a breakpoint is requested.

[turnidge, 2015/09/04 18:05:29]

I agree.  I have added a TODO/comment about this.  Perhaps I should implement a
caching approach in a follow-up CL?

+      intptr_t token_end_column = token_start_column + token_len - 1;
+      if ((token_end_column < requested_column) ||
+          (token_start_column > best_column)) {
+        // Prefer the token with the lowest column number over the
+        // requested column.
+        continue;
+      }
+    }
+
+    // Prefer the lowest (first) token pos.
+    if (pos < best_fit_pos) {
+      best_fit_pos = pos;
+      best_column = token_start_column;
     }
   }
+
   // Second pass (if we found a safe point in the first pass):
   // For all token positions on the same line, select the one
   // with the lowest compiled code address. E.g., in a line with
@@ -1651,9 +1692,24 @@ intptr_t Debugger::ResolveBreakpointPos(const Function& func,
     PcDescriptors::Iterator iter(desc, kSafepointKind);
     while (iter.MoveNext()) {
       const intptr_t pos = iter.TokenPos();
-      if ((pos != Scanner::kNoSourcePos) &&
-          (begin_pos <= pos) && (pos <= end_of_line_pos) &&
-          (iter.PcOffset() < lowest_pc_offset)) {
+      if ((pos == Scanner::kNoSourcePos) ||
+          (pos < begin_pos) ||
+          (pos > end_of_line_pos)) {
+        // Token is not on same line as best fit.
+        continue;
+      }
+
+      if (requested_column >= 0) {
+        intptr_t ignored = -1;
+        intptr_t token_start_column = -1;
+        script.GetTokenLocation(pos, &ignored, &token_start_column);
+        if (token_start_column != best_column) {

[hausner, 2015/09/03 00:37:50]

Maybe comment that there may be more than one token PC descriptor at the same
token position (e.g. for implicitly generated code).

[turnidge, 2015/09/04 18:05:29]

Done.

+          continue;
+        }
+      }
+
+      // Prefer the lowest pc offset.
+      if (iter.PcOffset() < lowest_pc_offset) {
         lowest_pc_offset = iter.PcOffset();
         best_fit_pos = pos;
       }
@@ -1661,10 +1717,13 @@ intptr_t Debugger::ResolveBreakpointPos(const Function& func,
     return best_fit_pos;
   }
 
-  // We didn't find a safe point in the given token range. Try and find
-  // a safe point in the remaining source code of the function.
+  // We didn't find a safe point in the given token range. Try and
+  // find a safe point in the remaining source code of the function.
+  // Since we have moved to the next line of the function, we no
+  // longer are requesting a specific column number.
   if (last_token_pos < func.end_token_pos()) {
-    return ResolveBreakpointPos(func, last_token_pos, func.end_token_pos());
+    return ResolveBreakpointPos(func, last_token_pos, func.end_token_pos(),
+                                -1 /* no column */);
   }
   return -1;
 }
@@ -1857,7 +1916,8 @@ RawFunction* Debugger::FindBestFit(const Script& script,
 
 BreakpointLocation* Debugger::SetBreakpoint(const Script& script,
                                             intptr_t token_pos,
-                                            intptr_t last_token_pos) {
+                                            intptr_t last_token_pos,
+                                            intptr_t requested_column) {
   Function& func = Function::Handle(isolate_);
   func = FindBestFit(script, token_pos);
   if (func.IsNull()) {
@@ -1881,14 +1941,16 @@ BreakpointLocation* Debugger::SetBreakpoint(const Script& script,
     DeoptimizeWorld();
     func ^= functions.At(0);
     intptr_t breakpoint_pos =
-        ResolveBreakpointPos(func, token_pos, last_token_pos);
+        ResolveBreakpointPos(func, token_pos, last_token_pos, requested_column);
     if (breakpoint_pos >= 0) {
-      BreakpointLocation* bpt = GetBreakpointLocation(script, breakpoint_pos);
+      BreakpointLocation* bpt =
+          GetBreakpointLocation(script, breakpoint_pos, requested_column);
       if (bpt != NULL) {
         // A source breakpoint for this location already exists.
         return bpt;
       }
-      bpt = new BreakpointLocation(script, token_pos, last_token_pos);
+      bpt = new BreakpointLocation(script, token_pos, last_token_pos,
+                                   requested_column);
       bpt->SetResolved(func, breakpoint_pos);
       RegisterBreakpointLocation(bpt);
 
@@ -1901,11 +1963,12 @@ BreakpointLocation* Debugger::SetBreakpoint(const Script& script,
       }
       if (FLAG_verbose_debug) {
         intptr_t line_number;
-        script.GetTokenLocation(breakpoint_pos, &line_number, NULL);
+        intptr_t column_number;
+        script.GetTokenLocation(breakpoint_pos, &line_number, &column_number);
         OS::Print("Resolved BP for "
-                  "function '%s' at line %" Pd "\n",
+                  "function '%s' at line %" Pd " col %" Pd "\n",
                   func.ToFullyQualifiedCString(),
-                  line_number);
+                  line_number, column_number);
       }
       return bpt;
     }
@@ -1914,15 +1977,18 @@ BreakpointLocation* Debugger::SetBreakpoint(const Script& script,
   // Register an unresolved breakpoint.
   if (FLAG_verbose_debug && !func.IsNull()) {
     intptr_t line_number;
-    script.GetTokenLocation(token_pos, &line_number, NULL);
+    intptr_t column_number;
+    script.GetTokenLocation(token_pos, &line_number, &column_number);
     OS::Print("Registering pending breakpoint for "
-              "uncompiled function '%s' at line %" Pd "\n",
+              "uncompiled function '%s' at line %" Pd " col %" Pd "\n",
               func.ToFullyQualifiedCString(),
-              line_number);
+              line_number, column_number);
   }
-  BreakpointLocation* bpt = GetBreakpointLocation(script, token_pos);
+  BreakpointLocation* bpt =
+      GetBreakpointLocation(script, token_pos, requested_column);
   if (bpt == NULL) {
-    bpt = new BreakpointLocation(script, token_pos, last_token_pos);
+    bpt = new BreakpointLocation(script, token_pos, last_token_pos,
+                                 requested_column);
     RegisterBreakpointLocation(bpt);
   }
   return bpt;
@@ -1969,7 +2035,8 @@ Breakpoint* Debugger::SetBreakpointAtEntry(const Function& target_function,
   BreakpointLocation* bpt_location =
       SetBreakpoint(script,
                     target_function.token_pos(),
-                    target_function.end_token_pos());
+                    target_function.end_token_pos(),
+                    -1 /* no column */);
   if (single_shot) {
     return bpt_location->AddSingleShot(this);
   } else {
@@ -1986,7 +2053,8 @@ Breakpoint* Debugger::SetBreakpointAtActivation(const Instance& closure) {
   const Script& script = Script::Handle(func.script());
   BreakpointLocation* bpt_location = SetBreakpoint(script,
                                                    func.token_pos(),
-                                                   func.end_token_pos());
+                                                   func.end_token_pos(),
+                                                   -1 /* no column */);
   return bpt_location->AddPerClosure(this, closure);
 }
 
@@ -2016,7 +2084,21 @@ Breakpoint* Debugger::BreakpointAtActivation(const Instance& closure) {
 
 Breakpoint* Debugger::SetBreakpointAtLine(const String& script_url,
                                           intptr_t line_number) {
-  BreakpointLocation* loc = BreakpointLocationAtLine(script_url, line_number);
+  BreakpointLocation* loc =
+      BreakpointLocationAtLineCol(script_url, line_number, -1 /* no column */);
+  if (loc != NULL) {
+    return loc->AddRepeated(this);
+  }
+  return NULL;
+}
+
+
+Breakpoint* Debugger::SetBreakpointAtLineCol(const String& script_url,
+                                             intptr_t line_number,
+                                             intptr_t column_number) {
+  BreakpointLocation* loc = BreakpointLocationAtLineCol(script_url,
+                                                        line_number,
+                                                        column_number);
   if (loc != NULL) {
     return loc->AddRepeated(this);
   }
@@ -2024,8 +2106,10 @@ Breakpoint* Debugger::SetBreakpointAtLine(const String& script_url,
 }
 
 
-BreakpointLocation* Debugger::BreakpointLocationAtLine(const String& script_url,
-                                                       intptr_t line_number) {
+BreakpointLocation* Debugger::BreakpointLocationAtLineCol(
+    const String& script_url,
+    intptr_t line_number,
+    intptr_t column_number) {
   Library& lib = Library::Handle(isolate_);
   Script& script = Script::Handle(isolate_);
   const GrowableObjectArray& libs =
@@ -2043,11 +2127,13 @@ BreakpointLocation* Debugger::BreakpointLocationAtLine(const String& script_url,
     // No script found with given url. Create a latent breakpoint which
     // will be set if the url is loaded later.
     BreakpointLocation* latent_bpt = GetLatentBreakpoint(script_url,
-                                                         line_number);
+                                                         line_number,
+                                                         column_number);
     if (FLAG_verbose_debug) {
-      OS::Print("Set latent breakpoint in url '%s' at line %" Pd "\n",
+      OS::Print("Set latent breakpoint in url '%s' at "
+                "line %" Pd " col %" Pd "\n",
                 script_url.ToCString(),
-                line_number);
+                line_number, column_number);
     }
     return latent_bpt;
   }
@@ -2079,7 +2165,7 @@ BreakpointLocation* Debugger::BreakpointLocationAtLine(const String& script_url,
   BreakpointLocation* bpt = NULL;
   ASSERT(first_token_idx <= last_token_idx);
   while ((bpt == NULL) && (first_token_idx <= last_token_idx)) {
-    bpt = SetBreakpoint(script, first_token_idx, last_token_idx);
+    bpt = SetBreakpoint(script, first_token_idx, last_token_idx, column_number);
     first_token_idx++;
   }
   if ((bpt == NULL) && FLAG_verbose_debug) {
@@ -2680,7 +2766,8 @@ void Debugger::NotifyCompilation(const Function& func) {
       if (!loc->IsResolved()) {
         // Resolve source breakpoint in the newly compiled function.
         intptr_t bp_pos =
-            ResolveBreakpointPos(func, loc->token_pos(), loc->end_token_pos());
+            ResolveBreakpointPos(func, loc->token_pos(), loc->end_token_pos(),
+                                 loc->requested_column_number());
         if (bp_pos < 0) {
           if (FLAG_verbose_debug) {
             OS::Print("Failed resolving breakpoint for function '%s'\n",
@@ -2694,14 +2781,18 @@ void Debugger::NotifyCompilation(const Function& func) {
         Breakpoint* bpt = loc->breakpoints();
         while (bpt != NULL) {
           if (FLAG_verbose_debug) {
-            OS::Print("Resolved BP %" Pd " to pos %" Pd ", line %" Pd ", "
-                      "function '%s' (requested range %" Pd "-%" Pd ")\n",
+            OS::Print("Resolved BP %" Pd " to pos %" Pd ", "
+                      "line %" Pd " col %" Pd ", "
+                      "function '%s' (requested range %" Pd "-%" Pd ", "
+                      "requested col %" Pd ")\n",
                       bpt->id(),
                       loc->token_pos(),
                       loc->LineNumber(),
+                      loc->ColumnNumber(),
                       func.ToFullyQualifiedCString(),
                       requested_pos,
-                      requested_end_pos);
+                      requested_end_pos,
+                      loc->requested_column_number());
           }
           SignalBpResolved(bpt);
           SendServiceBreakpointEvent(ServiceEvent::kBreakpointResolved, bpt);
@@ -2712,9 +2803,11 @@ void Debugger::NotifyCompilation(const Function& func) {
       if (FLAG_verbose_debug) {
         Breakpoint* bpt = loc->breakpoints();
         while (bpt != NULL) {
-          OS::Print("Setting breakpoint %" Pd " at line %" Pd " for %s '%s'\n",
+          OS::Print("Setting breakpoint %" Pd " at line %" Pd " col %" Pd ""
+                    " for %s '%s'\n",
                     bpt->id(),
                     loc->LineNumber(),
+                    loc->ColumnNumber(),
                     func.IsClosureFunction() ? "closure" : "function",
                     String::Handle(func.name()).ToCString());
           bpt = bpt->next();
@@ -2757,7 +2850,8 @@ void Debugger::NotifyDoneLoading() {
         }
         // Now find the token range at the requested line and make a
         // new unresolved source breakpoint.
-        intptr_t line_number = matched_loc->LineNumber();
+        intptr_t line_number = matched_loc->requested_line_number();
+        intptr_t column_number = matched_loc->requested_column_number();
         ASSERT(line_number >= 0);
         intptr_t first_token_pos, last_token_pos;
         script.TokenRangeAtLine(line_number, &first_token_pos, &last_token_pos);
@@ -2784,7 +2878,7 @@ void Debugger::NotifyDoneLoading() {
           // If there is one, assert in debug build but silently drop
           // the latent breakpoint in release build.
           BreakpointLocation* existing_loc =
-              GetBreakpointLocation(script, first_token_pos);
+              GetBreakpointLocation(script, first_token_pos, column_number);
           ASSERT(existing_loc == NULL);
           if (existing_loc == NULL) {
             // Create and register a new source breakpoint for the
@@ -2792,7 +2886,8 @@ void Debugger::NotifyDoneLoading() {
             BreakpointLocation* unresolved_loc =
                 new BreakpointLocation(script,
                                        first_token_pos,
-                                       last_token_pos);
+                                       last_token_pos,
+                                       column_number);
             RegisterBreakpointLocation(unresolved_loc);
 
             // Move breakpoints over.
@@ -2803,10 +2898,10 @@ void Debugger::NotifyDoneLoading() {
               bpt->set_bpt_location(unresolved_loc);
               if (FLAG_verbose_debug) {
                 OS::Print("Converted latent breakpoint "
-                          "%" Pd " in '%s' at line %" Pd "\n",
+                          "%" Pd " in '%s' at line %" Pd " col %" Pd "\n",
                           bpt->id(),
                           url.ToCString(),
-                          line_number);
+                          line_number, column_number);
               }
               bpt = bpt->next();
             }
@@ -2970,10 +3065,13 @@ void Debugger::RemoveInternalBreakpoints() {
 
 
 BreakpointLocation* Debugger::GetBreakpointLocation(const Script& script,
-                                                    intptr_t token_pos) {
+                                                    intptr_t token_pos,
+                                                    intptr_t requested_column) {
   BreakpointLocation* bpt = breakpoint_locations_;
   while (bpt != NULL) {
-    if ((bpt->script_ == script.raw()) && (bpt->token_pos_ == token_pos)) {
+    if ((bpt->script_ == script.raw()) &&
+        (bpt->token_pos_ == token_pos) &&
+        (bpt->requested_column_number_ == requested_column)) {
       return bpt;
     }
     bpt = bpt->next();
@@ -2999,18 +3097,21 @@ Breakpoint* Debugger::GetBreakpointById(intptr_t id) {
 
 
 BreakpointLocation* Debugger::GetLatentBreakpoint(const String& url,
-                                                  intptr_t line) {
+                                                  intptr_t line,
+                                                  intptr_t column) {
   BreakpointLocation* bpt = latent_locations_;
   String& bpt_url = String::Handle(isolate_);
   while (bpt != NULL) {
     bpt_url = bpt->url();
-    if (bpt_url.Equals(url) && (bpt->LineNumber() == line)) {
+    if (bpt_url.Equals(url) &&
+        (bpt->requested_line_number() == line) &&
+        (bpt->requested_column_number() == column)) {
       return bpt;
     }
     bpt = bpt->next();
   }
-  // No breakpint for this url and line requested. Allocate new one.
-  bpt = new BreakpointLocation(url, line);
+  // No breakpoint for this location requested. Allocate new one.
+  bpt = new BreakpointLocation(url, line, column);
   bpt->set_next(latent_locations_);
   latent_locations_ = bpt;
   return bpt;