[interpreter, debugger] support debug breaks via bytecode array copy

[interpreter, debugger] support debug breaks via bytecode array copy

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

Committed: https://crrev.com/e032a98d3d1c532716892d2aad0e9d4467acae61
Cr-Commit-Position: refs/heads/master@{#34190}

[Yang, 2016-02-16 09:22:21]

This is the implementation of what we discussed. I only ported it to ia32 to get
a first round of comments.

https://codereview.chromium.org/1703453002/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/debug/debug.cc#newcode406
src/debug/debug.cc:406: BytecodeArray* bytecode_array =
abstract_code()->GetBytecodeArray();
We may eventually want to move to a representation where the break points are
saved in a list of BreakPoint objects on the debug info, and changing break
points is just a matter of operating on that list and at the end applying those
break points to the bytecode array.

Patching the original bytecode array would then at the end require the debugger
to refresh the copy in the debug info and refresh the break points from
BreakPoint list.

But for now let's just mirror what we do for FCG.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:901:
builder()->SetStatementPosition(stmt);
Turns out I've been wrong about setting the return position here before. This
here matches what FCG does.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:76: OperandType::kReg16, OperandType::kReg16)
These are more than I originally thought. But we do have instructions of these
sizes. See for example CallRuntimeForPairWide, which has 4 16-bit operands.

[rmcilroy, 2016-02-16 10:46:39]

Looks good overall, just a few nits.

https://codereview.chromium.org/1703453002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1485: void
BytecodeGraphBuilder::Visit##Name() { CHECK(false); }
/s/CHECK(false)/UNREACHABLE()/

https://codereview.chromium.org/1703453002/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/debug/debug.cc#newcode1018
src/debug/debug.cc:1018: if (!frame->is_interpreted()) call_offset--;
Could you extract this code into a helper - it's repeated in a couple of places.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:901:
builder()->SetStatementPosition(stmt);
On 2016/02/16 09:22:20, Yang wrote:
> Turns out I've been wrong about setting the return position here before. This
> here matches what FCG does.

Ack. Note: we will only get a SetReturnPosition now where there is an implicit
return - just making sure this is what we want.

Should we make SetReturnPosition private to ensure we only set it on implicit
returns?

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:84: return Bytecode::kLdaUndefined;
UNREACHABLE() and return static_cast<Bytecode>(-1);

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:286: if (bytecode == Bytecode::k##Name) return
true;
Could you do this as a switch statement instead please

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:76: OperandType::kReg16, OperandType::kReg16)
On 2016/02/16 09:22:20, Yang wrote:
> These are more than I originally thought. But we do have instructions of these
> sizes. See for example CallRuntimeForPairWide, which has 4 16-bit operands.

Yeah, I realized this yesterday after leaving work. I guess this still works for
now. Could you name them by operand size (DebugBreak0 - DebugBreak8) please.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:450: TailCall(descriptor,
exit_trampoline_code_object, args, 0);
Could you make InterpreterReturn (and possibly use DispatchTo if you add
new_bytecode_offset as an argument) call DispatchToHandler().

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.h:134: // Dispatch to handler.
/s/handler/bytecode handler/

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:39: Code* GetHandler(Bytecode bytecode);
GetBytecodeHandler and add a comment please

[Yang, 2016-02-19 13:09:18]

Addressed comments and added platform ports.

https://codereview.chromium.org/1703453002/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1485: void
BytecodeGraphBuilder::Visit##Name() { CHECK(false); }
On 2016/02/16 10:46:38, rmcilroy wrote:
> /s/CHECK(false)/UNREACHABLE()/

Done.

https://codereview.chromium.org/1703453002/diff/1/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/debug/debug.cc#newcode1018
src/debug/debug.cc:1018: if (!frame->is_interpreted()) call_offset--;
On 2016/02/16 10:46:38, rmcilroy wrote:
> Could you extract this code into a helper - it's repeated in a couple of
places.

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:901:
builder()->SetStatementPosition(stmt);
On 2016/02/16 10:46:38, rmcilroy wrote:
> On 2016/02/16 09:22:20, Yang wrote:
> > Turns out I've been wrong about setting the return position here before.
This
> > here matches what FCG does.
> 
> Ack. Note: we will only get a SetReturnPosition now where there is an implicit
> return - just making sure this is what we want.
> 
> Should we make SetReturnPosition private to ensure we only set it on implicit
> returns?

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.cc
File src/interpreter/bytecodes.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:84: return Bytecode::kLdaUndefined;
On 2016/02/16 10:46:38, rmcilroy wrote:
> UNREACHABLE() and return static_cast<Bytecode>(-1);

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.c...
src/interpreter/bytecodes.cc:286: if (bytecode == Bytecode::k##Name) return
true;
On 2016/02/16 10:46:38, rmcilroy wrote:
> Could you do this as a switch statement instead please

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:76: OperandType::kReg16, OperandType::kReg16)
On 2016/02/16 10:46:38, rmcilroy wrote:
> On 2016/02/16 09:22:20, Yang wrote:
> > These are more than I originally thought. But we do have instructions of
these
> > sizes. See for example CallRuntimeForPairWide, which has 4 16-bit operands.
> 
> Yeah, I realized this yesterday after leaving work. I guess this still works
for
> now. Could you name them by operand size (DebugBreak0 - DebugBreak8) please.

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:450: TailCall(descriptor,
exit_trampoline_code_object, args, 0);
On 2016/02/16 10:46:38, rmcilroy wrote:
> Could you make InterpreterReturn (and possibly use DispatchTo if you add
> new_bytecode_offset as an argument) call DispatchToHandler().

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.h:134: // Dispatch to handler.
On 2016/02/16 10:46:38, rmcilroy wrote:
> /s/handler/bytecode handler/

Done.

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter.h
File src/interpreter/interpreter.h (right):

https://codereview.chromium.org/1703453002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter.h:39: Code* GetHandler(Bytecode bytecode);
On 2016/02/16 10:46:38, rmcilroy wrote:
> GetBytecodeHandler and add a comment please 

Done.

[rmcilroy, 2016-02-19 14:47:16]

Neat! lgtm with one final comment, thanks.

https://codereview.chromium.org/1703453002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1703453002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:429: if (bytecode_offset == nullptr)
bytecode_offset = BytecodeOffset();
Could you avoid this and the default argument type and just pass
BytecodeOffset() explicitly where necessary please (either make BytecodeOffset
public or add a 1 arg variant of DispatchToBytecodeHandler which calls this one
with BytecodeOffset() as the second arg).

[Yang, 2016-02-19 15:19:11]

On 2016/02/19 14:47:16, rmcilroy wrote:
> Neat! lgtm with one final comment, thanks.
> 
>
https://codereview.chromium.org/1703453002/diff/40001/src/interpreter/interpr...
> File src/interpreter/interpreter-assembler.cc (right):
> 
>
https://codereview.chromium.org/1703453002/diff/40001/src/interpreter/interpr...
> src/interpreter/interpreter-assembler.cc:429: if (bytecode_offset == nullptr)
> bytecode_offset = BytecodeOffset();
> Could you avoid this and the default argument type and just pass
> BytecodeOffset() explicitly where necessary please (either make BytecodeOffset
> public or add a 1 arg variant of DispatchToBytecodeHandler which calls this
one
> with BytecodeOffset() as the second arg).

Addressed.

[Yang, 2016-02-19 15:19:51]

Michael, do you want to take a look? I need you to at least look at the
src/heap/ changes. Thanks!

[Yang, 2016-02-19 15:19:58]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 15:20:04]

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

[commit-bot: I haz the power, 2016-02-19 15:53:49]

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

[commit-bot: I haz the power, 2016-02-19 15:53:50]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-02-22 09:37:49]

LGTM. Nice! Just some minor comments.

https://codereview.chromium.org/1703453002/diff/60001/src/debug/debug-frames.cc
File src/debug/debug-frames.cc (right):

https://codereview.chromium.org/1703453002/diff/60001/src/debug/debug-frames....
src/debug/debug-frames.cc:75: // TODO(yangguo): Get the bytecode array from the
frame.
Not sure about this TODO, why do we need to get the bytecode array from the
frame? The source position information should be the same for all versions that
could be active in the frame. That's one reason why I am so keen on preserving
the structure (i.e. same jump offsets, same bytecode offsets) of the bytecode
when we create the patched copy. I think everything we need to reason about the
bytecode should not depend on which version is active in the frame. WDYT? Am I
missing somehting here?

https://codereview.chromium.org/1703453002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1703453002/diff/60001/src/objects.h#newcode4476
src/objects.h:4476: void CopyTo(BytecodeArray* to);
nit: Can we call this "CopyBytecodesTo" instead? That would make it clear that
we are only copying the bytecode payload here, not the header fields.

https://codereview.chromium.org/1703453002/diff/60001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (left):

https://codereview.chromium.org/1703453002/diff/60001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:338: DCHECK(SmiValuesAre31Bits());  // Only available
for 31-bit SMI.
Why did we need to remove this check? I seem to be missing something.

[Michael Starzinger, 2016-02-22 09:37:49]

LGTM. Nice! Just some minor comments.

https://codereview.chromium.org/1703453002/diff/60001/src/debug/debug-frames.cc
File src/debug/debug-frames.cc (right):

https://codereview.chromium.org/1703453002/diff/60001/src/debug/debug-frames....
src/debug/debug-frames.cc:75: // TODO(yangguo): Get the bytecode array from the
frame.
Not sure about this TODO, why do we need to get the bytecode array from the
frame? The source position information should be the same for all versions that
could be active in the frame. That's one reason why I am so keen on preserving
the structure (i.e. same jump offsets, same bytecode offsets) of the bytecode
when we create the patched copy. I think everything we need to reason about the
bytecode should not depend on which version is active in the frame. WDYT? Am I
missing somehting here?

https://codereview.chromium.org/1703453002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1703453002/diff/60001/src/objects.h#newcode4476
src/objects.h:4476: void CopyTo(BytecodeArray* to);
nit: Can we call this "CopyBytecodesTo" instead? That would make it clear that
we are only copying the bytecode payload here, not the header fields.

https://codereview.chromium.org/1703453002/diff/60001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (left):

https://codereview.chromium.org/1703453002/diff/60001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:338: DCHECK(SmiValuesAre31Bits());  // Only available
for 31-bit SMI.
Why did we need to remove this check? I seem to be missing something.

[Yang, 2016-02-22 12:45:59]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-22 12:46:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1703453002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-22 12:46:17]

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

[Yang, 2016-02-22 12:48:44]

https://codereview.chromium.org/1703453002/diff/60001/src/debug/debug-frames.cc
File src/debug/debug-frames.cc (right):

https://codereview.chromium.org/1703453002/diff/60001/src/debug/debug-frames....
src/debug/debug-frames.cc:75: // TODO(yangguo): Get the bytecode array from the
frame.
On 2016/02/22 09:37:49, Michael Starzinger wrote:
> Not sure about this TODO, why do we need to get the bytecode array from the
> frame? The source position information should be the same for all versions
that
> could be active in the frame. That's one reason why I am so keen on preserving
> the structure (i.e. same jump offsets, same bytecode offsets) of the bytecode
> when we create the patched copy. I think everything we need to reason about
the
> bytecode should not depend on which version is active in the frame. WDYT? Am I
> missing somehting here?

Absolutely correct. This is a stale TODO I forgot to remove.

https://codereview.chromium.org/1703453002/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1703453002/diff/60001/src/objects.h#newcode4476
src/objects.h:4476: void CopyTo(BytecodeArray* to);
On 2016/02/22 09:37:49, Michael Starzinger wrote:
> nit: Can we call this "CopyBytecodesTo" instead? That would make it clear that
> we are only copying the bytecode payload here, not the header fields.

Done.

https://codereview.chromium.org/1703453002/diff/60001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (left):

https://codereview.chromium.org/1703453002/diff/60001/src/x64/assembler-x64.h...
src/x64/assembler-x64.h:338: DCHECK(SmiValuesAre31Bits());  // Only available
for 31-bit SMI.
On 2016/02/22 09:37:49, Michael Starzinger wrote:
> Why did we need to remove this check? I seem to be missing something.

As discussed offline, this constructor is only intended for x32. Reverting this
part of the change.

[commit-bot: I haz the power, 2016-02-22 13:16:52]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-22 13:17:57]

Description was changed from

==========
[interpreter, debugger] support debug breaks via bytecode array copy


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

to

==========
[interpreter, debugger] support debug breaks via bytecode array copy

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

Committed: https://crrev.com/e032a98d3d1c532716892d2aad0e9d4467acae61
Cr-Commit-Position: refs/heads/master@{#34190}
==========

[commit-bot: I haz the power, 2016-02-22 13:17:57]

Patchset 6 (id:??) landed as
https://crrev.com/e032a98d3d1c532716892d2aad0e9d4467acae61
Cr-Commit-Position: refs/heads/master@{#34190}