[kernel] Debugging of switch statement

[kernel] Debugging of switch statement

BUG=
R=ahe@google.com, kmillikin@google.com

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

[jensj, 2017-03-15 08:52:41]

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

[jensj, 2017-03-15 08:52:41]

Turns out debugging of switch cases via kernel was rather lacking.
This (at least together with other CL with fasta offsets) can gets us to the
same place as source-based debugging (at least they both pass the same service
test, testing switching on string) (upcoming CL).

There will probably still be missed cases left because we can switch on const
expressions, but at least int and string (probably the two most likely usages)
works. Switching on const list for instance also works though.

[Kevin Millikin (Google), 2017-03-15 11:13:49]

In the binary encoding, I think it would be better to have a List<Uint> of
expression offsets that parallels the List<Expression> in SwitchCase.

Could you try that?

[ahe, 2017-03-15 11:29:41]

Fasta changes: LGTM

[jensj, 2017-03-15 14:41:41]

I've taken a stab at Kevins suggestion.
PTAL.

[Kevin Millikin (Google), 2017-03-20 12:29:48]

LGTM with comments.

https://codereview.chromium.org/2750013002/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2750013002/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1025: List<int>
expressionsOffsets = <int>[];
Do this in the loop above.  expressionsOffsets ==> expressionOffsets.

https://codereview.chromium.org/2750013002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2750013002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2091: List<int>
expressionsOffsets = <int>[];
expressionsOffsets ==> expressionOffsets

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

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:746: List<FileOffset> expressionsOffsets; // 1-to-1 with
expressions.
expressionsOffsets ==> expressionOffsets

Since this is a parallel list there's no reason to encode the length separately.
 We could also have:

List<Expression> expressions;
FileOffset[expressions.length] offsets;

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

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3070: final List<int> expressionsOffsets;
expressionsOffsets ==> expressionOffsets

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:911:
caseNode.expressionsOffsets.length = readUInt();
I'd just change _fillNonTreeNodeList so it didn't have a type parameter of Node
and use that.  You might adjust the name if you thought it was ambiguous.

_fillNonTreeNodeList(caseNode.expressionOffsets, readOffset);

https://codereview.chromium.org/2750013002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2750013002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1598: // List of expression offsets of same length
as expressions_: This serves as
I think it's obvious that it serves as the positions of the expression.  And
it's equally useful in all cases.  Don't editorialize too much in comments.

If you think it's confusing and needs a remark, you could say that we wouldn't
need this if all constant expressions had a position.

[jensj, 2017-03-21 10:06:20]

https://codereview.chromium.org/2750013002/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_visitor.dart (right):

https://codereview.chromium.org/2750013002/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_visitor.dart:1025: List<int>
expressionsOffsets = <int>[];
On 2017/03/20 12:29:47, Kevin Millikin (Google) wrote:
> Do this in the loop above.  expressionsOffsets ==> expressionOffsets.

Done.

https://codereview.chromium.org/2750013002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2750013002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2091: List<int>
expressionsOffsets = <int>[];
On 2017/03/20 12:29:47, Kevin Millikin (Google) wrote:
> expressionsOffsets ==> expressionOffsets

Done.

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

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/binary.md#ne...
pkg/kernel/binary.md:746: List<FileOffset> expressionsOffsets; // 1-to-1 with
expressions.
On 2017/03/20 12:29:47, Kevin Millikin (Google) wrote:
> expressionsOffsets ==> expressionOffsets
> 
> Since this is a parallel list there's no reason to encode the length
separately.
>  We could also have:
> 
> List<Expression> expressions;
> FileOffset[expressions.length] offsets;

Done.

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

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3070: final List<int> expressionsOffsets;
On 2017/03/20 12:29:47, Kevin Millikin (Google) wrote:
> expressionsOffsets ==> expressionOffsets

Done.

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2750013002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:911:
caseNode.expressionsOffsets.length = readUInt();
On 2017/03/20 12:29:47, Kevin Millikin (Google) wrote:
> I'd just change _fillNonTreeNodeList so it didn't have a type parameter of
Node
> and use that.  You might adjust the name if you thought it was ambiguous.
> 
> _fillNonTreeNodeList(caseNode.expressionOffsets, readOffset);

I've changed it to not encode the size as well so that won't work anymore.

https://codereview.chromium.org/2750013002/diff/20001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2750013002/diff/20001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1598: // List of expression offsets of same length
as expressions_: This serves as
On 2017/03/20 12:29:47, Kevin Millikin (Google) wrote:
> I think it's obvious that it serves as the positions of the expression.  And
> it's equally useful in all cases.  Don't editorialize too much in comments.
> 
> If you think it's confusing and needs a remark, you could say that we wouldn't
> need this if all constant expressions had a position.

Comment removed.

[jensj, 2017-03-21 10:36:55]

Description was changed from

==========
[kernel] Debugging of switch statement

BUG=
==========

to

==========
[kernel] Debugging of switch statement

BUG=
R=ahe@google.com, kmillikin@google.com

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

[jensj, 2017-03-21 10:36:56]

Committed patchset #3 (id:40001) manually as
a1a34a1c84ab7a36c3eed27b835dfbc9490b93d2 (presubmit successful).