[inspector] migrate set/remove BreakPoint to debug-interface.h

[inspector] migrate set/remove BreakPoint to debug-interface.h

- introduced internal BreakPoint object for debug-interface.h.
- inspector uses this object instead of debug.js API via debug-interface.h

BUG=v8:5510

[kozy, 2017-02-13 07:35:55]

kozyatinskiy@chromium.org changed reviewers:
+ yangguo@chromium.org

[kozy, 2017-02-13 07:35:55]

Yang, this CL is PoC about how we can migrate to native API with minimum
changes. I need your opinion on this approach, if it sounds good to you then
I'll clean up code and will add more tests for corner cases.
In general I just introduced new internal struct for BreakPoint and exposed it
for inspector via debug-interface.h and added support of this into
CheckBreakPoint method inside of debug.cc without any usage of mirrors API.

[Yang, 2017-02-15 13:21:10]

awesome stuff. a few comments below.

https://codereview.chromium.org/2685163006/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2685163006/diff/60001/src/api.cc#newcode9094
src/api.cc:9094: i::Handle<i::Object> condition_obj =
Utils::OpenHandle(*condition);
Can we limit the condition to strings only? empty strings don't have to be
checked.

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

https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:637: /* inlined_jsframe_index */ 0, condition, false)
0 is wrong. If you have the situation where f calls g, and g calls h, and
everything is inlined into one frame, f has the index 0, g has the index 1, h
has the index 2.

It's probably easiest to pass in -1 (as a suitably named constant), and have the
FrameInspector the FrameSummary::GetTop() as frame summary for that case.

https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:647: // Ignore check if break point object is not a JSObject.
Can you add a TODO to remove the code below because this is just for the debug
context?

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

https://codereview.chromium.org/2685163006/diff/60001/src/objects-debug.cc#ne...
src/objects-debug.cc:1268: VerifyPointer(condition());
Let's add a check that condition is a string.

[kozy, 2017-02-15 16:14:01]

Description was changed from

==========
[inspector] migrate set/remove BreakPoint to debug-interface.h

It's one of the last three steps, other two:
- move live edit to native,
- move everything related to scopes (huge).

PoC.

BUG=none
==========

to

==========
[inspector] migrate set/remove BreakPoint to debug-interface.h

It's one of the last three steps, other two:
- move live edit to native,
- move everything related to scopes (huge).

BUG=none
==========

[kozy, 2017-02-15 16:14:12]

kozyatinskiy@chromium.org changed reviewers:
+ dgozman@chromium.org

[kozy, 2017-02-15 16:18:35]

Description was changed from

==========
[inspector] migrate set/remove BreakPoint to debug-interface.h

It's one of the last three steps, other two:
- move live edit to native,
- move everything related to scopes (huge).

BUG=none
==========

to

==========
[inspector] migrate set/remove BreakPoint to debug-interface.h

- introduced internal BreakPoint object for debug-interface.h.
- inspector uses this object instead of debug.js API via debug-interface.h

BUG=v8:5510
==========

[kozy, 2017-02-15 16:21:33]

All done.

Dmitry, please take a look on inspector side.

https://codereview.chromium.org/2685163006/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2685163006/diff/60001/src/api.cc#newcode9094
src/api.cc:9094: i::Handle<i::Object> condition_obj =
Utils::OpenHandle(*condition);
On 2017/02/15 13:21:10, Yang wrote:
> Can we limit the condition to strings only? empty strings don't have to be
> checked.

Done.

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

https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:637: /* inlined_jsframe_index */ 0, condition, false)
On 2017/02/15 13:21:10, Yang wrote:
> 0 is wrong. If you have the situation where f calls g, and g calls h, and
> everything is inlined into one frame, f has the index 0, g has the index 1, h
> has the index 2.
> 
> It's probably easiest to pass in -1 (as a suitably named constant), and have
the
> FrameInspector the FrameSummary::GetTop() as frame summary for that case.


I thought that when we call CheckBreakpoint on top of stack trace we have
deoptimized frame and all inlined frames are stored separately in this case.
Done.

https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:647: // Ignore check if break point object is not a JSObject.
On 2017/02/15 13:21:10, Yang wrote:
> Can you add a TODO to remove the code below because this is just for the debug
> context?

Done.

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

https://codereview.chromium.org/2685163006/diff/60001/src/objects-debug.cc#ne...
src/objects-debug.cc:1268: VerifyPointer(condition());
On 2017/02/15 13:21:10, Yang wrote:
> Let's add a check that condition is a string.

Done.

[Yang, 2017-02-15 16:32:23]

lgtm

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

https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:637: /* inlined_jsframe_index */ 0, condition, false)
On 2017/02/15 16:21:33, kozy wrote:
> On 2017/02/15 13:21:10, Yang wrote:
> > 0 is wrong. If you have the situation where f calls g, and g calls h, and
> > everything is inlined into one frame, f has the index 0, g has the index 1,
h
> > has the index 2.
> > 
> > It's probably easiest to pass in -1 (as a suitably named constant), and have
> the
> > FrameInspector the FrameSummary::GetTop() as frame summary for that case.
> 
> 
> I thought that when we call CheckBreakpoint on top of stack trace we have
> deoptimized frame and all inlined frames are stored separately in this case.
> Done.

Ah yes you are right! Can you add a comment about this?

[kozy, 2017-02-15 16:42:34]

thanks, added a comment.

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

https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:637: /* inlined_jsframe_index */ 0, condition, false)
On 2017/02/15 16:32:23, Yang wrote:
> On 2017/02/15 16:21:33, kozy wrote:
> > On 2017/02/15 13:21:10, Yang wrote:
> > > 0 is wrong. If you have the situation where f calls g, and g calls h, and
> > > everything is inlined into one frame, f has the index 0, g has the index
1,
> h
> > > has the index 2.
> > > 
> > > It's probably easiest to pass in -1 (as a suitably named constant), and
have
> > the
> > > FrameInspector the FrameSummary::GetTop() as frame summary for that case.
> > 
> > 
> > I thought that when we call CheckBreakpoint on top of stack trace we have
> > deoptimized frame and all inlined frames are stored separately in this case.
> > Done.
> 
> Ah yes you are right! Can you add a comment about this?

Added.

[kozy, 2017-02-15 16:42:39]

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

[commit-bot: I haz the power, 2017-02-15 16:42:44]

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, 2017-02-15 16:49:14]

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

[commit-bot: I haz the power, 2017-02-15 16:49:15]

Dry run: 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...)

[Yang, 2017-03-09 06:21:44]

On 2017/02/15 16:42:34, kozy wrote:
> thanks, added a comment.
> 
> https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2685163006/diff/60001/src/debug/debug.cc#newc...
> src/debug/debug.cc:637: /* inlined_jsframe_index */ 0, condition, false)
> On 2017/02/15 16:32:23, Yang wrote:
> > On 2017/02/15 16:21:33, kozy wrote:
> > > On 2017/02/15 13:21:10, Yang wrote:
> > > > 0 is wrong. If you have the situation where f calls g, and g calls h,
and
> > > > everything is inlined into one frame, f has the index 0, g has the index
> 1,
> > h
> > > > has the index 2.
> > > > 
> > > > It's probably easiest to pass in -1 (as a suitably named constant), and
> have
> > > the
> > > > FrameInspector the FrameSummary::GetTop() as frame summary for that
case.
> > > 
> > > 
> > > I thought that when we call CheckBreakpoint on top of stack trace we have
> > > deoptimized frame and all inlined frames are stored separately in this
case.
> > > Done.
> > 
> > Ah yes you are right! Can you add a comment about this?
> 
> Added.

why has this not landed yet? :)

[kozy, 2017-03-24 00:48:15]

we'd like to have BreakLocation as interface between inspector and debugger.