[wasm] Set and store breakpoints in wasm

[wasm] Set and store breakpoints in wasm

Store breakpoint positions in the WasmSharedModuleData in order to set
them on new instantiations. Also redirect them to all live instances at
the time the breakpoint is set.

Inside the WasmDebugInfo, we store the BreakPointInfo objects to find
hit breakpoints.

R=titzer@chromium.org, yangguo@chromium.org
BUG=v8:5822
Review-Url: https://codereview.chromium.org/2626253002
Cr-Commit-Position: refs/heads/master@{#42443}
Committed: https://chromium.googlesource.com/v8/v8/+/2f3de27e1142aa0d5d1b5fe82c5c632786390c8a

[Clemens Hammacher, 2017-01-12 15:10:46]

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

[commit-bot: I haz the power, 2017-01-12 15:10:51]

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

[Clemens Hammacher, 2017-01-12 15:22:12]

Description was changed from

==========
[wasm] Set and store breakpoints in wasm

Store breakpoint positions in the WasmSharedModuleData in order to set
them on new instantiations. Also redirect them to all live instances at
the time the breakpoint is set.

Inside the WasmDebugInfo, we store the BreakPointInfo objects to find
hit breakpoints.

Rtitzer@chromium.org, yangguo@chromium.org
BUG=v8:5822
==========

to

==========
[wasm] Set and store breakpoints in wasm

Store breakpoint positions in the WasmSharedModuleData in order to set
them on new instantiations. Also redirect them to all live instances at
the time the breakpoint is set.

Inside the WasmDebugInfo, we store the BreakPointInfo objects to find
hit breakpoints.

R=titzer@chromium.org, yangguo@chromium.org
BUG=v8:5822
==========

[Clemens Hammacher, 2017-01-12 15:22:13]

clemensh@chromium.org changed reviewers:
+ titzer@chromium.org, yangguo@chromium.org

[commit-bot: I haz the power, 2017-01-12 15:47:25]

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

[commit-bot: I haz the power, 2017-01-12 15:47:26]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-12 15:51:20]

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

https://codereview.chromium.org/2626253002/diff/1/src/debug/debug.cc#newcode540
src/debug/debug.cc:540: break_points_hit =
compiled_module->GetHitBreakpoints(location);
Do you want to name this method CheckBreakPoints for symmetry with the other
method below (or rename the JS one)?

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:1069: {
So as I see it, this will find the first bytecode position greater than or equal
to the given offset_in_func. Is that what we want? IIRC we have a bitmap
somewhere which tells us valid bytecode offsets; why not simply start at the
given offset and scan the bitmap forward to this position?

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-opcodes.h#new...
src/wasm/wasm-opcodes.h:682: static bool IsReturn(const byte opcode) { return
opcode == kExprReturn; }
I think it's best not to add too many utility functions here, otherwise this
class (and header file) will just fill up with lots of them.

[Clemens Hammacher, 2017-01-12 19:55:09]

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

[commit-bot: I haz the power, 2017-01-12 19:55:15]

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

[Clemens Hammacher, 2017-01-12 19:55:51]

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

https://codereview.chromium.org/2626253002/diff/1/src/debug/debug.cc#newcode540
src/debug/debug.cc:540: break_points_hit =
compiled_module->GetHitBreakpoints(location);
On 2017/01/12 at 15:51:20, titzer wrote:
> Do you want to name this method CheckBreakPoints for symmetry with the other
method below (or rename the JS one)?

Done.

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:1069: {
On 2017/01/12 at 15:51:20, titzer wrote:
> So as I see it, this will find the first bytecode position greater than or
equal to the given offset_in_func. Is that what we want? IIRC we have a bitmap
somewhere which tells us valid bytecode offsets; why not simply start at the
given offset and scan the bitmap forward to this position?

As discussed offline: Made is a DCHECK.

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-opcodes.h#new...
src/wasm/wasm-opcodes.h:682: static bool IsReturn(const byte opcode) { return
opcode == kExprReturn; }
On 2017/01/12 at 15:51:20, titzer wrote:
> I think it's best not to add too many utility functions here, otherwise this
class (and header file) will just fill up with lots of them.

Removed.

[commit-bot: I haz the power, 2017-01-12 20:00:04]

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

[commit-bot: I haz the power, 2017-01-12 20:00:04]

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

[Clemens Hammacher, 2017-01-12 20:03:01]

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

[commit-bot: I haz the power, 2017-01-12 20:03:05]

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-01-12 20:30:36]

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

[commit-bot: I haz the power, 2017-01-12 20:30:37]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-13 16:55:55]

On 2017/01/12 19:55:51, Clemens Hammacher wrote:
> https://codereview.chromium.org/2626253002/diff/1/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2626253002/diff/1/src/debug/debug.cc#newcode540
> src/debug/debug.cc:540: break_points_hit =
> compiled_module->GetHitBreakpoints(location);
> On 2017/01/12 at 15:51:20, titzer wrote:
> > Do you want to name this method CheckBreakPoints for symmetry with the other
> method below (or rename the JS one)?
> 
> Done.
> 
> https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-objects.cc
> File src/wasm/wasm-objects.cc (right):
> 
>
https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:1069: {
> On 2017/01/12 at 15:51:20, titzer wrote:
> > So as I see it, this will find the first bytecode position greater than or
> equal to the given offset_in_func. Is that what we want? IIRC we have a bitmap
> somewhere which tells us valid bytecode offsets; why not simply start at the
> given offset and scan the bitmap forward to this position?
> 
> As discussed offline: Made is a DCHECK.
> 
> https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-opcodes.h
> File src/wasm/wasm-opcodes.h (right):
> 
>
https://codereview.chromium.org/2626253002/diff/1/src/wasm/wasm-opcodes.h#new...
> src/wasm/wasm-opcodes.h:682: static bool IsReturn(const byte opcode) { return
> opcode == kExprReturn; }
> On 2017/01/12 at 15:51:20, titzer wrote:
> > I think it's best not to add too many utility functions here, otherwise this
> class (and header file) will just fill up with lots of them.
> 
> Removed.

lgtm

[titzer, 2017-01-13 16:56:01]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2017-01-13 16:56:06]

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-01-13 16:59:55]

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

[commit-bot: I haz the power, 2017-01-13 16:59:56]

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

[Clemens Hammacher, 2017-01-16 09:13:00]

Yang, PTAL on src/debug stuff, maybe also src/.

[Yang, 2017-01-16 15:34:18]

https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc#newc...
src/debug/debug.cc:539: wasm_frame->wasm_instance()->compiled_module(),
isolate_);
Does any of the logic in Debug::Break apply to wasm? If there is enough
difference, maybe the wasm interpreter should not call into Debug::Break at all,
instead of branching on wasm all the time.

[Clemens Hammacher, 2017-01-16 16:03:28]

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

[commit-bot: I haz the power, 2017-01-16 16:03:31]

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

[Clemens Hammacher, 2017-01-16 16:12:15]

The CQ bit was unchecked by clemensh@chromium.org

[Clemens Hammacher, 2017-01-16 16:12:25]

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

[commit-bot: I haz the power, 2017-01-16 16:12:39]

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

[Clemens Hammacher, 2017-01-16 16:17:27]

https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc#newc...
src/debug/debug.cc:539: wasm_frame->wasm_instance()->compiled_module(),
isolate_);
On 2017/01/16 at 15:34:17, Yang wrote:
> Does any of the logic in Debug::Break apply to wasm? If there is enough
difference, maybe the wasm interpreter should not call into Debug::Break at all,
instead of branching on wasm all the time.

Right, probably not. All the stepping related logic is quite different for wasm.
I changed it back to take a JavaScriptFrame*, and will implement another
Debug::BreakInWasm method in a follow-up CL.
Now we could probably also change back BreakLocation::FromFrame to take a
DebugInfo and a JavaScriptFrame, and have another BreakLocation::FromWasmFrame
method. Would you prefer that? I would add it to the follow-up CL then, to get
this one out.

[Yang, 2017-01-16 16:21:25]

On 2017/01/16 16:17:27, Clemens Hammacher wrote:
> https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc#newc...
> src/debug/debug.cc:539: wasm_frame->wasm_instance()->compiled_module(),
> isolate_);
> On 2017/01/16 at 15:34:17, Yang wrote:
> > Does any of the logic in Debug::Break apply to wasm? If there is enough
> difference, maybe the wasm interpreter should not call into Debug::Break at
all,
> instead of branching on wasm all the time.
> 
> Right, probably not. All the stepping related logic is quite different for
wasm.
> I changed it back to take a JavaScriptFrame*, and will implement another
> Debug::BreakInWasm method in a follow-up CL.
> Now we could probably also change back BreakLocation::FromFrame to take a
> DebugInfo and a JavaScriptFrame, and have another BreakLocation::FromWasmFrame
> method. Would you prefer that? I would add it to the follow-up CL then, to get
> this one out.

I dont have enough overview over what you have planned for stepping etc. E.g.
how handing over between stepping in JS and stepping in wasm works (step in,
step out). If wasm is mostly self contained, it probably makes sense to separate
it.

[commit-bot: I haz the power, 2017-01-16 16:46:23]

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

[commit-bot: I haz the power, 2017-01-16 16:46:25]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-16 18:53:56]

On 2017/01/16 at 16:21:25, yangguo wrote:
> On 2017/01/16 16:17:27, Clemens Hammacher wrote:
> > https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc
> > File src/debug/debug.cc (right):
> > 
> >
https://codereview.chromium.org/2626253002/diff/40001/src/debug/debug.cc#newc...
> > src/debug/debug.cc:539: wasm_frame->wasm_instance()->compiled_module(),
> > isolate_);
> > On 2017/01/16 at 15:34:17, Yang wrote:
> > > Does any of the logic in Debug::Break apply to wasm? If there is enough
> > difference, maybe the wasm interpreter should not call into Debug::Break at
all,
> > instead of branching on wasm all the time.
> > 
> > Right, probably not. All the stepping related logic is quite different for
wasm.
> > I changed it back to take a JavaScriptFrame*, and will implement another
> > Debug::BreakInWasm method in a follow-up CL.
> > Now we could probably also change back BreakLocation::FromFrame to take a
> > DebugInfo and a JavaScriptFrame, and have another
BreakLocation::FromWasmFrame
> > method. Would you prefer that? I would add it to the follow-up CL then, to
get
> > this one out.
> 
> I dont have enough overview over what you have planned for stepping etc. E.g.
how handing over between stepping in JS and stepping in wasm works (step in,
step out). If wasm is mostly self contained, it probably makes sense to separate
it.

I am currently looking into stepping. I haven't figured out all the details yet.
Do you think that blocks this CL? Otherwise I would propose to land this now and
figure out BreakLocation::FromFrame and other details later.

[Yang, 2017-01-17 13:46:41]

https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h
File src/wasm/wasm-objects.h (right):

https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h...
src/wasm/wasm-objects.h:398: MaybeHandle<FixedArray> CheckBreakPoints(const
BreakLocation& location);
Is this called from anywhere?

[Clemens Hammacher, 2017-01-17 14:25:11]

https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h
File src/wasm/wasm-objects.h (right):

https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h...
src/wasm/wasm-objects.h:398: MaybeHandle<FixedArray> CheckBreakPoints(const
BreakLocation& location);
On 2017/01/17 at 13:46:41, Yang wrote:
> Is this called from anywhere?

Not any more, it was called from Debug::Break and will be called from
Debug::BreakInWasm or any related method in the future.

[Yang, 2017-01-18 09:51:24]

On 2017/01/17 14:25:11, Clemens Hammacher wrote:
> https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h
> File src/wasm/wasm-objects.h (right):
> 
>
https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h...
> src/wasm/wasm-objects.h:398: MaybeHandle<FixedArray> CheckBreakPoints(const
> BreakLocation& location);
> On 2017/01/17 at 13:46:41, Yang wrote:
> > Is this called from anywhere?
> 
> Not any more, it was called from Debug::Break and will be called from
> Debug::BreakInWasm or any related method in the future.

src/debug lgtm

[Clemens Hammacher, 2017-01-18 09:55:12]

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

[commit-bot: I haz the power, 2017-01-18 09:55:17]

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

[Clemens Hammacher, 2017-01-18 10:00:41]

On 2017/01/18 at 09:51:24, yangguo wrote:
> On 2017/01/17 14:25:11, Clemens Hammacher wrote:
> >
https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h
> > File src/wasm/wasm-objects.h (right):
> > 
> >
https://codereview.chromium.org/2626253002/diff/80001/src/wasm/wasm-objects.h...
> > src/wasm/wasm-objects.h:398: MaybeHandle<FixedArray> CheckBreakPoints(const
> > BreakLocation& location);
> > On 2017/01/17 at 13:46:41, Yang wrote:
> > > Is this called from anywhere?
> > 
> > Not any more, it was called from Debug::Break and will be called from
> > Debug::BreakInWasm or any related method in the future.
> 
> src/debug lgtm

As discussed: Removed the CheckBreakPoints method for now, will re-introduce
once it's actually used.
Landing as soon as the bots are green.

[commit-bot: I haz the power, 2017-01-18 10:23:45]

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

[commit-bot: I haz the power, 2017-01-18 10:23:46]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-18 10:26:33]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-01-18 10:26:33]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2626253002/#ps100001
(title: "Remove CheckBreakPoints methods")

[commit-bot: I haz the power, 2017-01-18 10:26:45]

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-01-18 10:28:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484735193068840,
"parent_rev": "7634b0eb13e945191c04e63af86bf9fdc1f6aa4c", "commit_rev":
"2f3de27e1142aa0d5d1b5fe82c5c632786390c8a"}

[commit-bot: I haz the power, 2017-01-18 10:28:45]

Description was changed from

==========
[wasm] Set and store breakpoints in wasm

Store breakpoint positions in the WasmSharedModuleData in order to set
them on new instantiations. Also redirect them to all live instances at
the time the breakpoint is set.

Inside the WasmDebugInfo, we store the BreakPointInfo objects to find
hit breakpoints.

R=titzer@chromium.org, yangguo@chromium.org
BUG=v8:5822
==========

to

==========
[wasm] Set and store breakpoints in wasm

Store breakpoint positions in the WasmSharedModuleData in order to set
them on new instantiations. Also redirect them to all live instances at
the time the breakpoint is set.

Inside the WasmDebugInfo, we store the BreakPointInfo objects to find
hit breakpoints.

R=titzer@chromium.org, yangguo@chromium.org
BUG=v8:5822

Review-Url: https://codereview.chromium.org/2626253002
Cr-Commit-Position: refs/heads/master@{#42443}
Committed:
https://chromium.googlesource.com/v8/v8/+/2f3de27e1142aa0d5d1b5fe82c5c6327863...
==========

[commit-bot: I haz the power, 2017-01-18 10:28:47]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/2f3de27e1142aa0d5d1b5fe82c5c6327863...