[interpreter, debugger] implement bytecode break location iterator.

[interpreter, debugger] implement bytecode break location iterator.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4690
LOG=N

Committed: https://crrev.com/92498901247fd396093fe4997ba4102940670dcb
Cr-Commit-Position: refs/heads/master@{#33904}

[rmcilroy, 2016-02-11 12:32:03]

LGTM once kCallWide/kNewWide comment is addressed.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:75: }
nit - make this an else? Seems lopsided to me to make it a fallthrough given
that it is an either / or type of thing

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:193: DCHECK_EQ(CALLS_AND_RETURNS, break_locator_type_);
nit - new line above

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:212: case interpreter::Bytecode::kCall:
You need to list KCallWide and kNewWide as well (and Mythri is currently adding
CallIC and CallICWide soon  too). Maybe you could just add a helper to the
Bytecodes class for IsCallOrNew(Bytecode...)?

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:194: interpreter::SourcePositionTableIterator iterator_;
nit - source_position_iterator_

[vogelheim, 2016-02-11 12:33:59]

lgtm, but I'm a bit confused at the return statement business.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:183: ~BytecodeArrayIterator() override{};
nitpick: whitespace between override and {} ?
nitpick 2: No semicolon at end of line?

[I toyed around with this: With the semicolon, clang-format will remove the
whitespace. Without it, it will put it back. Weird. Either way, I think both
nitpicks are ok. :) ]

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1081: SetReturnPosition(literal);
I find the SetReturnPosition handling a bit odd. Not sure I understand how this
works, so feel free to ignore. By my reading:

- BytecodeArrayBuilder::SetReturnPosition will add a statement position to the
end of the function. This is independent of actual "return" statements in the
code: it should always be there.
- It's called for each actual return, and if there was none in the source, then
EnsureReturn will synthesize a return statement and call SetReturnPosition for
it.
- The source position table builder has logic to discard multiple return
positions, because for a function with multiple "return"s in the source, we'd
call SetReturnPosition multiple times, each time adding the same statement
position.

Wouldn't it be much simpler to just discard all of that, and instead have some
sort of 'finalize' call that will add the final statement position if it isn't
already there, and that must be called once at the end of every bytecode array
build?

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/source-...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/source-...
src/interpreter/source-position-table.h:38: bool AlreadyHasPosition(int
bytecode_offset) {
AlreadyHasPosition is somewhat weaker than AssertMonotonic. Can we no longer
guarantee that the bytecode offsets are monotonically increasing?

[Yang, 2016-02-11 14:16:55]

addressed comments.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:75: }
On 2016/02/11 12:32:03, rmcilroy wrote:
> nit - make this an else? Seems lopsided to me to make it a fallthrough given
> that it is an either / or type of thing

Done.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:193: DCHECK_EQ(CALLS_AND_RETURNS, break_locator_type_);
On 2016/02/11 12:32:03, rmcilroy wrote:
> nit - new line above

Done.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:212: case interpreter::Bytecode::kCall:
On 2016/02/11 12:32:03, rmcilroy wrote:
> You need to list KCallWide and kNewWide as well (and Mythri is currently
adding
> CallIC and CallICWide soon  too). Maybe you could just add a helper to the
> Bytecodes class for IsCallOrNew(Bytecode...)?

Done.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:183: ~BytecodeArrayIterator() override{};
On 2016/02/11 12:33:59, vogelheim wrote:
> nitpick: whitespace between override and {} ?
> nitpick 2: No semicolon at end of line?
> 
> [I toyed around with this: With the semicolon, clang-format will remove the
> whitespace. Without it, it will put it back. Weird. Either way, I think both
> nitpicks are ok. :) ]

Ah yes. Makes sense. I simply let clang-format handle it, while mistakenly
having semicolon at the end.

https://codereview.chromium.org/1682853004/diff/20001/src/debug/debug.h#newco...
src/debug/debug.h:194: interpreter::SourcePositionTableIterator iterator_;
On 2016/02/11 12:32:03, rmcilroy wrote:
> nit - source_position_iterator_

Done.

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1081: SetReturnPosition(literal);
On 2016/02/11 12:33:59, vogelheim wrote:
> I find the SetReturnPosition handling a bit odd. Not sure I understand how
this
> works, so feel free to ignore. By my reading:
> 
> - BytecodeArrayBuilder::SetReturnPosition will add a statement position to the
> end of the function. This is independent of actual "return" statements in the
> code: it should always be there.
> - It's called for each actual return, and if there was none in the source,
then
> EnsureReturn will synthesize a return statement and call SetReturnPosition for
> it.
> - The source position table builder has logic to discard multiple return
> positions, because for a function with multiple "return"s in the source, we'd
> call SetReturnPosition multiple times, each time adding the same statement
> position.
> 
> Wouldn't it be much simpler to just discard all of that, and instead have some
> sort of 'finalize' call that will add the final statement position if it isn't
> already there, and that must be called once at the end of every bytecode array
> build?

This simply reflects what Fullcodegen does. All return statements have the
source position at the end of the function.

The source position table has logic to discard multiple source positions for the
same code offset. Having multiple code offsets having the same source position
is fine. In this case, we have multiple return positions in the bytecode refer
to the same source position.

The finalize call you suggest would not do the same. Any return statements in
the middle of the code would not have a source position attached.

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/source-...
File src/interpreter/source-position-table.h (right):

https://codereview.chromium.org/1682853004/diff/20001/src/interpreter/source-...
src/interpreter/source-position-table.h:38: bool AlreadyHasPosition(int
bytecode_offset) {
On 2016/02/11 12:33:59, vogelheim wrote:
> AlreadyHasPosition is somewhat weaker than AssertMonotonic. Can we no longer
> guarantee that the bytecode offsets are monotonically increasing?

Correct. We can no longer guarantee that. For example, VisitExpressionStatement
may not emit bytecode if it's an empty statement. In this situation, FCG does
not emit a new position (see PositionsRecorder::WriteRecordedPositions), so we
imitate this behavior.

I'm sure we need to tweak this. The logic for this in FCG is pretty complicated,
and I'm not sure how much of that we need to replicate.

[Yang, 2016-02-11 14:17:21]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-11 14:17:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1682853004/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2016-02-11 14:17:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1682853004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1682853004/40001

[commit-bot: I haz the power, 2016-02-11 14:21:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-11 14:21:13]

Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)

[Yang, 2016-02-11 14:24:36]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-11 14:24:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/1682853004/#ps60001
(title: "fix build")

[commit-bot: I haz the power, 2016-02-11 14:24:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1682853004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1682853004/60001

[commit-bot: I haz the power, 2016-02-11 15:18:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-11 15:33:14]

Description was changed from

==========
[interpreter, debugger] implement bytecode break location iterator.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4690
LOG=N
==========

to

==========
[interpreter, debugger] implement bytecode break location iterator.

R=rmcilroy@chromium.org, vogelheim@chromium.org
BUG=v8:4690
LOG=N

Committed: https://crrev.com/92498901247fd396093fe4997ba4102940670dcb
Cr-Commit-Position: refs/heads/master@{#33904}
==========

[commit-bot: I haz the power, 2016-02-11 15:33:15]

Patchset 4 (id:??) landed as
https://crrev.com/92498901247fd396093fe4997ba4102940670dcb
Cr-Commit-Position: refs/heads/master@{#33904}