Debugger step 2

Debugger step 2

Add ActivationFrame and StackTrace classes.
Add Breakpoint info: url and line number.
Fix breakpoint stub so breakpoints don't get patched over by PatchStaticCall
Committed: https://code.google.com/p/dart/source/detail?r=2089

[hausner, 2011-12-05 17:40:44]

Ping...

[siva, 2011-12-05 19:21:44]

lgtm

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

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode41
runtime/vm/debugger.cc:41: ASSERT(this->token_index_ != 0);
Storing the pc_ and token_index_ in this structure seems to un-normalize the
data somewhat. Why is it not sufficient to just store the func object and
pc_desc_index? Are you worried that the cost of accessing the pc_ and
token_index_ using the pc_descriptors array would be very high?

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode131
runtime/vm/debugger.cc:131: }
The line number computation code is duplicated here and above maybe it can be
factored into a single function intptr_t ComputeLineNumber(script, token_index);

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode281
runtime/vm/debugger.cc:281: for (int i = 0; i < stack->Length(); i++) {
stack->Length() returns an intptr_t so should i also be an intptr_t?

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode313
runtime/vm/debugger.cc:313: }
Why not register DefaultBreakpointHandler as the default handler in
Debugger::Initialize() and
ASSERT(bp_handler_ != NULL); here.

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

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode26
runtime/vm/debugger.h:26: RawScript* SourceCode();
const?

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode27
runtime/vm/debugger.h:27: RawString* SourceUrl();
const?

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode59
runtime/vm/debugger.h:59: intptr_t TokenIndex();
above 4 methods can be 'const'?

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode61
runtime/vm/debugger.h:61: char* ToCString();
'const'?

[hausner, 2011-12-05 21:11:23]

Thanks Siva.

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

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode41
runtime/vm/debugger.cc:41: ASSERT(this->token_index_ != 0);
On 2011/12/05 19:21:44, asiva wrote:
> Storing the pc_ and token_index_ in this structure seems to un-normalize the
> data somewhat. Why is it not sufficient to just store the func object and
> pc_desc_index? Are you worried that the cost of accessing the pc_ and
> token_index_ using the pc_descriptors array would be very high?

I tried to cache some data so it would not have to be computed each time. (Even
just getting the token index requires allocating handles.) Also, I have to get
the code and pc descriptors anyway to get the PC. Why duplicate that code
pattern in an accessor for token_index?

If you feel strongly I can remove token_index_ though.

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode131
runtime/vm/debugger.cc:131: }
On 2011/12/05 19:21:44, asiva wrote:
> The line number computation code is duplicated here and above maybe it can be
> factored into a single function intptr_t ComputeLineNumber(script,
token_index);

I noticed that too, but I could really only factor out the call to
GetTokenLocation. I'm not sure that's really an abstraction. In a way,
GetTokenLocation is the factored-out code here.

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode281
runtime/vm/debugger.cc:281: for (int i = 0; i < stack->Length(); i++) {
On 2011/12/05 19:21:44, asiva wrote:
> stack->Length() returns an intptr_t so should i also be an intptr_t?

Done.

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.cc#newcode313
runtime/vm/debugger.cc:313: }
On 2011/12/05 19:21:44, asiva wrote:
> Why not register DefaultBreakpointHandler as the default handler in
> Debugger::Initialize() and
> ASSERT(bp_handler_ != NULL); here.

Yes. Done.

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

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode26
runtime/vm/debugger.h:26: RawScript* SourceCode();
On 2011/12/05 19:21:44, asiva wrote:
> const?

Can't be const because it's not initialized in the constructor.

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode27
runtime/vm/debugger.h:27: RawString* SourceUrl();
On 2011/12/05 19:21:44, asiva wrote:
> const?

ditto.

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode59
runtime/vm/debugger.h:59: intptr_t TokenIndex();
On 2011/12/05 19:21:44, asiva wrote:
> above 4 methods can be 'const'?

Unfortunately not. DartFunction() assigns to a field in ActivationFrame. The
other functions call DartFunction. It's the price to pay for lazy evaluation.

http://codereview.chromium.org/8775060/diff/1/runtime/vm/debugger.h#newcode61
runtime/vm/debugger.h:61: char* ToCString();
On 2011/12/05 19:21:44, asiva wrote:
> 'const'?

const char* as the return type? Yes. Done. The function itself cannot be const
because it calls DartFunction().