Set breakpoint at url, line number

Set breakpoint at url, line number

Add debugger functionality to set breakpoint at given url and line number.
Committed: https://code.google.com/p/dart/source/detail?r=3420

[hausner, 2012-01-17 22:58:46]

Adding Anton and Pavel to cc list.

[siva, 2012-01-18 01:32:03]

LGTM with some comments.

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode363
runtime/bin/main.cc:363: Dart_Handle library) {
indentation?

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode372
runtime/bin/main.cc:372: url = Dart_NewString(bpt_line);
Dart_Handle url = ....;

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode373
runtime/bin/main.cc:373: line_number = Dart_NewInteger(atoi(colon + 1));
Dart_Handle line_number = ...;

Also we may want to protect against input strings of the form "myurl:"

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode388
runtime/bin/main.cc:388: function_name = Dart_NewString(dot + 1);
We may want to protect against input strings of the form "myclass."

http://codereview.chromium.org/9240014/diff/1/runtime/include/dart_debugger_a...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9240014/diff/1/runtime/include/dart_debugger_a...
runtime/include/dart_debugger_api.h:29: *
Will it point to the existing breakpoint if a breakpoint already exists at the
specified script_url and line number?

http://codereview.chromium.org/9240014/diff/1/runtime/include/dart_debugger_a...
runtime/include/dart_debugger_api.h:46: *   if a breakpoint was successfully
created.
Ditto comment regarding breakpoint already existing?

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc#newcode283
runtime/vm/debugger.cc:283: }
Shouldn't we check in the breakpoint list that you have to see if a breakpoint
already exists at this function/token_index?

Maybe a TODO here if you want to do this in another CL.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc#newcode293
runtime/vm/debugger.cc:293: }
If 'i' happens to be the last descriptor in this function does it make sense to
set a breakpoint at this index as we won't find anymore descriptors and will end
up not setting a breakpoint.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc#newcode317
runtime/vm/debugger.cc:317: }
why did you drop "return NULL"; ?

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger_api_impl.cc
File runtime/vm/debugger_api_impl.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger_api_impl.cc...
runtime/vm/debugger_api_impl.cc:138: Debugger* debugger = isolate->debugger();
ASSERT(debugger != NULL);

http://codereview.chromium.org/9240014/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/object.cc#newcode1596
runtime/vm/object.cc:1596: (token_index < func.end_token_index())) {
Maybe function could have a method
bool containsTokenIndex(intptr_t token_index);

as the same condition is repeated in LookupFUnctionInScript.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/object.cc#newcode4065
runtime/vm/object.cc:4065: }
It might be worthwhile to add an unit test for this function.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/raw_object.h#newcode494
runtime/vm/raw_object.h:494: intptr_t end_token_index_;
This new field needs to be written out and read in raw_object_snapshot.cc

[hausner, 2012-01-18 23:26:38]

Thanks for the review. Additional unit tests will follow in subsequent checkins.

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode363
runtime/bin/main.cc:363: Dart_Handle library) {
On 2012/01/18 01:32:03, asiva wrote:
> indentation?

Done.

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode372
runtime/bin/main.cc:372: url = Dart_NewString(bpt_line);
On 2012/01/18 01:32:03, asiva wrote:
> Dart_Handle url = ....;

Done.

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode373
runtime/bin/main.cc:373: line_number = Dart_NewInteger(atoi(colon + 1));
On 2012/01/18 01:32:03, asiva wrote:
> Dart_Handle line_number = ...;
> 
> Also we may want to protect against input strings of the form "myurl:"

This is really just a make-shift UI for me to test the debugger functions, so I
don't think i need all the error handling of an end-user feature. Agree?

http://codereview.chromium.org/9240014/diff/1/runtime/bin/main.cc#newcode388
runtime/bin/main.cc:388: function_name = Dart_NewString(dot + 1);
On 2012/01/18 01:32:03, asiva wrote:
> We may want to protect against input strings of the form "myclass."

Ditto

http://codereview.chromium.org/9240014/diff/1/runtime/include/dart_debugger_a...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9240014/diff/1/runtime/include/dart_debugger_a...
runtime/include/dart_debugger_api.h:29: *
On 2012/01/18 01:32:03, asiva wrote:
> Will it point to the existing breakpoint if a breakpoint already exists at the
> specified script_url and line number?

Good question. So far, there are no such checks but I'll have to add them.

http://codereview.chromium.org/9240014/diff/1/runtime/include/dart_debugger_a...
runtime/include/dart_debugger_api.h:46: *   if a breakpoint was successfully
created.
On 2012/01/18 01:32:03, asiva wrote:
> Ditto comment regarding breakpoint already existing?
Ditto answer.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc#newcode283
runtime/vm/debugger.cc:283: }
On 2012/01/18 01:32:03, asiva wrote:
> Shouldn't we check in the breakpoint list that you have to see if a breakpoint
> already exists at this function/token_index?
> 
> Maybe a TODO here if you want to do this in another CL.

Done

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc#newcode293
runtime/vm/debugger.cc:293: }
On 2012/01/18 01:32:03, asiva wrote:
> If 'i' happens to be the last descriptor in this function does it make sense
to
> set a breakpoint at this index as we won't find anymore descriptors and will
end
> up not setting a breakpoint.
We return NULL if there is no possible PC location for a breakpoint after the
requested breakpoint location. If the requested breakpoint location is
(textually) outside the function, we don't even get here in the first place.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger.cc#newcode317
runtime/vm/debugger.cc:317: }
On 2012/01/18 01:32:03, asiva wrote:
> why did you drop "return NULL"; ?
Accident. I need automated test cases sooner than later. Thanks!

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger_api_impl.cc
File runtime/vm/debugger_api_impl.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/debugger_api_impl.cc...
runtime/vm/debugger_api_impl.cc:138: Debugger* debugger = isolate->debugger();
On 2012/01/18 01:32:03, asiva wrote:
> ASSERT(debugger != NULL);

Done.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/object.cc#newcode1596
runtime/vm/object.cc:1596: (token_index < func.end_token_index())) {
On 2012/01/18 01:32:03, asiva wrote:
> Maybe function could have a method
> bool containsTokenIndex(intptr_t token_index);
> 
> as the same condition is repeated in LookupFUnctionInScript.

Yes, but I don't think the code becomes more readable if we make too many
one-liner functions. If you feel strongly about it, I can add the function,
otherwise I prefer to leave it out.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/object.cc#newcode4065
runtime/vm/object.cc:4065: }
On 2012/01/18 01:32:03, asiva wrote:
> It might be worthwhile to add an unit test for this function.

Will do in a later change. I gave it a try but ran into asserts because the unit
test code apparently does not set up libraries exactly right. I'll look at that
later.

http://codereview.chromium.org/9240014/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

http://codereview.chromium.org/9240014/diff/1/runtime/vm/raw_object.h#newcode494
runtime/vm/raw_object.h:494: intptr_t end_token_index_;
On 2012/01/18 01:32:03, asiva wrote:
> This new field needs to be written out and read in raw_object_snapshot.cc
Good catch. Done.