First bits of external debugger API

First bits of external debugger API

The debugger API is in a separate set of files. Embedders do not have
to include the debug api if they don't need to.
Committed: https://code.google.com/p/dart/source/detail?r=2198

[siva, 2011-12-07 05:44:48]

LGTM once comments are addressed.

http://codereview.chromium.org/8826007/diff/1004/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/8826007/diff/1004/runtime/bin/bin.gypi#newcode90
runtime/bin/bin.gypi:90: '../include/dart_dapi.h',
dapi seems so close to api that it might be mistaken for dart API, would ddi
(dart debug interface) and dpi (dart profiler interface) be better names for
these files?
(the name dart_papi.h sounds very weird).

Just my view you don't have to change it.

http://codereview.chromium.org/8826007/diff/5004/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/bin/bin.gypi#newcode90
runtime/bin/bin.gypi:90: '../include/dart_dapi.h',
would dart_ddi.h (ddi for dart debug interface) and dart_dpi.h (dart profiler
interface) be better. I am wondering about dart_papi.h sounds like a baseball
player, that too a red sox guy!!!!!

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

http://codereview.chromium.org/8826007/diff/5004/runtime/bin/main.cc#newcode388
runtime/bin/main.cc:388: }
I am assuming this a temporary setup, long term it might make more sense to have
a different embedder for the debugger shell which accepts commands and invokes
the appropriate interface function.

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h
File runtime/include/dart_dapi.h (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h#...
runtime/include/dart_dapi.h:8: 
extra blank line?

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h#...
runtime/include/dart_dapi.h:14: DART_EXPORT void Dart_InitDebugger();
Is Dart_InitDebugger guaranteed not to fail?

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h#...
runtime/include/dart_dapi.h:20: Dart_Breakpoint* breakpoint);
Todd has been adding some documentation on top of each of these API functions,
see dart_api.h.
It might be useful to continue that in this file too.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc
File runtime/vm/dart_dapi_impl.cc (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:14: namespace dart {
extra blank line?

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:30: DARTSCOPE(isolate);
In the dart API functions we normally check for valid input and return errors.
In this case we should check and return an error if breakpoint == NULL.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:37: }
Should the isolate state check happen after validating the input params library,
class, function etc.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:42: return Api::Error("No library specified");
The wording of error messages are not consistent with the ones in dart_api.cc,
again Todd has been trying to make the error messages uniform and more
descriptive. In this case it would be:

"%s expects argument 'library' to be non-null", CURRENT_FUNC

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:45:
String::CheckedHandle(Api::UnwrapHandle(class_name_in));
Need similar null checks on class_name_in parameter.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:47:
String::CheckedHandle(Api::UnwrapHandle(function_name_in));
Ditto on check.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:54: return Api::Error("Breakpoint target function
does not exist");
I guess the error message needs to be more descriptive....

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:62: Breakpoint* bpt =
isolate->debugger()->SetBreakpointAtEntry(bp_target);
We seem to be using isolate->debugger() a lot in this function maybe we could
have a local variable debugger.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:63: if (breakpoint != NULL) {
breakpoint will be non NULL here if we error out earlier.

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

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:188: return breakpoints_ != NULL;
If you start deleting breakpoints then this condition will not be true once a
breakpoint is hit and you delete it. Would that be an issue?

[hausner, 2011-12-07 22:14:26]

Thanks!

http://codereview.chromium.org/8826007/diff/5004/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/bin/bin.gypi#newcode90
runtime/bin/bin.gypi:90: '../include/dart_dapi.h',
On 2011/12/07 05:44:48, asiva wrote:
> would dart_ddi.h (ddi for dart debug interface) and dart_dpi.h (dart profiler
> interface) be better. I am wondering about dart_papi.h sounds like a baseball
> player, that too a red sox guy!!!!!

Agree. Renamed as discussed in person.

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

http://codereview.chromium.org/8826007/diff/5004/runtime/bin/main.cc#newcode388
runtime/bin/main.cc:388: }
On 2011/12/07 05:44:48, asiva wrote:
> I am assuming this a temporary setup, long term it might make more sense to
have
> a different embedder for the debugger shell which accepts commands and invokes
> the appropriate interface function.

Yes.

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h
File runtime/include/dart_dapi.h (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h#...
runtime/include/dart_dapi.h:8: 
On 2011/12/07 05:44:48, asiva wrote:
> extra blank line?

Done.

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h#...
runtime/include/dart_dapi.h:14: DART_EXPORT void Dart_InitDebugger();
On 2011/12/07 05:44:48, asiva wrote:
> Is Dart_InitDebugger guaranteed not to fail?

At this point, it always succeeds. But it makes sense to add a return value.
Done.

http://codereview.chromium.org/8826007/diff/5004/runtime/include/dart_dapi.h#...
runtime/include/dart_dapi.h:20: Dart_Breakpoint* breakpoint);
On 2011/12/07 05:44:48, asiva wrote:
> Todd has been adding some documentation on top of each of these API functions,
> see dart_api.h.
> It might be useful to continue that in this file too.

Added proper function comment.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc
File runtime/vm/dart_dapi_impl.cc (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:14: namespace dart {
On 2011/12/07 05:44:48, asiva wrote:
> extra blank line?

Done.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:30: DARTSCOPE(isolate);
On 2011/12/07 05:44:48, asiva wrote:
> In the dart API functions we normally check for valid input and return errors.
> In this case we should check and return an error if breakpoint == NULL.

The idea was to allow NULL as a valid value. The caller may not want to get a
pointer to the Breakpoint object.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:37: }
On 2011/12/07 05:44:48, asiva wrote:
> Should the isolate state check happen after validating the input params
library,
> class, function etc.
Yes, good point. Rearranged the order of things.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:42: return Api::Error("No library specified");
On 2011/12/07 05:44:48, asiva wrote:
> The wording of error messages are not consistent with the ones in dart_api.cc,
> again Todd has been trying to make the error messages uniform and more
> descriptive. In this case it would be:
> 
> "%s expects argument 'library' to be non-null", CURRENT_FUNC

Ok, adapted the scheme used in dart_api_impl.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:45:
String::CheckedHandle(Api::UnwrapHandle(class_name_in));
On 2011/12/07 05:44:48, asiva wrote:
> Need similar null checks on class_name_in parameter.

Done.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:47:
String::CheckedHandle(Api::UnwrapHandle(function_name_in));
On 2011/12/07 05:44:48, asiva wrote:
> Ditto on check.

Done.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:54: return Api::Error("Breakpoint target function
does not exist");
On 2011/12/07 05:44:48, asiva wrote:
> I guess the error message needs to be more descriptive....

How? You mean by including the function name?

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:62: Breakpoint* bpt =
isolate->debugger()->SetBreakpointAtEntry(bp_target);
On 2011/12/07 05:44:48, asiva wrote:
> We seem to be using isolate->debugger() a lot in this function maybe we could
> have a local variable debugger.

It's only used twice. Done.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:63: if (breakpoint != NULL) {
On 2011/12/07 05:44:48, asiva wrote:
> breakpoint will be non NULL here if we error out earlier.

*breakpoint is set to NULL early on. It only has a non-null value if we succeed
setting a breakpoint.

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

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:188: return breakpoints_ != NULL;
On 2011/12/07 05:44:48, asiva wrote:
> If you start deleting breakpoints then this condition will not be true once a
> breakpoint is hit and you delete it. Would that be an issue?

Yes, it would be an issue. Another solution (which is also not ideal) is to have
a sticky flag that remains set if the debugger has ever been used. Both
solutions don't work right if the VM runs for a while and produces optimized
code before the debugger becomes active. Clearly, this needs to be thought out
more. Added a TODO.

[siva, 2011-12-07 22:27:08]

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc
File runtime/vm/dart_dapi_impl.cc (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:30: DARTSCOPE(isolate);
Does that mean you will implement the delete API also in terms of
library/class/function name and not by passing in the Dart_Breakpoint object?

On 2011/12/07 22:14:26, hausner wrote:
> On 2011/12/07 05:44:48, asiva wrote:
> > In the dart API functions we normally check for valid input and return
errors.
> > In this case we should check and return an error if breakpoint == NULL.
> 
> The idea was to allow NULL as a valid value. The caller may not want to get a
> pointer to the Breakpoint object.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:54: return Api::Error("Breakpoint target function
does not exist");
Yes, the fully qualified function name (i.e library, class, function).

On 2011/12/07 22:14:26, hausner wrote:
> On 2011/12/07 05:44:48, asiva wrote:
> > I guess the error message needs to be more descriptive....
> 
> How? You mean by including the function name?

[hausner, 2011-12-07 22:36:49]

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc
File runtime/vm/dart_dapi_impl.cc (right):

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:30: DARTSCOPE(isolate);
On 2011/12/07 22:27:08, asiva wrote:
> Does that mean you will implement the delete API also in terms of
> library/class/function name and not by passing in the Dart_Breakpoint object?
> 
> On 2011/12/07 22:14:26, hausner wrote:
> > On 2011/12/07 05:44:48, asiva wrote:
> > > In the dart API functions we normally check for valid input and return
> errors.
> > > In this case we should check and return an error if breakpoint == NULL.
> > 
> > The idea was to allow NULL as a valid value. The caller may not want to get
a
> > pointer to the Breakpoint object.
> 

Hmm, good point. I'm pretty sure there will also be a way to enumerate
breakpoints at some time, but maybe it would make sense to require the parameter
to be non-null. I'll leave this for the next change.

http://codereview.chromium.org/8826007/diff/5004/runtime/vm/dart_dapi_impl.cc...
runtime/vm/dart_dapi_impl.cc:54: return Api::Error("Breakpoint target function
does not exist");
On 2011/12/07 22:27:08, asiva wrote:
> Yes, the fully qualified function name (i.e library, class, function).
> 
> On 2011/12/07 22:14:26, hausner wrote:
> > On 2011/12/07 05:44:48, asiva wrote:
> > > I guess the error message needs to be more descriptive....
> > 
> > How? You mean by including the function name?
> 

Ok, will do in the next CL.