[wasm] Implement stepping in wasm code

[wasm] Implement stepping in wasm code

Implement stepping by remembering the current step action in the wasm
interpreter handle in WasmDebugInfo, and using it when continuing
execution in the interpreter.
The control flow is as follows: After module compilation, the user sets
a breakpoint in wasm. The respective function is redirected to the
interpreter and the breakpoint is set on the interpreter. When it is
hit, we notify all debug event listeners, which might prepare stepping.
When returning from these listeners, before continuing execution, we
check whether stepping was requested and continue execution in the
interpreter accordingly.

Stepping from Wasm to JS and vice versa will be implemented and tested
in a follow-up CL. Testing this requires breakpoints and stepping in
Wasm to be exposed via the inspector interface, such that we can write
an inspector test. This mixed JS-Wasm-execution is hard to set up in a
cctest.

R=titzer@chromium.org, yangguo@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2649533002
Cr-Commit-Position: refs/heads/master@{#42624}
Committed: https://chromium.googlesource.com/v8/v8/+/3dea55b41379a606d7a1f5f33bbdda69e440f2b6

[Clemens Hammacher, 2017-01-20 10:27:27]

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

[commit-bot: I haz the power, 2017-01-20 10:27:36]

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-20 10:29:06]

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

[commit-bot: I haz the power, 2017-01-20 10:29:07]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19590)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/16048)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/32936)

[Clemens Hammacher, 2017-01-20 14:01:22]

Description was changed from

==========
[wasm] Implement stepping in wasm code

Implement stepping by remembering the current step action in the wasm
interpreter handle in WasmDebugInfo, and using it when continuing
execution in the interpreter.
The control flow is as follows: After module compilation, the user sets
a breakpoint in wasm. The respective function is redirected to the
interpreter and the breakpoint is set on the interpreter. When it is
hit, we notify all debug event listeners, which might prepare stepping.
When returning from these listeners, before continuing execution, we
check whether stepping was requested and continue execution in the
interpreter accordingly.

R=titzer@chromium.org, yangguo@chromium.org
BUG=
==========

to

==========
[wasm] Implement stepping in wasm code

Implement stepping by remembering the current step action in the wasm
interpreter handle in WasmDebugInfo, and using it when continuing
execution in the interpreter.
The control flow is as follows: After module compilation, the user sets
a breakpoint in wasm. The respective function is redirected to the
interpreter and the breakpoint is set on the interpreter. When it is
hit, we notify all debug event listeners, which might prepare stepping.
When returning from these listeners, before continuing execution, we
check whether stepping was requested and continue execution in the
interpreter accordingly.

Stepping from Wasm to JS and vice versa will be implemented and tested
in a follow-up CL. Testing this requires breakpoints and stepping in
Wasm to be exposed via the inspector interface, such that we can write
an inspector test. This mixed JS-Wasm-execution is hard to set up in a
cctest.

R=titzer@chromium.org, yangguo@chromium.org
BUG=
==========

[Clemens Hammacher, 2017-01-20 14:48:37]

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

[commit-bot: I haz the power, 2017-01-20 14:48:42]

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-20 14:54:12]

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

[commit-bot: I haz the power, 2017-01-20 14:54:13]

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

[Clemens Hammacher, 2017-01-20 14:57:56]

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

[commit-bot: I haz the power, 2017-01-20 14:58:02]

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-20 15:34:14]

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

[commit-bot: I haz the power, 2017-01-20 15:34:15]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-20 16:37:10]

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

[commit-bot: I haz the power, 2017-01-20 16:37:17]

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-20 17:12:58]

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

[commit-bot: I haz the power, 2017-01-20 17:12:59]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-23 10:10:10]

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

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:997: if (frame->is_wasm()) {
How do we know that this is a WasmInterpreterEntryFrame? Could it also be a
WasmCompiledFrame?

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:1059: if (!frames_it.done()) {
Does it make sense to move this condition into the loop?

AFAICT the only two ways to exit the loop are either to finish the frame
iterator, if the frame is not JS, or if the frame is not subject to debugging,
but it is hard to tell if I am missing something because of the structure.

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.js
File src/debug/debug.js (right):

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.js#newc...
src/debug/debug.js:1300: if (stepaction) {
This can probably be further simplified to just be a switch with a default case,
and no if.

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

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:113: bool finished = false;
Can you drop in a TODO here that the interpreter returns after so many
instructions are executed, and that this would probably be the place to
implement stack checks (i.e. checks for interruption)?

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:205: Handle<WasmCompiledModule> compiled_module(
Should probably discuss more about how this works, because I don't fully
understand how breakpoints are stored in general. E.g. to what extend breakpoint
state might better live in the WasmInterpreter.

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-interpret...
File src/wasm/wasm-interpreter.h (right):

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-interpret...
src/wasm/wasm-interpreter.h:123: // Tells a thread to pause after certain
instructions.
It should also be possible to implement this outside the interpreter, simply by
iterating the code and installing breakpoints at returns and calls. I'm not sure
if it's simpler until we try, though.

[Clemens Hammacher, 2017-01-23 12:14:22]

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

[commit-bot: I haz the power, 2017-01-23 12:14:27]

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-23 12:14:55]

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

[commit-bot: I haz the power, 2017-01-23 12:15:02]

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)

[Clemens Hammacher, 2017-01-23 12:25:34]

Rebased on Yang's latest CL.
PTAL

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

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:997: if (frame->is_wasm()) {
On 2017/01/23 at 10:10:09, titzer wrote:
> How do we know that this is a WasmInterpreterEntryFrame? Could it also be a
WasmCompiledFrame?

Not in stepping or when paused at a breakpoint, but I think it could actually
happen if we pause on a thrown exception.
I added an if there, and we just do nothing if it's compiled.

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.cc#newc...
src/debug/debug.cc:1059: if (!frames_it.done()) {
On 2017/01/23 at 10:10:10, titzer wrote:
> Does it make sense to move this condition into the loop?
> 
> AFAICT the only two ways to exit the loop are either to finish the frame
iterator, if the frame is not JS, or if the frame is not subject to debugging,
but it is hard to tell if I am missing something because of the structure.

I agree that the loop is hard to read. It basically just deoptimizes everything
on the top which is not subject to debugging. It stops at the first frame this
is subject to debugging, i.e. unoptimized JS or Wasm.

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.js
File src/debug/debug.js (right):

https://codereview.chromium.org/2649533002/diff/60001/src/debug/debug.js#newc...
src/debug/debug.js:1300: if (stepaction) {
On 2017/01/23 at 10:10:10, titzer wrote:
> This can probably be further simplified to just be a switch with a default
case, and no if.

Yang just removed this whole code anyway :)

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

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:113: bool finished = false;
On 2017/01/23 at 10:10:10, titzer wrote:
> Can you drop in a TODO here that the interpreter returns after so many
instructions are executed, and that this would probably be the place to
implement stack checks (i.e. checks for interruption)?

Done.

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-debug.cc#...
src/wasm/wasm-debug.cc:205: Handle<WasmCompiledModule> compiled_module(
On 2017/01/23 at 10:10:10, titzer wrote:
> Should probably discuss more about how this works, because I don't fully
understand how breakpoints are stored in general. E.g. to what extend breakpoint
state might better live in the WasmInterpreter.

Breakpoints are currently still created from JS, and contain information like an
"active" flag, and a "condition" which we might also want to evaluate for wasm
at some point (e.g. by creating an artificial context which binds local
variables).
I think it currently makes sense to reuse that part of the breakpoint logic.

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-interpret...
File src/wasm/wasm-interpreter.h (right):

https://codereview.chromium.org/2649533002/diff/60001/src/wasm/wasm-interpret...
src/wasm/wasm-interpreter.h:123: // Tells a thread to pause after certain
instructions.
On 2017/01/23 at 10:10:10, titzer wrote:
> It should also be possible to implement this outside the interpreter, simply
by iterating the code and installing breakpoints at returns and calls. I'm not
sure if it's simpler until we try, though.

This is basically what JS does: Flood the function(s) with breakpoints, and
later clear them again. We would have to be careful tough to only clear those
breakpoints related to stepping.
This flag seemed simpler to me, as it allows to just set the flag and continue
execution. JS has a similar thing (hook_on_function_call_) to efficiently break
on each function call.

[Yang, 2017-01-23 14:26:20]

src/debug lgtm.

https://codereview.chromium.org/2649533002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2649533002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1084: // TODO(clemensh): Implement stepping from JS into WASM
or vice versa.
StepFrame will be removed soonish. Once Framework blackboxing has been
reimplemented here: https://codereview.chromium.org/2633803002

[titzer, 2017-01-23 18:30:46]

lgtm

https://codereview.chromium.org/2649533002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2649533002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1049: while (!frames_it.done()) {
As per previous comment, mind cleaning up this loop while you're here?

[Clemens Hammacher, 2017-01-23 18:43:42]

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

[commit-bot: I haz the power, 2017-01-23 18:43:53]

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-23 18:47:27]

https://codereview.chromium.org/2649533002/diff/80001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2649533002/diff/80001/src/debug/debug.cc#newc...
src/debug/debug.cc:1049: while (!frames_it.done()) {
On 2017/01/23 at 18:30:45, titzer wrote:
> As per previous comment, mind cleaning up this loop while you're here?

Done, but I'm not sure if it improves readability.
To me, the code before was clearer: Skip over non-debuggable functions (while
loop), then prepare stepping out to the function where we landed.
Now the code to prepare stepping is inside the loop, but has a break right after
it.
Waiting for another comment before landing ;)

[commit-bot: I haz the power, 2017-01-23 19:19:32]

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

[commit-bot: I haz the power, 2017-01-23 19:19:33]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-24 09:26:10]

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

[commit-bot: I haz the power, 2017-01-24 09:26:13]

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-24 09:27:44]

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

[commit-bot: I haz the power, 2017-01-24 09:27:45]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19780)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19851)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  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...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19749)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/33181)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21407)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21493)

[Clemens Hammacher, 2017-01-24 09:35:51]

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

[commit-bot: I haz the power, 2017-01-24 09:36:01]

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-24 09:45:20]

As discussed offline, I rearranged the code in the loop, it's much shorter now.
Landing once bots are green.

[commit-bot: I haz the power, 2017-01-24 10:10:57]

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

[commit-bot: I haz the power, 2017-01-24 10:10:58]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-24 10:11:09]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-01-24 10:11:09]

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/2649533002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-24 10:11: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, 2017-01-24 10:13:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485252669277960,
"parent_rev": "d9253a2f7c6cc043e28fa26decd0d238ebd7ddd8", "commit_rev":
"3dea55b41379a606d7a1f5f33bbdda69e440f2b6"}

[commit-bot: I haz the power, 2017-01-24 10:13:41]

Description was changed from

==========
[wasm] Implement stepping in wasm code

Implement stepping by remembering the current step action in the wasm
interpreter handle in WasmDebugInfo, and using it when continuing
execution in the interpreter.
The control flow is as follows: After module compilation, the user sets
a breakpoint in wasm. The respective function is redirected to the
interpreter and the breakpoint is set on the interpreter. When it is
hit, we notify all debug event listeners, which might prepare stepping.
When returning from these listeners, before continuing execution, we
check whether stepping was requested and continue execution in the
interpreter accordingly.

Stepping from Wasm to JS and vice versa will be implemented and tested
in a follow-up CL. Testing this requires breakpoints and stepping in
Wasm to be exposed via the inspector interface, such that we can write
an inspector test. This mixed JS-Wasm-execution is hard to set up in a
cctest.

R=titzer@chromium.org, yangguo@chromium.org
BUG=
==========

to

==========
[wasm] Implement stepping in wasm code

Implement stepping by remembering the current step action in the wasm
interpreter handle in WasmDebugInfo, and using it when continuing
execution in the interpreter.
The control flow is as follows: After module compilation, the user sets
a breakpoint in wasm. The respective function is redirected to the
interpreter and the breakpoint is set on the interpreter. When it is
hit, we notify all debug event listeners, which might prepare stepping.
When returning from these listeners, before continuing execution, we
check whether stepping was requested and continue execution in the
interpreter accordingly.

Stepping from Wasm to JS and vice versa will be implemented and tested
in a follow-up CL. Testing this requires breakpoints and stepping in
Wasm to be exposed via the inspector interface, such that we can write
an inspector test. This mixed JS-Wasm-execution is hard to set up in a
cctest.

R=titzer@chromium.org, yangguo@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2649533002
Cr-Commit-Position: refs/heads/master@{#42624}
Committed:
https://chromium.googlesource.com/v8/v8/+/3dea55b41379a606d7a1f5f33bbdda69e44...
==========

[commit-bot: I haz the power, 2017-01-24 10:13:42]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/3dea55b41379a606d7a1f5f33bbdda69e44...