Introduce remote objects in Debugger protocol

Introduce remote objects in Debugger protocol
Committed: https://code.google.com/p/dart/source/detail?r=7864

[siva, 2012-05-22 01:49:09]

LGTM

http://codereview.chromium.org/10407071/diff/11002/runtime/bin/dbg_connection.cc
File runtime/bin/dbg_connection.cc (right):

http://codereview.chromium.org/10407071/diff/11002/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:133: return atoi(r.ValueChars());
It might be safer to use strtol here as it checks for overflow and underflow

http://codereview.chromium.org/10407071/diff/11002/runtime/include/dart_debug...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/10407071/diff/11002/runtime/include/dart_debug...
runtime/include/dart_debugger_api.h:27: * Caches a given \object and returns an
object id. The cache is only
This object id is valid only while the VM is paused as the cache is destroyed
when execution is resumed.

http://codereview.chromium.org/10407071/diff/11002/runtime/include/dart_debug...
runtime/include/dart_debugger_api.h:306: * Returns in \class_id the class id of
the given \object.
Should you put in some wording that the class id is valid even after execution
resumes and is valid until the isolate is shutdown.

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

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger.cc#new...
runtime/vm/debugger.cc:30: RawObject* GetObj(intptr_t obj_id);
const?

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
File runtime/vm/debugger_api_impl.cc (right):

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
runtime/vm/debugger_api_impl.cc:51: return -1;
Since this is part of the API it might be better to have a named constant for
-1.

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
runtime/vm/debugger_api_impl.cc:393: UNWRAP_AND_CHECK_PARAM(Instance, obj,
object_in);
  if (class_id == NULL) {
    return Api::NewError("%s expects argument 'class_id' to be non-null.",
                         CURRENT_FUNC);
  }
  *class_id = Class::Handle(....);

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
runtime/vm/debugger_api_impl.cc:439: }
Ditto Api::NewError return values here for NULL pointer values....

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart
File tools/ddbg.dart (right):

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode36
tools/ddbg.dart:36: po <id> Print object
missing help for command 'pc' ?

Also noticed 'h' is not listed here.

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode150
tools/ddbg.dart:150: assert(fields != null);
Just curious, what is the point of this assert, woudn't we get a
NullPointerException anyway if it turns out to be null.

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode151
tools/ddbg.dart:151: assert(fields is List);
Also what is the style here? instead of adding our own type check asserts in
this case does it make sense to code as:
List fields = props["fields"];

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode235
tools/ddbg.dart:235: assert(locals is List);
Ditto question about explicit type check assert.

[hausner, 2012-05-22 18:03:57]

http://codereview.chromium.org/10407071/diff/11002/runtime/bin/dbg_connection.cc
File runtime/bin/dbg_connection.cc (right):

http://codereview.chromium.org/10407071/diff/11002/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:133: return atoi(r.ValueChars());
On 2012/05/22 01:49:09, asiva wrote:
> It might be safer to use strtol here as it checks for overflow and underflow

Done.

http://codereview.chromium.org/10407071/diff/11002/runtime/include/dart_debug...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/10407071/diff/11002/runtime/include/dart_debug...
runtime/include/dart_debugger_api.h:27: * Caches a given \object and returns an
object id. The cache is only
On 2012/05/22 01:49:09, asiva wrote:
> This object id is valid only while the VM is paused as the cache is destroyed
> when execution is resumed.

Done.

http://codereview.chromium.org/10407071/diff/11002/runtime/include/dart_debug...
runtime/include/dart_debugger_api.h:306: * Returns in \class_id the class id of
the given \object.
On 2012/05/22 01:49:09, asiva wrote:
> Should you put in some wording that the class id is valid even after execution
> resumes and is valid until the isolate is shutdown.

Done.

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

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger.cc#new...
runtime/vm/debugger.cc:30: RawObject* GetObj(intptr_t obj_id);
On 2012/05/22 01:49:09, asiva wrote:
> const?

Done.

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
File runtime/vm/debugger_api_impl.cc (right):

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
runtime/vm/debugger_api_impl.cc:51: return -1;
On 2012/05/22 01:49:09, asiva wrote:
> Since this is part of the API it might be better to have a named constant for
> -1.
I'd rather solve this by documenting that -1 is an error value, similar to file
handles in Unix/Posix.

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
runtime/vm/debugger_api_impl.cc:393: UNWRAP_AND_CHECK_PARAM(Instance, obj,
object_in);
On 2012/05/22 01:49:09, asiva wrote:
>   if (class_id == NULL) {
>     return Api::NewError("%s expects argument 'class_id' to be non-null.",
>                          CURRENT_FUNC);
>   }
>   *class_id = Class::Handle(....);

It's kind of non-sensical to call this function with a NULL class_id, but is it
an error? Done.

http://codereview.chromium.org/10407071/diff/11002/runtime/vm/debugger_api_im...
runtime/vm/debugger_api_impl.cc:439: }
Not in this case. I want the caller to be able to get only part of the info. For
example, retrieving the static field values is an expensive operation. If the
caller isn't interested in the fields, it saves time and memory when passing
NULL for that parameter.

On 2012/05/22 01:49:09, asiva wrote:
> Ditto Api::NewError return values here for NULL pointer values....

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart
File tools/ddbg.dart (right):

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode36
tools/ddbg.dart:36: po <id> Print object
On 2012/05/22 01:49:09, asiva wrote:
> missing help for command 'pc' ?
> 
> Also noticed 'h' is not listed here.

Done.

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode150
tools/ddbg.dart:150: assert(fields != null);
On 2012/05/22 01:49:09, asiva wrote:
> Just curious, what is the point of this assert, woudn't we get a
> NullPointerException anyway if it turns out to be null.

I guess so. Old C++ habits...

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode151
tools/ddbg.dart:151: assert(fields is List);
On 2012/05/22 01:49:09, asiva wrote:
> Also what is the style here? instead of adding our own type check asserts in
> this case does it make sense to code as:
> List fields = props["fields"];

This is my early code in which I was experimenting. Cleaning it up.

http://codereview.chromium.org/10407071/diff/11002/tools/ddbg.dart#newcode235
tools/ddbg.dart:235: assert(locals is List);
On 2012/05/22 01:49:09, asiva wrote:
> Ditto question about explicit type check assert.

Done.