First part of inspecting local variables

First part of inspecting local variables

This change introduces variable descriptors that describe the name, stack slot index, and source code stretch in which a variable is valid.

Activation frames in the stack trace now can enumerate the variables that are active in that frame.

Next step: fetch the value of variables from the stack.
Committed: https://code.google.com/p/dart/source/detail?r=2671

[siva, 2011-12-20 19:11:26]

LGTM with some comments.

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

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:227: *value = Instance::null();
Do you need a TODO here stating that getting of local variable values is not
implemented yet?

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

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/debugger.h#newcode73
runtime/vm/debugger.h:73: RawFunction* function_;
How is this object pointer traversed during GC?

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

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4593: ASSERT(var_index < names.Length());
This assertion should be:
ASSERT(Length() == names.Length());

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4600: intptr_t* begin_pos,
begin_pos and end_pos don't seem to convey which position, is it code position
or token position, maybe you could qualify it as begin_tok_pos or
begin_token_pos. Ditto for end_pos.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4615: const Array& names =
Array::Handle(raw_ptr()->names_);
ASSERT(Length() == names.Length());

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4639: result.raw_ptr()->names_ = Array::New(num_variables);
For this I would use a SetNames or set_names accessor which
would be implemented as:
void set_names(const Array& names) {
  StorePointer(&raw_ptr()->names_, names.raw());
}
and change this to:
const Array& names = Array::Handle(Array::New(num_variables, Heap::kOld));
result.set_names(names);

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4640: result.raw_ptr()->length_ = num_variables;
The length_ field should be set inside the NoGCScope scope that you have above
because if we trigger a GC and length_ is not set then we would not be able to
calculate the instance size.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/scopes.cc#newcode191
runtime/vm/scopes.cc:191: LocalVariable *var = vars[i];
LocalVariable* var = vars[i];

[hausner, 2011-12-20 21:43:55]

Thank you.

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

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:227: *value = Instance::null();
On 2011/12/20 19:11:26, asiva wrote:
> Do you need a TODO here stating that getting of local variable values is not
> implemented yet?
Done. It's what I'll do in the next change.

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

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/debugger.h#newcode73
runtime/vm/debugger.h:73: RawFunction* function_;
On 2011/12/20 19:11:26, asiva wrote:
> How is this object pointer traversed during GC?
Good point. Changed it into a handle.

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

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4593: ASSERT(var_index < names.Length());
On 2011/12/20 19:11:26, asiva wrote:
> This assertion should be:
> ASSERT(Length() == names.Length());

True, that is more accurate.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4600: intptr_t* begin_pos,
On 2011/12/20 19:11:26, asiva wrote:
> begin_pos and end_pos don't seem to convey which position, is it code position
> or token position, maybe you could qualify it as begin_tok_pos or
> begin_token_pos. Ditto for end_pos.

Done.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4615: const Array& names =
Array::Handle(raw_ptr()->names_);
On 2011/12/20 19:11:26, asiva wrote:
> ASSERT(Length() == names.Length());

Done.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4639: result.raw_ptr()->names_ = Array::New(num_variables);
On 2011/12/20 19:11:26, asiva wrote:
> For this I would use a SetNames or set_names accessor which
> would be implemented as:
> void set_names(const Array& names) {
>   StorePointer(&raw_ptr()->names_, names.raw());
> }
> and change this to:
> const Array& names = Array::Handle(Array::New(num_variables, Heap::kOld));
> result.set_names(names);
> 

Done. But without the setter boilerplate. There is only this one place where we
assign to names_, so a setter seems unnecessary noise.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4640: result.raw_ptr()->length_ = num_variables;
On 2011/12/20 19:11:26, asiva wrote:
> The length_ field should be set inside the NoGCScope scope that you have above
> because if we trigger a GC and length_ is not set then we would not be able to
> calculate the instance size.

Done.

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

http://codereview.chromium.org/8992020/diff/7001/runtime/vm/scopes.cc#newcode191
runtime/vm/scopes.cc:191: LocalVariable *var = vars[i];
On 2011/12/20 19:11:26, asiva wrote:
> LocalVariable* var = vars[i];

Done.

[srdjan, 2011-12-20 22:07:04]

DBC

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

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:80: Function* ActivationFrame::DartFunction() {
Maybe you could return const Function&, since you will always return a non-null
value, and then the caller does not need to worry if it is NUL or not?

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:160: ActiveVariables* ActivationFrame::LocalVariables() {
Ditto const ActiveVariables&

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/scopes.h#newcode292
runtime/vm/scopes.h:292: intptr_t end_token_index_;  // Token index of end of
scope.
Inclusive or exclusive?

[hausner, 2011-12-20 22:13:29]

Thanks for taking a look.

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

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:80: Function* ActivationFrame::DartFunction() {
On 2011/12/20 22:07:04, srdjan wrote:
> Maybe you could return const Function&, since you will always return a
non-null
> value, and then the caller does not need to worry if it is NUL or not?

Will address in next checkin. Good idea.

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:160: ActiveVariables* ActivationFrame::LocalVariables() {
On 2011/12/20 22:07:04, srdjan wrote:
> Ditto const ActiveVariables&
Ditto

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/scopes.h
File runtime/vm/scopes.h (right):

http://codereview.chromium.org/8992020/diff/17001/runtime/vm/scopes.h#newcode292
runtime/vm/scopes.h:292: intptr_t end_token_index_;  // Token index of end of
scope.
On 2011/12/20 22:07:04, srdjan wrote:
> Inclusive or exclusive?
Inclusive, but it depends on what the parser assigns to the SequenceNode end
token index...