[debugger] separate break point info from code instrumentation.

[debugger] separate break point info from code instrumentation.

Previously, we would both instrument the code, and add/remove
BreakPointInfo objects through BreakLocation. This is bad design and
unsuitable for having two different code kinds.

We would now add/remove BreakPointInfo objects, and use that as source
of truth when instrumenting the code. If we have both bytecode and FCG
code, we would simply apply these break points twice to either.

Notable changes:
- Removed many functionality from BreakLocation.
- Instrumentation (patching code for breaks) happens by applying break
  point info onto code.
- Instrumentation (code patching) is done by the BreakIterator. For
  bytecode, it's BytecodeArrayBreakIterator. For FCG code, it's
  CodeBreakIterator.
- Changes to code instrumentation mostly involves clearing current
  instrumentation and then (re-)applying break points.
- DebugInfo can now reference both bytecode and FCG code.

R=jgruber@chromium.org, mstarzinger@chromium.org
BUG=v8:5265
Committed: https://crrev.com/b8c050424ea928cd75dec3346013a339fdfaccad
Cr-Commit-Position: refs/heads/master@{#38596}

[Yang, 2016-08-11 12:00:59]

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

[commit-bot: I haz the power, 2016-08-11 12:01:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 12:20:56]

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

[commit-bot: I haz the power, 2016-08-11 12:20:57]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12332)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Yang, 2016-08-11 12:55:17]

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

[commit-bot: I haz the power, 2016-08-11 12:55:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 13:44:31]

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

[commit-bot: I haz the power, 2016-08-11 13:44:32]

Dry run: This issue passed the CQ dry run.

[jgruber, 2016-08-11 14:28:14]

LGTM with comments

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

https://codereview.chromium.org/2238893002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:119: } else {
Is abstract_code_->IsByteCodeArray() at this point? What about
debug_info->DebugCode() == abstract_code_ above?

Maybe we could add some DCHECKs in CodeBreakIterator/BytecodeArrayBreakIterator
constructors.

https://codereview.chromium.org/2238893002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1079: } else {
Same here.

https://codereview.chromium.org/2238893002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1117: ApplyBreakPoints(debug_info);
Why are breakpoints reapplied here?

https://codereview.chromium.org/2238893002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2238893002/diff/20001/src/objects.h#newcode10916
src/objects.h:10916: static const int kAbstractCodeIndex =
kSharedFunctionInfoIndex + kPointerSize;
Is this still used anywhere?

[Yang, 2016-08-12 05:33:46]

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

https://codereview.chromium.org/2238893002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:119: } else {
On 2016/08/11 14:28:13, jgruber wrote:
> Is abstract_code_->IsByteCodeArray() at this point? What about
> debug_info->DebugCode() == abstract_code_ above?
> 
> Maybe we could add some DCHECKs in
CodeBreakIterator/BytecodeArrayBreakIterator
> constructors.

Done. Not sure what kind of DCHECKs you had in mind for the BreakIterators
though.

https://codereview.chromium.org/2238893002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1079: } else {
On 2016/08/11 14:28:13, jgruber wrote:
> Same here.

Done.

https://codereview.chromium.org/2238893002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1117: ApplyBreakPoints(debug_info);
On 2016/08/11 14:28:13, jgruber wrote:
> Why are breakpoints reapplied here?

Because having a function flooded with one-shots means that we patch every break
location to break. When we clear these one-shots, we still want to break at
actual break points. So to clear one-shots, we clear all break locations, and
then re-apply break points.

https://codereview.chromium.org/2238893002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2238893002/diff/20001/src/objects.h#newcode10916
src/objects.h:10916: static const int kAbstractCodeIndex =
kSharedFunctionInfoIndex + kPointerSize;
On 2016/08/11 14:28:13, jgruber wrote:
> Is this still used anywhere?

Good catch. Removed.

[Yang, 2016-08-12 05:34:10]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-08-12 05:34:11]

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

[commit-bot: I haz the power, 2016-08-12 05:34:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 05:37:27]

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

[commit-bot: I haz the power, 2016-08-12 05:37:28]

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

[Yang, 2016-08-12 05:43:26]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-08-12 05:43:27]

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

[commit-bot: I haz the power, 2016-08-12 05:43:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 06:06:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-12 06:06:59]

Description was changed from

==========
[debugger] separate break point info from code instrumentation.

Previously, we would both instrument the code, and add/remove
BreakPointInfo objects through BreakLocation. This is bad design and
unsuitable for having two different code kinds.

We would now add/remove BreakPointInfo objects, and use that as source
of truth when instrumenting the code. If we have both bytecode and FCG
code, we would simply apply these break points twice to either.

Notable changes:
- Removed many functionality from BreakLocation.
- Instrumentation (patching code for breaks) happens by applying break
  point info onto code.
- Instrumentation (code patching) is done by the BreakIterator. For
  bytecode, it's BytecodeArrayBreakIterator. For FCG code, it's
  CodeBreakIterator.
- Changes to code instrumentation mostly involves clearing current
  instrumentation and then (re-)applying break points.
- DebugInfo can now reference both bytecode and FCG code.


R=jgruber@chromium.org, mstarzinger@chromium.org
BUG=v8:5265
==========

to

==========
[debugger] separate break point info from code instrumentation.

Previously, we would both instrument the code, and add/remove
BreakPointInfo objects through BreakLocation. This is bad design and
unsuitable for having two different code kinds.

We would now add/remove BreakPointInfo objects, and use that as source
of truth when instrumenting the code. If we have both bytecode and FCG
code, we would simply apply these break points twice to either.

Notable changes:
- Removed many functionality from BreakLocation.
- Instrumentation (patching code for breaks) happens by applying break
  point info onto code.
- Instrumentation (code patching) is done by the BreakIterator. For
  bytecode, it's BytecodeArrayBreakIterator. For FCG code, it's
  CodeBreakIterator.
- Changes to code instrumentation mostly involves clearing current
  instrumentation and then (re-)applying break points.
- DebugInfo can now reference both bytecode and FCG code.

R=jgruber@chromium.org, mstarzinger@chromium.org
BUG=v8:5265

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

[commit-bot: I haz the power, 2016-08-12 06:07:00]

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