TokenPositions on more nodes when running from Kernel

TokenPositions on more nodes when running from Kernel

Utilize the offsets added in previous CLs.
While this doesn't by it self give anything that resembles
a good debugging experience, it should now be possible to
set *some* breakpoints and actually break on them.

Because of the way observatory works (and because of an
unfinished Script::GenerateLineNumberArray (for kernel))
the formatting of the code you try to load up and 'debug'
has to be quite specific (e.g. no indentation at all),
and even then it mostly doesn't work.

This is step #3 in introducing these things, next step(s) will be
fixing stuff like the above.

R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/b2b00e89092dc184786da08cc7ab60d69d11ebec

[jensj, 2017-01-11 10:18:10]

jensj@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-01-12 13:43:04]

LGTM in general, but please document new fields and (especially) classes.  It
might not be you, but someone else, who has to insert the next PositionScope or
figure out if one should be moved around to try to fix a bug....

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:379: intptr_t value = ReadUInt();
Just do: intptr_t value = ReadUInt() - 1;

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:464: TokenPosition max_position_;
Please document these.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:648: class PositionScope {
Document this class.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5789: // The VM has a per-isolate table of functions
indexed by the enclosing
This comment (the part about "we don't have token positions") is wrong.  Update
it or else it will become confusing.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5797: TokenPosition position = node->position();
Is there any reason not to just continue numbering them?  Since it doesn't
matter if they're actually token positions or not, it seems simpler than
sometimes using token positions and sometimes a synthetic position based on
numbering them.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.h#n...
runtime/vm/kernel_to_il.h:808: Fragment Return(TokenPosition position =
TokenPosition::kNoSource);
None of the other methods have default values for position.  I suggest getting
rid of the default and forcing it to be passed at all the call sites (there
shouldn't be too many).  That's for consistency, but also for forcing a
developer to make a decision and for making it easy to grep for kNoSource and
find use sites.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.h#n...
runtime/vm/kernel_to_il.h:825: TokenPosition position =
TokenPosition::kNoSource);
Also here, I'd make this required.  For consistency, token position is normally
the first argument.  I'd really like it to be always first or always last (first
is fine with me).

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3477: 
I'm not a fan of these inserted blank lines (also in the other files).  Is the
Clang formatter inserting them?

[jensj, 2017-01-13 10:14:44]

Thanks for the feedback. PTAL.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:379: intptr_t value = ReadUInt();
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> Just do: intptr_t value = ReadUInt() - 1;

Done.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:464: TokenPosition max_position_;
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> Please document these.

I've added some comments (though at the getters instead where I think they make
more sense in regards to helping any user of them).

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:648: class PositionScope {
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> Document this class.

Done.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5789: // The VM has a per-isolate table of functions
indexed by the enclosing
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> This comment (the part about "we don't have token positions") is wrong. 
Update
> it or else it will become confusing.

Will do.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:5797: TokenPosition position = node->position();
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> Is there any reason not to just continue numbering them?  Since it doesn't
> matter if they're actually token positions or not, it seems simpler than
> sometimes using token positions and sometimes a synthetic position based on
> numbering them.

Numbering them 0, 1, 2 etc is rather weird at it is actually used as that
functions token position, e.g. if you debug that function in Observatory it will
show you the lines between start token position and end token position. So
starting at 0 will show you from the start of the file until whatever end is
set.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.h#n...
runtime/vm/kernel_to_il.h:808: Fragment Return(TokenPosition position =
TokenPosition::kNoSource);
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> None of the other methods have default values for position.  I suggest getting
> rid of the default and forcing it to be passed at all the call sites (there
> shouldn't be too many).  That's for consistency, but also for forcing a
> developer to make a decision and for making it easy to grep for kNoSource and
> find use sites.

Done.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/kernel_to_il.h#n...
runtime/vm/kernel_to_il.h:825: TokenPosition position =
TokenPosition::kNoSource);
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> Also here, I'd make this required.  For consistency, token position is
normally
> the first argument.  I'd really like it to be always first or always last
(first
> is fine with me).

Done.

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/2628693004/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:3477: 
On 2017/01/12 13:43:04, Kevin Millikin (Google) wrote:
> I'm not a fan of these inserted blank lines (also in the other files).  Is the
> Clang formatter inserting them?

That's probably me in an attempt to block stuff together that is logically 'a
unit' (and separate stuff that aren't).

E.g. here we do a check and bail out (I see that as a separate unit of work
which I can later ignore (i.e. no longer worry about), which is harder for me
without the blank line). Then we do some initializing. Then we go into the
kernel case.

But if you really dislike it, I'll remove them.

https://codereview.chromium.org/2628693004/diff/20001/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2628693004/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:97: declaration->position(), declaration->position(),
This is different than before --- I haven't looked into what the right thing is,
I will fix it later if it turns out this is wrong.

[Kevin Millikin (Google), 2017-01-13 12:08:04]

LGTM

[jensj, 2017-01-13 12:10:27]

Description was changed from

==========
TokenPositions on more nodes when running from Kernel

Utilize the offsets added in previous CLs.
While this doesn't by it self give anything that resembles
a good debugging experience, it should now be possible to
set *some* breakpoints and actually break on them.

Because of the way observatory works (and because of an
unfinished Script::GenerateLineNumberArray (for kernel))
the formatting of the code you try to load up and 'debug'
has to be quite specific (e.g. no indentation at all),
and even then it mostly doesn't work.

This is step #3 in introducing these things, next step(s) will be
fixing stuff like the above.
==========

to

==========
TokenPositions on more nodes when running from Kernel

Utilize the offsets added in previous CLs.
While this doesn't by it self give anything that resembles
a good debugging experience, it should now be possible to
set *some* breakpoints and actually break on them.

Because of the way observatory works (and because of an
unfinished Script::GenerateLineNumberArray (for kernel))
the formatting of the code you try to load up and 'debug'
has to be quite specific (e.g. no indentation at all),
and even then it mostly doesn't work.

This is step #3 in introducing these things, next step(s) will be
fixing stuff like the above.

R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/b2b00e89092dc184786da08cc7ab60d69d11ebec
==========

[jensj, 2017-01-13 12:10:29]

Committed patchset #2 (id:20001) manually as
b2b00e89092dc184786da08cc7ab60d69d11ebec (presubmit successful).