Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(279)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 8872049: First debugger API unit test (Closed)

Created:
9 years ago by hausner
Modified:
9 years ago
Reviewers:
siva
CC:
reviews_dartlang.org
Visibility:
Public.

Description

First debugger API unit test Add unit test file, expose more debugger functionality in the API. Committed: https://code.google.com/p/dart/source/detail?r=2320

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 26

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+295 lines, -34 lines) Patch
M runtime/include/dart_debugger_api.h View 1 2 2 chunks +65 lines, -0 lines 0 comments Download
M runtime/vm/debugger.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M runtime/vm/debugger.cc View 1 2 5 chunks +45 lines, -27 lines 0 comments Download
M runtime/vm/debugger_api_impl.cc View 1 2 2 chunks +92 lines, -6 lines 0 comments Download
A runtime/vm/debugger_api_impl_test.cc View 1 2 1 chunk +87 lines, -0 lines 0 comments Download
M runtime/vm/object.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M runtime/vm/vm_sources.gypi View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
hausner
9 years ago (2011-12-09 00:15:39 UTC) #1
siva
LGTM with some nit comments. http://codereview.chromium.org/8872049/diff/5001/runtime/include/dart_debugger_api.h File runtime/include/dart_debugger_api.h (right): http://codereview.chromium.org/8872049/diff/5001/runtime/include/dart_debugger_api.h#newcode18 runtime/include/dart_debugger_api.h:18: Dart_StackTrace stack_trace); Why do ...
9 years ago (2011-12-09 02:20:04 UTC) #2
hausner
9 years ago (2011-12-13 00:14:59 UTC) #3 
http://codereview.chromium.org/8872049/diff/5001/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/8872049/diff/5001/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:18: Dart_StackTrace stack_trace);
On 2011/12/09 02:20:04, asiva wrote:
> Why do we need to always create a stack trace and pass it as a param to the
> handler function. Why not have a Dart_GetStackTrace function which will return
> the current stack trace.
> See comment below regarding events...
> For some events I may not be interested in a stack trace at all just the
current
> location (i.e top frame).

As we discussed in person, I strive for an interface that allows clients to make
as few API calls as possible, since remote debuggers and cross-isolate debugging
may ultimately make API calls expensive.

http://codereview.chromium.org/8872049/diff/5001/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:45: Dart_BreakpointHandler bp_handler);
On 2011/12/09 02:20:04, asiva wrote:
> I am wondering if long term we would need an event handler here not just a
> breakpoint handler. The events could be breakpoints, exception thrown,
exception
> caught, isolate death, new isolate start, single step ...

I like the DebugEvent idea as a generalization. I am building things bottom-up
right now and when I know enough about debugging APIs, I'll definitely
generalize along this idea.

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

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:23: DEFINE_FLAG(charp, bpt, NULL, "Debug breakpoint at
<func>");
On 2011/12/09 02:20:04, asiva wrote:
> Is this flag used anymore?
No. Good catch.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:25: static const bool verbose = false;
On 2011/12/09 02:20:04, asiva wrote:
> Can this be turned on at the command line?
No it's just for my own "debugger debugging" at this point. The verbose output
will go away over time.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:102: func_name.ToCString());
On 2011/12/09 02:20:04, asiva wrote:
> ditto comment regarding library URL needed for full qualification?

Yes. Cf other comment.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:333: if (bp_handler_ != NULL) {
On 2011/12/09 02:20:04, asiva wrote:
> Can bp_handler_ ever be NULL, it ties back to the question of can somebody
call
> the dart API function and set this to NULL?

Yes, it can be NULL if the caller would like to remove its registered breakpoint
callback. In that case, we just install the default handler.

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

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:36: type* var = reinterpret_cast<type*>(param);
On 2011/12/09 02:20:04, asiva wrote:
> We are handing out these C++ VM object pointers and are assuming that the
> embedder will pass back a pointer of the appropriate type here. Not sure if
this
> is going to be a problem. I am wondering if we should have a base
> Debugger_Object class with virtual methods IsBreakpoint, IsActivationFrame,
> IsStackTrace etc. and then have the individual classes Breakpoint, Stacktrace,
> ActivationFrame etc. derive from this base class and return true for the
> appropriate method. That way you can check here if it is the appropriate type
> before the cast and report an error if it is some random object.

As we discussed in person, we have type safety through the typedefs that make
Dart_Breakpoint, Dart_StackTrace, Dart_Isolate etc incompatible. I agree that
longer-term we want a much more robust API that can detect whether the pointers
point to valid objects of the correct type. I put that on the long term TODO
list.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:81:
reinterpret_cast<BreakpointHandler*>(bp_handler);
On 2011/12/09 02:20:04, asiva wrote:
> can bp_handler be NULL? Do we need a non NULL check here?

It can be NULL. Cf. other comment.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:138: CURRENT_FUNC,
On 2011/12/09 02:20:04, asiva wrote:
> I presume the library URL is also needed to fully qualify the name?

I think the names would become very unreadable if we added the library url. It's
technically necessary to clearly identify the function, though.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:156: *breakpoint =
reinterpret_cast<Dart_Breakpoint>(bpt);
On 2011/12/09 02:20:04, asiva wrote:
> Should the setting of *breakpoint be under an else { .... } as *breakpoint has
> already been initialized to NULL so there is no need to again assign NULL to
it
> when bpt is NULL.

Yes, that's cleaner. Done.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:36: BARK_IF_ERROR(res);
On 2011/12/09 02:20:04, asiva wrote:
> EXPECT(!Dart_IsError(res)); ?

Done.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:39: BARK_IF_ERROR(res);
On 2011/12/09 02:20:04, asiva wrote:
> EXPECT(!Dart_IsError(res)); ?

Done.

http://codereview.chromium.org/8872049/diff/5001/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:72: BARK_IF_ERROR(res);
On 2011/12/09 02:20:04, asiva wrote:
> Is this BARK stuff here for debugging purposes? :-)

I'd like to keep it since it prints the error message if there is an error. 
Renamed to EXPECT_NOT_ERROR and included the EXPECT(!Dart_IsError( )).

Powered by Google App Engine
This is Rietveld 408576698