Debugging in kernel shaping up.

Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.

R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/2d5147be9d7435c06dd892d145d8717b6d1f62d5
Committed: https://github.com/dart-lang/sdk/commit/e7766c950eafabfd631d693902483a13bf3bfac7

[jensj, 2017-01-16 09:51:07]

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

[jensj, 2017-01-24 07:30:41]

ping.

[Kevin Millikin (Google), 2017-01-24 13:44:13]

I wonder if the debug-step checks are in the right places.  Can you verify that
they're OK?

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel.cc#newcod...
runtime/vm/kernel.cc:1182: free(valid_token_positions.RemoveLast());
These are allocated with new and deallocated with free.  Deallocate with delete.

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_reader.cc...
runtime/vm/kernel_reader.cc:451: if (index < list->length()) {
Since you've possibly got empty lists for yield positions:

if ((index < list->length()) && (list->At(index)->length() > 0))

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_reader.cc...
runtime/vm/kernel_reader.cc:459: for (intptr_t i = 0; i < size; ++i) {
It seems simpler to me to remove the duplicates from the sorted list and then
copy everything instead of detecting the distinct elements to count them and
then repeating that to copy them.  Something like:

intptr_t last = 0;
for (intptr_t current = 1; current < size; ++current) {
  if ((*list)[last] != (*list)[current]) {
    (*list)[++last] = (*list)[current];
  }
}
list->TruncateTo(last + 1);

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2545: // Doing what's done in
EffectGraphVisitor::VisitReturnNode.
This kind of comment isn't useful.

It won't even make sense (at least might stop being true) if something changes
at a remote place.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2552: const Function& function =
parsed_function_->function();
Note that we are not doing the same thing as the classic flow graph builder's
VisitReturnNode, anyway.  There, we place the single-step check before any
finally blocks, and here we place it after.

That might explain why you are having trouble stepping past the end positions of
some functions.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2682: // Do approximately as in
EffectGraphVisitor::VisitStoreLocalNode.
Again, this comment isn't useful.

This seems somewhat surprisingly different from what the other flow graph
builder is doing.  There we are adding the single-step check before the code for
evaluating the right-hand side of some (but not all) source variable
assignments.  Here we are adding it for a possibly different set of assignments
that both includes and excludes some, and we're doing it after the code for
evaluating the right-hand side.

I think I'd be more comfortable if it was obviously the same.  If it's not, we
need a comment here that says more than just "approximately the same".  Ideally
we'd have the team come up with (1) a description of which positions need
single-step checks and whether it's a correctness or safety issue to have too
many or too few, (2) a characterization of the ones that differ between the two
front-ends, and (3) an argument that's it's OK that they differ.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:3230: dispatch <<=
I'm not sure why this is called dispatch.  I don't think of it as dispatching. 
I'd probably just write:

Fragment check(new (Z) DebugStepCheckInstr(...));
body = check + body;

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8829: RawGrowableObjectArray*
Script::GenerateLineNumberArray() const {
This function really needs a comment somewhere documenting the format of this
array, which is otherwise unstructured.

It looks like each line is described by a one-based line number and then a
sequence of (debug position, one-based column number), and that each line is
null-prefixed, and there is at least one line.  (I wonder why the lines are
null-prefixed and not null-terminated.)

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8850: int token_idx = 0;
You can also spell out index.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8853: for (int i = 0; i < line_count; i++) {
Instead of i, it's clearer to use the name 'line_index' to keep the indexes
obviously parallel and distinct.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8857: intptr_t end = LONG_MAX;
INTPTR_MAX

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8864: intptr_t seen = value.Value();
seen ==> debug_position ?

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8865: if (seen == -1) {
Can we just eliminate the -1 entries, or do we need them?

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8871: if (i != last_line) {
last_line is just used to detect the first debug position on the line that is
not -1, right?  We don't actually use the value for anything.  I find it clearer
to have something like:

bool first = true;
while (token_index < token_count) {
  ...
  if (first) {
    ...
    first = false;
  }
  ...
}

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:3558: void set_tokens_seen(const Array& value) const;
I don't really like the name.  They're not tokens, really.  And I don't know why
it's significant that they were seen (or not seen).  Obviously they were
seen....

Instead of tokens_seen I suggest something like debug_positions and instead of
yields_seen I suggest something like yield_positions.

[jensj, 2017-01-25 12:52:22]

PTAL.

In regards to the debug-step checks I suggest that we either leave them as-is
for now (it's not "done" anyway), or remove them from the CL entirely.
As you pointed out in regards to a comment referencing the flow graph builder,
even if it was exactly the same now, it wouldn't be guaranteed to be so in the
future if either implementation change slightly.

I'm good with either option, and no matter which is chosen more effort needs to
be put into it.

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel.cc#newcod...
runtime/vm/kernel.cc:1182: free(valid_token_positions.RemoveLast());
On 2017/01/24 13:44:12, Kevin Millikin (Google) wrote:
> These are allocated with new and deallocated with free.  Deallocate with
delete.

Done.

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_reader.cc...
runtime/vm/kernel_reader.cc:451: if (index < list->length()) {
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> Since you've possibly got empty lists for yield positions:
> 
> if ((index < list->length()) && (list->At(index)->length() > 0))

Done.
Though it shouldn't really change anything (though, sure, it should be faster).

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_reader.cc...
runtime/vm/kernel_reader.cc:459: for (intptr_t i = 0; i < size; ++i) {
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> It seems simpler to me to remove the duplicates from the sorted list and then
> copy everything instead of detecting the distinct elements to count them and
> then repeating that to copy them.  Something like:
> 
> intptr_t last = 0;
> for (intptr_t current = 1; current < size; ++current) {
>   if ((*list)[last] != (*list)[current]) {
>     (*list)[++last] = (*list)[current];
>   }
> }
> list->TruncateTo(last + 1);

Semi-done.

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2545: // Doing what's done in
EffectGraphVisitor::VisitReturnNode.
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> This kind of comment isn't useful.
> 
> It won't even make sense (at least might stop being true) if something changes
> at a remote place.

I will remove the comment.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2552: const Function& function =
parsed_function_->function();
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> Note that we are not doing the same thing as the classic flow graph builder's
> VisitReturnNode, anyway.  There, we place the single-step check before any
> finally blocks, and here we place it after.
> 
> That might explain why you are having trouble stepping past the end positions
of
> some functions.

I don't think that's the issue (in regards to the hack above), but I will
investigate.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2682: // Do approximately as in
EffectGraphVisitor::VisitStoreLocalNode.
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> Again, this comment isn't useful.
> 
> This seems somewhat surprisingly different from what the other flow graph
> builder is doing.  There we are adding the single-step check before the code
for
> evaluating the right-hand side of some (but not all) source variable
> assignments.  Here we are adding it for a possibly different set of
assignments
> that both includes and excludes some, and we're doing it after the code for
> evaluating the right-hand side.
> 
> I think I'd be more comfortable if it was obviously the same.  If it's not, we
> need a comment here that says more than just "approximately the same". 
Ideally
> we'd have the team come up with (1) a description of which positions need
> single-step checks and whether it's a correctness or safety issue to have too
> many or too few, (2) a characterization of the ones that differ between the
two
> front-ends, and (3) an argument that's it's OK that they differ.

I will remove the comment.

I would suggest going in detail with the insertions of DebugStepCheckInstr in
another CL though.

a) The insertion seems rather arbitrary anyway.
b) I have refactored it quite a bit for an upcomming CL anyways (where I have
done some comparison of the insertions via --print-flow-graph, though I could
probably do more)
c) it seems to me that doing too few will simply not allow us to stop at points
where we would want to stop (and as a note, having none we definitely have too
few). Whether having to many can do any harm I'm not sure. I can't really see
how though.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:3230: dispatch <<=
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> I'm not sure why this is called dispatch.  I don't think of it as dispatching.

> I'd probably just write:
> 
> Fragment check(new (Z) DebugStepCheckInstr(...));
> body = check + body;

Done.

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8829: RawGrowableObjectArray*
Script::GenerateLineNumberArray() const {
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> This function really needs a comment somewhere documenting the format of this
> array, which is otherwise unstructured.
> 
> It looks like each line is described by a one-based line number and then a
> sequence of (debug position, one-based column number), and that each line is
> null-prefixed, and there is at least one line.  (I wonder why the lines are
> null-prefixed and not null-terminated.)

In runtime/include/dart_tools_api.h Dart_ScriptGetTokenInfo is described as

/**
 * Returns an array containing line number and token offset info
 * for the given script.
 *
 * Returns an array of numbers. Null values indicate the beginning of
 * a new line. The first number after null is the line number.
 * The line number is followed by pairs of numbers, with the first value
 * being the "token offset" and the second value being the column number
 * of the token.
 * The "token offset" is a value that is used to indicate a location
 * in code, similarly to a "PC" address.
 * Source lines with no tokens are omitted.
 *
 * Requires there to be a current isolate.
 *
 * \return A handle to an array or an error object.
 */

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8850: int token_idx = 0;
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> You can also spell out index.

Done.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8853: for (int i = 0; i < line_count; i++) {
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> Instead of i, it's clearer to use the name 'line_index' to keep the indexes
> obviously parallel and distinct.

Done.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8857: intptr_t end = LONG_MAX;
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> INTPTR_MAX

Done.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8864: intptr_t seen = value.Value();
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> seen ==> debug_position ?

Done.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8865: if (seen == -1) {
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> Can we just eliminate the -1 entries, or do we need them?

I'll move the check to the recording of the entries instead.

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:8871: if (i != last_line) {
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> last_line is just used to detect the first debug position on the line that is
> not -1, right?  We don't actually use the value for anything.  I find it
clearer
> to have something like:
> 
> bool first = true;
> while (token_index < token_count) {
>   ...
>   if (first) {
>     ...
>     first = false;
>   }
>   ...
> }

It was done to look like the non-kernel version. I'll rewrite.

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

https://codereview.chromium.org/2632183002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:3558: void set_tokens_seen(const Array& value) const;
On 2017/01/24 13:44:13, Kevin Millikin (Google) wrote:
> I don't really like the name.  They're not tokens, really.  And I don't know
why
> it's significant that they were seen (or not seen).  Obviously they were
> seen....
> 
> Instead of tokens_seen I suggest something like debug_positions and instead of
> yields_seen I suggest something like yield_positions.

Done.

[jensj, 2017-01-31 09:01:36]

ping.

[Kevin Millikin (Google), 2017-01-31 09:14:01]

LGTM.

[jensj, 2017-01-31 10:01:36]

Description was changed from

==========
Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.
==========

to

==========
Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.

R=kmillikin@google.com

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

[jensj, 2017-01-31 10:01:37]

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

[jensj, 2017-01-31 10:31:21]

Description was changed from

==========
Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.

R=kmillikin@google.com

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

to

==========
Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.

R=kmillikin@google.com

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

[jensj, 2017-01-31 10:34:04]

Do we think it can compile now?

(the delta sucks because i rebased)

[Kevin Millikin (Google), 2017-01-31 10:40:37]

LGTM with the change I've noted.

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

https://codereview.chromium.org/2632183002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:8877: intptr_t end = kMaxInt32;
Maybe we actually want TokenPosition::kMaxSourcePos here?  At least, if we
change TokenPosition we'll still have the right constant.

[jensj, 2017-01-31 10:59:13]

Description was changed from

==========
Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.

R=kmillikin@google.com

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

to

==========
Debugging in kernel shaping up.

This introduces

- a list of valid token positions, so debugging actually
  starts to work, and the observatory can be loaded without crash.

- a list of valid yield positions, so stepping over await stuff
  works as expected.

- Adding "DebugStepCheckInstr" to the kernel generated il so stepping
  over await stuff works as expected, we can break in empty methods etc.

With this, approximately 80% of the service tests pass in kernel mode.

R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2d5147be9d7435c06dd892d145d8717b6d1f62d5
Committed:
https://github.com/dart-lang/sdk/commit/e7766c950eafabfd631d693902483a13bf3bfac7
==========

[jensj, 2017-01-31 10:59:14]

Committed patchset #4 (id:60001) manually as
e7766c950eafabfd631d693902483a13bf3bfac7 (presubmit successful).