Kernel debugging; service tests

Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with (before fasta)
    ```
    python tools/test.py -mrelease -cdartk --no-tree-shake --no-dfe service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

As fasta has now landed debugging does not currently work, but this is still needed in order to both test better and allow for proper kernel debugging support.

R=hausner@google.com, kmillikin@google.com

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

[jensj, 2017-02-08 12:35:40]

Description was changed from

==========
Kernel debugging; service tests

- Offsets om more stuff
- Status file for kernel
 -> Testing with
    python tools/test.py -mrelease -cdartk --use-standalone-dartk
--no-tree-shake service
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

Note that this currently also includes
https://codereview.chromium.org/2670303003/
hopefully that can be resolved by itself (either by landing or finding some
other
fix).
==========

to

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with
    ```
    python tools/test.py -mrelease -cdartk --use-standalone-dartk
--no-tree-shake service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

Note that this currently also includes
https://codereview.chromium.org/2670303003/
hopefully that can be resolved by itself (either by landing or finding some
other
fix).
==========

[jensj, 2017-02-08 13:16:35]

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

[Kevin Millikin (Google), 2017-02-08 15:37:53]

A first round of comments.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:820: FileOffset fileOffset;
Document these two offsets here.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/ast_from_analyzer.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1374: int _getOffset(AstNode
node, ast.Expression expression) {
This is a warty API.  It takes an analyzer AstNode (isn't it always an
Expression?) and a Kernel Expression.  It just ignores the second argument
except in case the first argument is a MethodInvocation and the second argument
has a non-negative file offset.

As far as I can tell, the left-hand side of an assignment cannot be a method
invocation, so we definitely don't need to pass the second argument to the
recursive call.  And for the non-recursive call (am I missing other call sites?)
we could avoid passing the second argument with:

// For some method invocations we have already set a file offset to
// override the default behavior of _getOffset.
if (node is! MethodInvocation || result.fileOffset < 0) {
  result.fileOffset = _getOffset(node);
}
return result;

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1880: ..fileOffset =
node.isOperator.offset);
It seems weird that the IsExpression that comes from 'is!' has a file offset,
but the IsExpression that comes from 'is' does not.  I can't figure out why, so
it probably needs a comment.

It also seems better to call _getOffset(node) instead of writing
node.isOperator.offset explicitly.  The only documentation we currently have of
which offsets we record for which kinds of expressions is by reading _getOffset.
 It would be easy to miss the case for is!.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1934: ..fileOffset =
node.methodName.end;
This definitely needs a comment.  We are explicitly setting the file offset
here, for the case of invoking a method through a local, to override the default
behavior of the call in 'build' that would use the methodName's (start)
position, right?

Add a comment to that effect here.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3003: /// Offset of the equals sign in the source file
it comes from.
For dartdoc comments, I suggest following the style guide:
https://www.dartlang.org/guides/language/effective-dart/documentation

Namely, the first paragraph should be a short (one line) description, followed
by a blank line and then more details.  Putting it all on one line like this
makes the entire comment show up as the summary in some views, which looks
cluttered.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:1: //
Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
2017

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/debugger_location_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/debugger_location_test.dart:58: void dummy() {
Do we have a plan for a better way to do this?

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/field_script_other.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/field_script_other.dart:5: part of
field_scrip_test;
scrip => script

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:663:
expect(result['dependencies'].length, isPositive);
Do we plan to construct them, or do we think they are not needed?

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_assign_call_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_assign_call_test.dart:1: import
'test_helper.dart';
Even tests should have a license header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_assign_int_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_assign_int_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_catch_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_catch_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_closure_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_closure_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_create_list_and_map_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_create_list_and_map_test.dart:1:
import 'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_for_each_loop_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_for_each_loop_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_for_loop_with_break_and_continue_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_for_loop_with_break_and_continue_test.dart:1:
import 'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_function_expression_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_function_expression_test.dart:1:
import 'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_is_and_as_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_is_and_as_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_new_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_new_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_simple_async_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_async_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_simple_async_with_returns_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_async_with_returns_test.dart:1:
import 'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_simple_linear_2_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_linear_2_test.dart:1:
import 'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_simple_linear_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_linear_test.dart:1: import
'test_helper.dart';
License header.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/service_kernel.status (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/service_kernel.status:1: [ $compiler == dartk
]
Even status files have a license.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/service_kernel.status:11: # TEMPORARY STUFF
(HOPEFULLY)
How about an indication why they fail?

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/service_kernel.status:16: # New failures that
I need to fix
This comment isn't helpful.

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

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1331: bool
TranslationHelper::ShouldAddDebugInstruction(const Function& function,
Rename this to something like NeedsDebugStepCheck.

Why?  First it's just a predicate and it doesn't care how the caller will
interpret the result.  We could use the result for anything, not necessarily
adding an instruction.  It will end up looking really strange at the call site
if we end up using it for some other purpose.

Second, the "DebugInstruction" has a name which is DebugStepCheckInstr.  That's
not necessarily a great name, but it's confusing to arbitrarily use different
names at each level of abstraction.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1333: if (!(FLAG_support_debugger &&
position.IsDebugPause())) return false;
I think this reads better as:

return FLAG_support_debugger &&
    position.IsDebugPause() &&
    !function.is_native() &&
    function.is_debuggable();

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1338: bool
TranslationHelper::ShouldAddDebugInstruction(Value* value,
This function's implementation is cluttered.  Suggestions below.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1340: if (!(FLAG_support_debugger &&
position.IsDebugPause())) return false;
Push negation in (something like negation normal form).  It's easier to read:

if (!FLAG_support_debugger || !position.IsDebugPause()) return false;

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1341: if (value->definition()->IsConstant()) return
true;
Name value->definition(), it will not change and we don't have to repeat it
everywhere:

Definition* definition = value->definition();

Put the simple cases first:

if (definition->IsConstant() || definition->IsLoadStaticField()) return true;

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1348: if (value->definition()->IsLoadLocal() &&
Just return the value of the last test:

return definition->IsLoadLocal() &&
    !definition->AsLoadLocal()->local().IsInternal();

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1356: Fragment
TranslationHelper::GetDebugInstruction(TokenPosition position) {
Move this to the graph builder.  Use the same name as the flow graph instruction
otherwise it looks complicated when it's in fact simple.  (It's not "getting"
anything anyway, it's making it.)

TranslationHelper::GetDebugInstruction ==> FlowGraphBuilder::DebugStepCheck.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:4511: if (H.ShouldAddDebugInstruction(stack_,
node->position())) {
It's not usual that we build the graph by prepending a fragment to an existing
one.  It's unusual enough that I would do it immediately when it becomes
possible.  Call DebugStepCheck() after TranslateExpression, not in between
CheckVariableTypeInCheckedMode and StoreLocal, which seems kind of random.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:4568: instructions =
H.GetDebugInstruction(node->position()) + instructions;
Call DebugStepCheck() after TranslateExpression, not here.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:5351: // In parser.cc's
Parser::ParseVariableDeclaration the position is actually
Don't refer to the (other) producer(s) of this value, it's not important and it
will be an obsolete comment if (when?) parser.cc is deleted.

Instead, please refer to the consumer of the value, which is important.  Just
say that the debugger expects the position of the equals if there is an
initializer or the identifier if not.

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

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:289: bool ShouldAddDebugInstruction(const Function&
function,
Why are these in the translation helper instead of in the graph builder?  Do we
anticipate using them for something other than graph building?

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:292: Fragment GetDebugInstruction(TokenPosition
position);
This one should definitely be in the graph builder.  It's just building a
fragment of a graph which doesn't make sense in any context outside of the graph
builder.

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

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9129: if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
||
This is Scanner::IsIdentChar(c), isn't it?

[Cutch, 2017-02-08 16:48:19]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2017-02-08 16:48:20]

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:52: var
stream = await isolate.vm.getEventStream(VM.kDebugStream);
Please factor this code into a helper method.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:60: if
(event.kind == ServiceEvent.kPauseBreakpoint) {
this is hasStoppedAtBreakpoint

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:81:
completer = new Completer();
(here and elsewhere) Please use the helper: hasStoppedAtBreakpoint

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/debugger_location_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/debugger_location_test.dart:58: void dummy() {
Why is this needed?

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:663:
expect(result['dependencies'].length, isPositive);
Please do not add public fields to the service protocol.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:710: if
(isKernel(isolate)) {
please find a way to express this without adding all of these 'if kernel else'
branches to tests.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_source_report_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_source_report_test.dart:102:
expect(coverage['ranges'].length, 7); // kernel async transform adds one.
please find a way to express the difference without adding all of these 'if
kernel else ' branches to tests.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/set_library_debuggable_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/set_library_debuggable_test.dart:33:
stoppedInFunction('testMain', contains: true, includeOwner: true),
why is includeOwner needed here?

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/set_name_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/set_name_rpc_test.dart:13: if
(isKernel(isolate)) {
dito

[Kevin Millikin (Google), 2017-02-08 19:38:21]

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:710: if
(isKernel(isolate)) {
On 2017/02/08 16:48:20, Cutch wrote:
> please find a way to express this without adding all of these 'if kernel else'
> branches to tests.

Do you have a suggestion?

[Cutch, 2017-02-08 22:02:37]

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:663:
expect(result['dependencies'].length, isPositive);
On 2017/02/08 16:48:20, Cutch wrote:
> Please do not add public fields to the service protocol.

Ignore. I misread this code, this isn't a new field.

[jensj, 2017-02-13 14:04:17]

Thanks for the review.

PTAL.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:820: FileOffset fileOffset;
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> Document these two offsets here.

Done.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/ast_from_analyzer.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1374: int _getOffset(AstNode
node, ast.Expression expression) {
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> This is a warty API.  It takes an analyzer AstNode (isn't it always an
> Expression?) and a Kernel Expression.  It just ignores the second argument
> except in case the first argument is a MethodInvocation and the second
argument
> has a non-negative file offset.
> 
> As far as I can tell, the left-hand side of an assignment cannot be a method
> invocation, so we definitely don't need to pass the second argument to the
> recursive call.  And for the non-recursive call (am I missing other call
sites?)
> we could avoid passing the second argument with:
> 
> // For some method invocations we have already set a file offset to
> // override the default behavior of _getOffset.
> if (node is! MethodInvocation || result.fileOffset < 0) {
>   result.fileOffset = _getOffset(node);
> }
> return result;

Done.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1880: ..fileOffset =
node.isOperator.offset);
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> It seems weird that the IsExpression that comes from 'is!' has a file offset,
> but the IsExpression that comes from 'is' does not.  I can't figure out why,
so
> it probably needs a comment.
> 
> It also seems better to call _getOffset(node) instead of writing
> node.isOperator.offset explicitly.  The only documentation we currently have
of
> which offsets we record for which kinds of expressions is by reading
_getOffset.
>  It would be easy to miss the case for is!.

Done.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/ast_from_analyzer.dart:1934: ..fileOffset =
node.methodName.end;
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> This definitely needs a comment.  We are explicitly setting the file offset
> here, for the case of invoking a method through a local, to override the
default
> behavior of the call in 'build' that would use the methodName's (start)
> position, right?
> 
> Add a comment to that effect here.

Done.

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3003: /// Offset of the equals sign in the source file
it comes from.
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> For dartdoc comments, I suggest following the style guide:
> https://www.dartlang.org/guides/language/effective-dart/documentation
> 
> Namely, the first paragraph should be a short (one line) description, followed
> by a blank line and then more details.  Putting it all on one line like this
> makes the entire comment show up as the summary in some views, which looks
> cluttered.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:1: //
Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> 2017

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:52: var
stream = await isolate.vm.getEventStream(VM.kDebugStream);
On 2017/02/08 16:48:19, Cutch wrote:
> Please factor this code into a helper method.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:60: if
(event.kind == ServiceEvent.kPauseBreakpoint) {
On 2017/02/08 16:48:19, Cutch wrote:
> this is hasStoppedAtBreakpoint

Acknowledged.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart:81:
completer = new Completer();
On 2017/02/08 16:48:20, Cutch wrote:
> (here and elsewhere) Please use the helper: hasStoppedAtBreakpoint

I've refactored this file and the file it was copied from.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/debugger_location_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/debugger_location_test.dart:58: void dummy() {
On 2017/02/08 16:48:20, Cutch wrote:
> Why is this needed?

Even though unittest.dart is included, it isn't included in the dill file unless
we actually use it. By reading this field we actually use the file, and it is
thus included in the file.
If it isn't included we cannot set breakpoints in unittest.dart.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/debugger_location_test.dart:58: void dummy() {
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> Do we have a plan for a better way to do this?

I don't know. It seems somewhat artificial to me to want to set a breakpoint in
an unused library, so I'm pretty okay with this...

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/field_script_other.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/field_script_other.dart:5: part of
field_scrip_test;
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> scrip => script

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:663:
expect(result['dependencies'].length, isPositive);
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> Do we plan to construct them, or do we think they are not needed?

I haven't talked to anyone about it. It shown in the observatory interface, but
other than that I'm not sure what it's actually used for.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:710: if
(isKernel(isolate)) {
On 2017/02/08 16:48:20, Cutch wrote:
> please find a way to express this without adding all of these 'if kernel else'
> branches to tests.

The only other way I see is to do what I've done with e.g.
add_breakpoint_rpc_kernel_test.dart vs add_breakpoint_rpc_test.dart - i.e.
create a copy, make it kernel specific, skip the kernel one in the vm status
file, and skip the original one in the kernel status file. I though this was
better in this case.

If you prefer I can split out the 3 files that use this though
(get_object_rpc_test.dart, get_source_report_test.dart and
set_name_rpc_test.dart).

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_assign_call_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_assign_call_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> Even tests should have a license header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_assign_int_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_assign_int_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_catch_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_catch_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_closure_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_closure_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_create_list_and_map_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_create_list_and_map_test.dart:1:
import 'test_helper.dart';
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_for_each_loop_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_for_each_loop_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:51, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_for_loop_with_break_and_continue_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_for_loop_with_break_and_continue_test.dart:1:
import 'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_function_expression_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_function_expression_test.dart:1:
import 'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_is_and_as_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_is_and_as_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_new_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_new_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_simple_async_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_async_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File
runtime/observatory/tests/service/next_through_simple_async_with_returns_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_async_with_returns_test.dart:1:
import 'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_simple_linear_2_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_linear_2_test.dart:1:
import 'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_simple_linear_test.dart
(right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_simple_linear_test.dart:1: import
'test_helper.dart';
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> License header.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/service_kernel.status (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/service_kernel.status:1: [ $compiler == dartk
]
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Even status files have a license.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/service_kernel.status:11: # TEMPORARY STUFF
(HOPEFULLY)
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> How about an indication why they fail?

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/service_kernel.status:16: # New failures that
I need to fix
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> This comment isn't helpful.

Yeah, sorry about that. It was a new test that came in when I rebased, and as
there were lots of other stuff misbehaving i just needed to get rid of this...

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/set_library_debuggable_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/set_library_debuggable_test.dart:33:
stoppedInFunction('testMain', contains: true, includeOwner: true),
On 2017/02/08 16:48:20, Cutch wrote:
> why is includeOwner needed here?

From source the name of the async function is something like
"testMain_async_body", via kernel it's just "async_op", so without includeOwner
I get the following error:
"Expected to be in function testMain but actually in function async_op".

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

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1331: bool
TranslationHelper::ShouldAddDebugInstruction(const Function& function,
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Rename this to something like NeedsDebugStepCheck.
> 
> Why?  First it's just a predicate and it doesn't care how the caller will
> interpret the result.  We could use the result for anything, not necessarily
> adding an instruction.  It will end up looking really strange at the call site
> if we end up using it for some other purpose.
> 
> Second, the "DebugInstruction" has a name which is DebugStepCheckInstr. 
That's
> not necessarily a great name, but it's confusing to arbitrarily use different
> names at each level of abstraction.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1333: if (!(FLAG_support_debugger &&
position.IsDebugPause())) return false;
On 2017/02/08 15:37:53, Kevin Millikin (Google) wrote:
> I think this reads better as:
> 
> return FLAG_support_debugger &&
>     position.IsDebugPause() &&
>     !function.is_native() &&
>     function.is_debuggable();

Semi-done. git cl format runtime has a different idea about line-breaks.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1338: bool
TranslationHelper::ShouldAddDebugInstruction(Value* value,
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> This function's implementation is cluttered.  Suggestions below.

Acknowledged.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1340: if (!(FLAG_support_debugger &&
position.IsDebugPause())) return false;
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Push negation in (something like negation normal form).  It's easier to read:
> 
> if (!FLAG_support_debugger || !position.IsDebugPause()) return false;

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1341: if (value->definition()->IsConstant()) return
true;
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Name value->definition(), it will not change and we don't have to repeat it
> everywhere:
> 
> Definition* definition = value->definition();
> 
> Put the simple cases first:
> 
> if (definition->IsConstant() || definition->IsLoadStaticField()) return true;

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1348: if (value->definition()->IsLoadLocal() &&
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Just return the value of the last test:
> 
> return definition->IsLoadLocal() &&
>     !definition->AsLoadLocal()->local().IsInternal();

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:1356: Fragment
TranslationHelper::GetDebugInstruction(TokenPosition position) {
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Move this to the graph builder.  Use the same name as the flow graph
instruction
> otherwise it looks complicated when it's in fact simple.  (It's not "getting"
> anything anyway, it's making it.)
> 
> TranslationHelper::GetDebugInstruction ==> FlowGraphBuilder::DebugStepCheck.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:4511: if (H.ShouldAddDebugInstruction(stack_,
node->position())) {
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> It's not usual that we build the graph by prepending a fragment to an existing
> one.  It's unusual enough that I would do it immediately when it becomes
> possible.  Call DebugStepCheck() after TranslateExpression, not in between
> CheckVariableTypeInCheckedMode and StoreLocal, which seems kind of random.

You're right. I think it was somewhat of a 'bad merge'.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:4568: instructions =
H.GetDebugInstruction(node->position()) + instructions;
On 2017/02/08 15:37:53, Kevin Millikin (Google) wrote:
> Call DebugStepCheck() after TranslateExpression, not here.

Done.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.cc:5351: // In parser.cc's
Parser::ParseVariableDeclaration the position is actually
On 2017/02/08 15:37:52, Kevin Millikin (Google) wrote:
> Don't refer to the (other) producer(s) of this value, it's not important and
it
> will be an obsolete comment if (when?) parser.cc is deleted.
> 
> Instead, please refer to the consumer of the value, which is important.  Just
> say that the debugger expects the position of the equals if there is an
> initializer or the identifier if not.

Done.

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

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:289: bool ShouldAddDebugInstruction(const Function&
function,
On 2017/02/08 15:37:53, Kevin Millikin (Google) wrote:
> Why are these in the translation helper instead of in the graph builder?  Do
we
> anticipate using them for something other than graph building?

I paid to little attention to the name; I just saw it as a place for utility
functions. I'll move it.

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:292: Fragment GetDebugInstruction(TokenPosition
position);
On 2017/02/08 15:37:53, Kevin Millikin (Google) wrote:
> This one should definitely be in the graph builder.  It's just building a
> fragment of a graph which doesn't make sense in any context outside of the
graph
> builder.

Done.

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

https://codereview.chromium.org/2680303002/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9129: if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
||
On 2017/02/08 15:37:53, Kevin Millikin (Google) wrote:
> This is Scanner::IsIdentChar(c), isn't it?

Yes, but that's private.

[hausner, 2017-02-14 13:16:03]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2017-02-14 13:16:04]

DBC: Comments in the tests that document at which token we expect a breakpoint
will be helpful for future maintenance.

The tests might be brittle, breaking when we change the behavior of the debugger
slightly. I don't know how to fix the that, though.

https://codereview.chromium.org/2680303002/diff/60001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_closure_test.dart (right):

https://codereview.chromium.org/2680303002/diff/60001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_closure_test.dart:24:
"$file:${LINE_A+0}:9",
Add comments to explain which token you expect at that location?

"$file:${LINE_A+0}:9",  // Multiplication operator *
"$file:${LINE_A+0}:7",  // Identifier i
...

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

https://codereview.chromium.org/2680303002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9129: if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
||
You could use Scanner::IsIdentStartChar() and Scanner::IsIdentChar(). When the
VM scanner is eventually going away, a few basic functions will probably survive
because they are helpful in situations like this.

[jensj, 2017-02-14 13:23:55]

https://codereview.chromium.org/2680303002/diff/60001/runtime/observatory/tes...
File runtime/observatory/tests/service/next_through_closure_test.dart (right):

https://codereview.chromium.org/2680303002/diff/60001/runtime/observatory/tes...
runtime/observatory/tests/service/next_through_closure_test.dart:24:
"$file:${LINE_A+0}:9",
On 2017/02/14 13:16:04, hausner wrote:
> Add comments to explain which token you expect at that location?
> 
> "$file:${LINE_A+0}:9",  // Multiplication operator *
> "$file:${LINE_A+0}:7",  // Identifier i
> ...
> 

I will add that.

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

https://codereview.chromium.org/2680303002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9129: if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z')
||
On 2017/02/14 13:16:04, hausner wrote:
> You could use Scanner::IsIdentStartChar() and Scanner::IsIdentChar(). When the
> VM scanner is eventually going away, a few basic functions will probably
survive
> because they are helpful in situations like this.

Kevin suggested the same thing, but the methods are private.
What do you prefer --- making them public, make Script a friend class or keep it
as is?

[jensj, 2017-02-15 14:00:45]

Description was changed from

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with
    ```
    python tools/test.py -mrelease -cdartk --use-standalone-dartk
--no-tree-shake service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

Note that this currently also includes
https://codereview.chromium.org/2670303003/
hopefully that can be resolved by itself (either by landing or finding some
other
fix).
==========

to

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with
    ```
    python tools/test.py -mrelease -cdartk --no-tree-shake --no-dfe service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.
==========

[jensj, 2017-03-03 11:37:35]

ping.

[Kevin Millikin (Google), 2017-03-06 13:57:59]

The changes to Kernel, the VM's reader, and the flow graph builder LGTM.

[hausner, 2017-03-06 16:58:56]

Rest of VM changes LGTM.

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

https://codereview.chromium.org/2680303002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:9233: for (intptr_t i = offset + 1; i < source.Length();
++i, ++*token_len) {
I find this hard to read, since the for loop more or less does what the loop
body should do, and the body contains half the termination condition.

Assuming that source.CharAt() returns 0 as the last character in the source, the
loop could be rewritten as:

for (intptr_t i = offset + 1; Scanner::IsIdentChar(source.CharAt(i)); ++i) {
  ++*token_len;
}

[Cutch, 2017-03-07 01:58:37]

Not LGTM until the following are addressed:

1) Please address the branches in the individual tests (e.g. at the least, make
a helper function with the branch but call the helper functions in the tests)

2) I think that function names from kernel programs need to match the names of
the functions generated by the VM's parser. e.g. async_op differences

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/get_object_rpc_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/get_object_rpc_test.dart:710: if
(isKernel(isolate)) {
On 2017/02/13 14:04:15, jensj wrote:
> On 2017/02/08 16:48:20, Cutch wrote:
> > please find a way to express this without adding all of these 'if kernel
else'
> > branches to tests.
> 
> The only other way I see is to do what I've done with e.g.
> add_breakpoint_rpc_kernel_test.dart vs add_breakpoint_rpc_test.dart - i.e.
> create a copy, make it kernel specific, skip the kernel one in the vm status
> file, and skip the original one in the kernel status file. I though this was
> better in this case.
> 
> If you prefer I can split out the 3 files that use this though
> (get_object_rpc_test.dart, get_source_report_test.dart and
> set_name_rpc_test.dart).

Why not create a helper functions which include the if (isKernel(...)) and then
just call those in the tests?

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/set_library_debuggable_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/set_library_debuggable_test.dart:33:
stoppedInFunction('testMain', contains: true, includeOwner: true),
On 2017/02/13 14:04:16, jensj wrote:
> On 2017/02/08 16:48:20, Cutch wrote:
> > why is includeOwner needed here?
> 
> From source the name of the async function is something like
> "testMain_async_body", via kernel it's just "async_op", so without
includeOwner
> I get the following error:
> "Expected to be in function testMain but actually in function async_op".

The function names reported via kernel and the VM today need to be the same.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/set_library_debuggable_test.dart:38:
stoppedInFunction('testMain', contains: true, includeOwner: true),
dito here

[jensj, 2017-03-07 08:43:28]

Description was changed from

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with
    ```
    python tools/test.py -mrelease -cdartk --no-tree-shake --no-dfe service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.
==========

to

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with (before fasta)
    ```
    python tools/test.py -mrelease -cdartk --no-tree-shake --no-dfe service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

As fasta has now landed debugging does not currently work, but this is still
needed in order to both test better and allow for proper kernel debugging
support.
==========

[jensj, 2017-03-07 08:43:40]

In regards to the 2 issues John brought up:

1) I have removed the branches and marked them as skip in kernel for now. This
has to change anyway, as we've moved to fasta and the kernel isolate. How it is
addressed once fasta-debugging gets up-to-speed will be an issue for another CL.
I thus see that issue as 'closed' here.

2) I'm rather sure this was fixed about a week ago, but it cannot currently be
tested because of the switch to fasta. I have removed the includeOwner stuff.
Thanks for pointing that out.

In total this means that all changes to existing tests have been canceled and
only additions remain.
That should address all concerns that John brought up, leaving me 3 lgtm's.
Landing.

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
File runtime/observatory/tests/service/set_library_debuggable_test.dart (right):

https://codereview.chromium.org/2680303002/diff/40001/runtime/observatory/tes...
runtime/observatory/tests/service/set_library_debuggable_test.dart:33:
stoppedInFunction('testMain', contains: true, includeOwner: true),
On 2017/03/07 01:58:37, Cutch wrote:
> On 2017/02/13 14:04:16, jensj wrote:
> > On 2017/02/08 16:48:20, Cutch wrote:
> > > why is includeOwner needed here?
> > 
> > From source the name of the async function is something like
> > "testMain_async_body", via kernel it's just "async_op", so without
> includeOwner
> > I get the following error:
> > "Expected to be in function testMain but actually in function async_op".
> 
> The function names reported via kernel and the VM today need to be the same.

I actually think they are today. It was done a week or so ago. I'll remove the
includeOwner stuff.
It's not currently easy to test though, as the switch to fasta removed all
debugging support.

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

https://codereview.chromium.org/2680303002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:9233: for (intptr_t i = offset + 1; i < source.Length();
++i, ++*token_len) {
On 2017/03/06 16:58:55, hausner wrote:
> I find this hard to read, since the for loop more or less does what the loop
> body should do, and the body contains half the termination condition.
> 
> Assuming that source.CharAt() returns 0 as the last character in the source,
the
> loop could be rewritten as:
> 
> for (intptr_t i = offset + 1; Scanner::IsIdentChar(source.CharAt(i)); ++i) {
>   ++*token_len;
> }

Done.

[jensj, 2017-03-07 08:44:06]

Description was changed from

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with (before fasta)
    ```
    python tools/test.py -mrelease -cdartk --no-tree-shake --no-dfe service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

As fasta has now landed debugging does not currently work, but this is still
needed in order to both test better and allow for proper kernel debugging
support.
==========

to

==========
Kernel debugging; service tests

- Offsets on more stuff
- Status file for kernel
 -> Testing with (before fasta)
    ```
    python tools/test.py -mrelease -cdartk --no-tree-shake --no-dfe service
    ```
    now shows everything as passing.
    Reload stuff, and evaluation stuff (e.g.) has been disabled though.
- Lots of new service tests that tests what positions we stop at when
  saying next.

As fasta has now landed debugging does not currently work, but this is still
needed in order to both test better and allow for proper kernel debugging
support.

R=hausner@google.com, kmillikin@google.com

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

[jensj, 2017-03-07 08:44:08]

Committed patchset #7 (id:120001) manually as
9837563cd18e624761cd1521c4b1b3f755368a0f (presubmit successful).

[Cutch, 2017-03-07 17:20:42]

On 2017/03/07 08:43:40, jensj wrote:
> In regards to the 2 issues John brought up:
> 
> 1) I have removed the branches and marked them as skip in kernel for now. This
> has to change anyway, as we've moved to fasta and the kernel isolate. How it
is
> addressed once fasta-debugging gets up-to-speed will be an issue for another
CL.
> I thus see that issue as 'closed' here.
> 

SGTM.

> 2) I'm rather sure this was fixed about a week ago, but it cannot currently be
> tested because of the switch to fasta. I have removed the includeOwner stuff.
> Thanks for pointing that out.
> 
> In total this means that all changes to existing tests have been canceled and
> only additions remain.
> That should address all concerns that John brought up, leaving me 3 lgtm's.
> Landing.
> 

Thanks!