Support column-based breakpoints in the VM and Observatory.

Support column-based breakpoints in the VM and Observatory.

BUG=
R=hausner@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/dee6ddf966e38901f425c95d4fa18a7e40f9d068

[turnidge, 2015-09-02 22:12:58]

turnidge@google.com changed reviewers:
+ hausner@google.com, rmacnak@google.com

[turnidge, 2015-09-02 22:12:58]

Matthias - can you review the debugger changes?

Ryan - can you take a look at everything?

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

Some comments covering the debugger changes.

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (left):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:122: line_number_ = -1;  // Recalculate lazily.
Why is it not necessary to recalculate the line number once we know the
definitive location of the breakpoint?

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:102: ASSERT(line_number_ >= 0);
This assert would fire right away, since the field is initialized to -1. 

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

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1625: // that line, the safe point with the lowest
compiled code address.
Update the comment to describe how the requested column is taken into account?

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1660: if (requested_column >= 0) {
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)?

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1663: script.GetTokenLocation(pos, &ignored,
&token_start_column, &token_len);
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.

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1684: // compiled code address but is not the lowest
token position in the
Could you please update the comment to include the column number explanation?

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1706: if (token_start_column != best_column) {
Maybe comment that there may be more than one token PC descriptor at the same
token position (e.g. for implicitly generated code).

[rmacnak, 2015-09-03 00:55:19]

https://codereview.chromium.org/1312763010/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/debugger.dart:747: int bptFound = 0;
btpsFound = 0 or bptFound = false

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8622: *token_len = 1;
Maybe otherwise keyword length?

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/service/serv...
File runtime/vm/service/service.md (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/service/serv...
runtime/vm/service/service.md:377: int column)
int column [optional]

[turnidge, 2015-09-04 18:05:30]

I have added tests for latent breakpoints.

I have added protocol support for a new type: UnresolvedSourceLocation.

I have done some UI tweaks.

Ready for another look.

https://codereview.chromium.org/1312763010/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/debugger.dart:747: int bptFound = 0;
On 2015/09/03 00:55:19, rmacnak wrote:
> btpsFound = 0 or bptFound = false

Done.

var foundBreakpoint = false;

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (left):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:122: line_number_ = -1;  // Recalculate lazily.
On 2015/09/03 00:37:50, hausner wrote:
> Why is it not necessary to recalculate the line number once we know the
> definitive location of the breakpoint?

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.

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:102: ASSERT(line_number_ >= 0);
On 2015/09/03 00:37:50, hausner wrote:
> This assert would fire right away, since the field is initialized to -1. 
> 
> It looks like this constructor is not executed in our tests.

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

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1625: // that line, the safe point with the lowest
compiled code address.
On 2015/09/03 00:37:50, hausner wrote:
> Update the comment to describe how the requested column is taken into account?

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.

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1660: if (requested_column >= 0) {
On 2015/09/03 00:37:50, hausner wrote:
> 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)?

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?

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1663: script.GetTokenLocation(pos, &ignored,
&token_start_column, &token_len);
On 2015/09/03 00:37:50, hausner wrote:
> 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.

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

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1684: // compiled code address but is not the lowest
token position in the
On 2015/09/03 00:37:50, hausner wrote:
> Could you please update the comment to include the column number explanation?

Done.

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1706: if (token_start_column != best_column) {
On 2015/09/03 00:37:50, hausner wrote:
> Maybe comment that there may be more than one token PC descriptor at the same
> token position (e.g. for implicitly generated code).

Done.

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8622: *token_len = 1;
On 2015/09/03 00:55:19, rmacnak wrote:
> Maybe otherwise keyword length?

My hope was that the literal pointed to the keyword but perhaps this was wrong. 
(See scanner.cc:319)

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/service/serv...
File runtime/vm/service/service.md (right):

https://codereview.chromium.org/1312763010/diff/20001/runtime/vm/service/serv...
runtime/vm/service/service.md:377: int column)
On 2015/09/03 00:55:19, rmacnak wrote:
> int column [optional]

Done.

[hausner, 2015-09-04 22:45:16]

Only one concern remains: before this change, LineNumber() could be called on an
unresolved breakpoint and it returned the requested line. This change, as it is,
breaks the embedder API in a subtle way.

Otherwise debugger changes LGTM.

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:161: ASSERT(IsResolved());
So far, LineNumber() could be invoked before the breakpoint is resolved, and
you'd get the requested line number. After resolution, it returns the actual
line number. The API function Dart_GetBreakpointLine() calls this function
directly, so the program would die (or compute a nonsense line number) when
called on an unresolved breakpoint.

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:165: script.GetTokenLocation(token_pos_, &line_number_,
NULL);
Wouldn't it make sense to calculate the column number pro-actively here? OTOH,
calculating the column number is significantly more expensive than just the line
number, so maybe your approach of only determining it when asked for is good.

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:1656: //   - is a safepoint,
nit: this is implied by the fact that you only look at positions found in the
first pass.

[turnidge, 2015-09-08 18:41:02]

Ok, waiting on approval from rmacnak.

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:161: ASSERT(IsResolved());
On 2015/09/04 22:45:16, hausner wrote:
> So far, LineNumber() could be invoked before the breakpoint is resolved, and
> you'd get the requested line number. After resolution, it returns the actual
> line number. The API function Dart_GetBreakpointLine() calls this function
> directly, so the program would die (or compute a nonsense line number) when
> called on an unresolved breakpoint.

Thanks.  I have fixed Dart_GetBreakpointLine().

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:165: script.GetTokenLocation(token_pos_, &line_number_,
NULL);
On 2015/09/04 22:45:16, hausner wrote:
> Wouldn't it make sense to calculate the column number pro-actively here? OTOH,
> calculating the column number is significantly more expensive than just the
line
> number, so maybe your approach of only determining it when asked for is good.

Yes, I debated this too.  Right now all of the calls to ColumnNumber occur when
printing verbose debugging output, so I decided it was worth avoiding the extra
cost in this case for the time being.  Not sure though.

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:1656: //   - is a safepoint,
On 2015/09/04 22:45:16, hausner wrote:
> nit: this is implied by the fact that you only look at positions found in the
> first pass.

I'm not sure I really get the nit.  The comment does not make any claim about
only looking at the same positions as the first pass, so not sure how this is
implied.

[hausner, 2015-09-08 19:38:34]

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1312763010/diff/100001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:1656: //   - is a safepoint,
On 2015/09/08 18:41:02, turnidge wrote:
> On 2015/09/04 22:45:16, hausner wrote:
> > nit: this is implied by the fact that you only look at positions found in
the
> > first pass.
> 
> I'm not sure I really get the nit.  The comment does not make any claim about
> only looking at the same positions as the first pass, so not sure how this is
> implied.

Yes, you are right. I falsely thought the second pass only looks at the
positions found in the first pass.

[rmacnak, 2015-09-09 01:45:22]

lgtm

[turnidge, 2015-09-09 16:01:16]

Committed patchset #7 (id:120001) manually as
dee6ddf966e38901f425c95d4fa18a7e40f9d068 (presubmit successful).