[debugger] introduce abstract interface for break location.

[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Future changes will introduce an
implementation to iterate bytecode arrays.

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

Committed: https://crrev.com/24b40f35f43b7f1f0e8427720b3409b28119ee32
Cr-Commit-Position: refs/heads/master@{#33869}

[Yang, 2016-02-10 11:03:43]

Description was changed from

==========
[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Following changes will introduce an
implementation to iterate bytecode arrays.

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

to

==========
[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Future changes will introduce an
implementation to iterate bytecode arrays.

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

[Yang, 2016-02-10 11:04:01]

Ross, please rubberstamp src/interpreter/

[rmcilroy, 2016-02-10 12:24:25]

interpreter/ lgtm. Didn't look at the rest.

[vogelheim, 2016-02-10 12:36:44]

lgtm

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

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode65
src/debug/debug.cc:65: code_offset_(code_offset),
rmode_ ?

rmode_ is still in the header. Either remove from the header if unused, or
initialize to some reasonable default, or have a really good reason why it
should remain uninitialized.

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode65
src/debug/debug.cc:65: code_offset_(code_offset),
Same with data_

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode92
src/debug/debug.cc:92: if (!Done()) Next();
Not sure I get this... but this looks kinda weird:

So the underlying iterator implements Done()/Next() in a way that a freshly
constructed is in an 'empty' state and needs to be Next()-ed before first use.
This iterator however exposes a semantic where the iterator immediately points
to the first element, and Next()-ing it right away would skip the first element.
Both look the same, having an interface of Done()/Next().

No idea what the right thing is, but it does look funny...

-----
If I get a choice: I prefer the new-iterator-points-at-first-element style,
since that fits nicely with the C++ for-construct.

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode221
src/debug/debug.cc:221: }
Why not keep the for loop?

[That's totally a matter of taste; decide as you see fit.]
[Here & elsewhere in the file.]

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.h#newcode154
src/debug/debug.h:154: ~CodeIterator() override{};
style nitpick: space between override and {}

[Yang, 2016-02-10 13:06:38]

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

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode65
src/debug/debug.cc:65: code_offset_(code_offset),
On 2016/02/10 12:36:44, vogelheim wrote:
> rmode_ ?
> 
> rmode_ is still in the header. Either remove from the header if unused, or
> initialize to some reasonable default, or have a really good reason why it
> should remain uninitialized.

Thanks. I forgot about these. Done.

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode92
src/debug/debug.cc:92: if (!Done()) Next();
On 2016/02/10 12:36:44, vogelheim wrote:
> Not sure I get this... but this looks kinda weird:
> 
> So the underlying iterator implements Done()/Next() in a way that a freshly
> constructed is in an 'empty' state and needs to be Next()-ed before first use.
> This iterator however exposes a semantic where the iterator immediately points
> to the first element, and Next()-ing it right away would skip the first
element.
> Both look the same, having an interface of Done()/Next().
> 
> No idea what the right thing is, but it does look funny...
> 
> -----
> If I get a choice: I prefer the new-iterator-points-at-first-element style,
> since that fits nicely with the C++ for-construct.

Yes. The CodeIterator mainly acts as an adaptor to the RelocIterator. I want to
keep the current semantic of having the iterator already advance to the first
element.

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.cc#newcode221
src/debug/debug.cc:221: }
On 2016/02/10 12:36:44, vogelheim wrote:
> Why not keep the for loop?
> 
> [That's totally a matter of taste; decide as you see fit.]
> [Here & elsewhere in the file.]

Done. I sort of disliked having a line break in the for-statement :P

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.h
File src/debug/debug.h (right):

https://codereview.chromium.org/1682853003/diff/1/src/debug/debug.h#newcode154
src/debug/debug.h:154: ~CodeIterator() override{};
On 2016/02/10 12:36:44, vogelheim wrote:
> style nitpick: space between override and {}

Done.

[Yang, 2016-02-10 13:15:22]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-10 13:15:23]

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/1682853003/#ps20001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-02-10 13:15:25]

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

[commit-bot: I haz the power, 2016-02-10 13:18:28]

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

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

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

[Yang, 2016-02-10 13:29:38]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-10 13:29:39]

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/1682853003/#ps40001
(title: "fix compile")

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

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

[commit-bot: I haz the power, 2016-02-10 13:34:51]

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

[commit-bot: I haz the power, 2016-02-10 13:34:52]

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

[Yang, 2016-02-10 13:54:27]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-02-10 13:54:28]

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/1682853003/#ps60001
(title: "revert stray edit")

[commit-bot: I haz the power, 2016-02-10 13:54:41]

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

[commit-bot: I haz the power, 2016-02-10 14:19:38]

Description was changed from

==========
[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Future changes will introduce an
implementation to iterate bytecode arrays.

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

to

==========
[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Future changes will introduce an
implementation to iterate bytecode arrays.

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

[commit-bot: I haz the power, 2016-02-10 14:19:38]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Future changes will introduce an
implementation to iterate bytecode arrays.

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

to

==========
[debugger] introduce abstract interface for break location.

The break location heavily relies on relocation info. This change
abstracts that away. Currently there is only one implementation for
this interface, for JIT code. Future changes will introduce an
implementation to iterate bytecode arrays.

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

Committed: https://crrev.com/24b40f35f43b7f1f0e8427720b3409b28119ee32
Cr-Commit-Position: refs/heads/master@{#33869}
==========

[commit-bot: I haz the power, 2016-02-10 14:20:15]

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