Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 11 months ago by Clemens Hammacher Modified:
3 years, 11 months ago Reviewers:
titzer, Yang CC:
v8-reviews_googlegroups.com, Yang Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[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
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix win compile error #Patch Set 4 : Add TODOs #
Total comments: 12
Patch Set 5 : Rebase to Yangs CL and address Ben's comments #
Total comments: 3
Patch Set 6 : Move condition and code into loop #Patch Set 7 : Rearrange code inside the loop #Patch Set 8 : Rebase #
Created: 3 years, 11 months ago
Messages
Total messages: 46 (37 generated)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
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= ==========
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
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.
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
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?
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ;)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
The CQ bit was checked by clemensh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
As discussed offline, I rearranged the code in the loop, it's much shorter now. Landing once bots are green.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by clemensh@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485252669277960,
"parent_rev": "d9253a2f7c6cc043e28fa26decd0d238ebd7ddd8", "commit_rev":
"3dea55b41379a606d7a1f5f33bbdda69e440f2b6"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/v8/v8/+/3dea55b41379a606d7a1f5f33bbdda69e44... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
