Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(354)

Issue 2651043004: [wasm] Add inspector test for stepping (Closed)

Created:
3 years, 11 months ago by Clemens Hammacher
Modified:
3 years, 10 months ago
Reviewers:
kozy, titzer, Yang
CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[wasm] Add inspector test for stepping This also fixes bugs found by the new test. It only tests stepping inside of wasm code. Wasm to JS and vice versa will follow in another CL. R=yangguo@chromium.org, titzer@chromium.org, kozyatinskiy@chromium.org BUG=v8:5822 Review-Url: https://codereview.chromium.org/2651043004 Cr-Commit-Position: refs/heads/master@{#42729} Committed: https://chromium.googlesource.com/v8/v8/+/b7947f8cd73ca254ea086c441dd75dd239a19b58

Patch Set 1 #

Patch Set 2 : Add expected output #

Total comments: 3

Patch Set 3 : Add SaveContext scope #

Total comments: 10

Patch Set 4 : Beautification #

Unified diffs Side-by-side diffs Delta from patch set Stats (+250 lines, -5 lines) Patch
M src/wasm/wasm-debug.cc View 1 chunk +1 line, -1 line 0 comments Download
M src/wasm/wasm-interpreter.cc View 2 chunks +4 lines, -3 lines 0 comments Download
A test/inspector/debugger/wasm-stepping.js View 1 2 3 1 chunk +160 lines, -0 lines 0 comments Download
A test/inspector/debugger/wasm-stepping-expected.txt View 1 2 3 1 chunk +83 lines, -0 lines 0 comments Download
M test/inspector/protocol-test.js View 1 chunk +2 lines, -1 line 0 comments Download

Depends on Patchset:

Messages

Total messages: 27 (18 generated)
Clemens Hammacher
3 years, 11 months ago (2017-01-24 21:20:32 UTC) #7
titzer
lgtm https://codereview.chromium.org/2651043004/diff/20001/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2651043004/diff/20001/src/runtime/runtime-wasm.cc#newcode173 src/runtime/runtime-wasm.cc:173: isolate->set_context(*instance->compiled_module()->native_context()); Do we need to set it back ...
3 years, 11 months ago (2017-01-25 10:04:09 UTC) #8
Yang
lgtm https://codereview.chromium.org/2651043004/diff/20001/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2651043004/diff/20001/src/runtime/runtime-wasm.cc#newcode173 src/runtime/runtime-wasm.cc:173: isolate->set_context(*instance->compiled_module()->native_context()); We have a SaveContext scope. Is that ...
3 years, 11 months ago (2017-01-25 10:12:23 UTC) #9
Clemens Hammacher
https://codereview.chromium.org/2651043004/diff/20001/src/runtime/runtime-wasm.cc File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2651043004/diff/20001/src/runtime/runtime-wasm.cc#newcode173 src/runtime/runtime-wasm.cc:173: isolate->set_context(*instance->compiled_module()->native_context()); On 2017/01/25 at 10:12:22, Yang wrote: > We ...
3 years, 11 months ago (2017-01-25 11:41:57 UTC) #12
kozy
lgtm % test comments https://codereview.chromium.org/2651043004/diff/40001/test/inspector/debugger/wasm-stepping.js File test/inspector/debugger/wasm-stepping.js (right): https://codereview.chromium.org/2651043004/diff/40001/test/inspector/debugger/wasm-stepping.js#newcode46 test/inspector/debugger/wasm-stepping.js:46: var addSourceUrl = (code, url) ...
3 years, 11 months ago (2017-01-25 18:04:59 UTC) #15
Clemens Hammacher
Thanks for the comments, Alexey. It improved the test case a lot. It's much more ...
3 years, 11 months ago (2017-01-25 21:06:11 UTC) #17
kozy
thanks! lgtm
3 years, 11 months ago (2017-01-25 21:41:10 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2651043004/60001
3 years, 10 months ago (2017-01-27 08:14:49 UTC) #24
commit-bot: I haz the power
3 years, 10 months ago (2017-01-27 08:50:56 UTC) #27
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/b7947f8cd73ca254ea086c441dd75dd239a...

Powered by Google App Engine
This is Rietveld 408576698