[debugger] fix break locations for assignments and return.

[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.

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

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

[Yang, 2016-02-29 13:57:03]

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

[commit-bot: I haz the power, 2016-02-29 13:57:10]

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

[commit-bot: I haz the power, 2016-02-29 14:10:32]

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

[commit-bot: I haz the power, 2016-02-29 14:10:32]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14482)

[Yang, 2016-02-29 14:36:43]

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

[commit-bot: I haz the power, 2016-02-29 14:36:55]

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

[vogelheim, 2016-02-29 14:52:32]

I'm confused about the logic here. I'm easily confused, so that might be just
me. :)  Could you briefly comment on what the intent is?

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

https://codereview.chromium.org/1744123003/diff/1/src/debug/debug.cc#newcode87
src/debug/debug.cc:87: int BreakLocation::Iterator::ReturnPosition() {
name nitpick: I would have expected 'return position' to be the position of a
(literal) return statement. But it's really a sort of
last-logical-position-of-a-function position. Not really sure there's a more
elegant name for this, though... :-/

https://codereview.chromium.org/1744123003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1744123003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1047: void
BytecodeArrayBuilder::EnsureReturn(FunctionLiteral* literal) {
Remove literal parameter?

https://codereview.chromium.org/1744123003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1187: if (return_position_ ==
RelocInfo::kNoPosition) return;
Is this ever supposed to happen?

https://codereview.chromium.org/1744123003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1190: return_position_);
I'm confused at this logic. (Not specifically this part, but the general
idea...)

Is the idea to always set a return position at the end of the function, or is
the plan to set a return position to wherever returns are, but make sure there's
at least one (w/ end of the function being the default)?

If the former, we could just set if once & unconditionally when generating the
table.

If the latter, then I don't think this code does what it's meant to. This will
always set a statement position at the end (return_position_), regardless of
whether it's set via the first Return() call or is the default via
EnsureReturnPosition(). Also, I'd suspect in this case we'd want to set a
statement for each actual Return(), while this would set it only for the first
(since exit_seen_in_block_ will be true after the first Return().

[commit-bot: I haz the power, 2016-02-29 14:58:35]

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

[commit-bot: I haz the power, 2016-02-29 14:58:36]

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

[Yang, 2016-02-29 15:19:24]

On 2016/02/29 14:58:36, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The idea is to match full codegen more closely. What FCG does is this: it emits
a debug break location before the return statement, including the evaluation of
the return value. The source position of this is the position of the return
statement. After the value is evaluated, we jump to into the return sequence,
which contains another break location. The source position of that break is the
end of the function.

What bytecode currently does is to attach the source position of the return
statement to explicit return statements and the position of the end of the
function to the implicit return. This works for some tests, but obviously not
for all.

What this change does is to add a statement position for the bytecode at the
return value evaluation to break before the return statement, and another
position at the return bytecode, which is the bytecode equivalent of FCG's
return sequence, to break at the end of the function.

Alternatively to storing the end of function position, we could also store a
special position byte pattern, in order to avoid inefficient encoding due to
jumping position delta.

Does that clarify?

[vogelheim, 2016-02-29 17:43:30]

lgtm

Thanks for the explanation; I understand this much better now. Also, please
ignore all comments in previous mails. Here's 2 new ones, though. :)

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

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1187: if (return_position_ ==
RelocInfo::kNoPosition) return;
DCHECK? Or is this supposed to happen?

(Initialize is called from MakeBytecode, so I'd expect this condition to never
be true.)

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:882:
builder()->SetStatementPosition(stmt);
Just for my understanding: These switched lines is what sets the first
breakpoint, at (before) the return value evaluation?

[Yang, 2016-02-29 17:53:12]

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

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1187: if (return_position_ ==
RelocInfo::kNoPosition) return;
On 2016/02/29 17:43:30, vogelheim wrote:
> DCHECK? Or is this supposed to happen?
> 
> (Initialize is called from MakeBytecode, so I'd expect this condition to never
> be true.)

I didnt want to add the initialize call to the about 100 uses of the bytecode
array builder in tests :p

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:882:
builder()->SetStatementPosition(stmt);
On 2016/02/29 17:43:30, vogelheim wrote:
> Just for my understanding: These switched lines is what sets the first
> breakpoint, at (before) the return value evaluation?

Yes, correct.

[rmcilroy, 2016-03-01 04:47:01]

Only looked at interpreter stuff. LGTM with a comment.

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

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1180: void
BytecodeArrayBuilder::InitializeReturnPosition(FunctionLiteral* literal) {
I'm not particularly keen on this being a separate function. Could we instead
set it in the constructor? I would be fine with return_position_ being set to
kNoPosition if the literal passed to the constructor is nullptr if that would
help with tests.

[Yang, 2016-03-01 08:09:18]

https://codereview.chromium.org/1744123003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.cc (right):

https://codereview.chromium.org/1744123003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.cc:1047: void
BytecodeArrayBuilder::EnsureReturn(FunctionLiteral* literal) {
On 2016/02/29 14:52:32, vogelheim wrote:
> Remove literal parameter?

Done.

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

https://codereview.chromium.org/1744123003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.cc:1180: void
BytecodeArrayBuilder::InitializeReturnPosition(FunctionLiteral* literal) {
On 2016/03/01 04:47:01, rmcilroy (Slow - Traveling) wrote:
> I'm not particularly keen on this being a separate function. Could we instead
> set it in the constructor? I would be fine with return_position_ being set to
> kNoPosition if the literal passed to the constructor is nullptr if that would
> help with tests. 

Done.

[Yang, 2016-03-01 08:09:26]

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

[commit-bot: I haz the power, 2016-03-01 08:09:39]

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

[commit-bot: I haz the power, 2016-03-01 09:01:04]

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

[commit-bot: I haz the power, 2016-03-01 09:01:05]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-03-01 12:33:55]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-03-01 12:33:55]

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/1744123003/#ps40001
(title: "fixes and addressed comments")

[commit-bot: I haz the power, 2016-03-01 12:33:59]

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

[commit-bot: I haz the power, 2016-03-01 12:40:16]

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

[commit-bot: I haz the power, 2016-03-01 12:40:17]

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

[Yang, 2016-03-01 12:42:13]

Description was changed from

==========
[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.


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

to

==========
[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.


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

[Yang, 2016-03-01 12:42:24]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2016-03-01 12:42:40]

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

[commit-bot: I haz the power, 2016-03-01 12:47:30]

Description was changed from

==========
[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.


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

to

==========
[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.


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

[commit-bot: I haz the power, 2016-03-01 12:47:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 12:48:04]

Description was changed from

==========
[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.


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

to

==========
[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.

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

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

[commit-bot: I haz the power, 2016-03-01 12:48:06]

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