Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629883002:
[wasm] Add tests for breakpoints (Closed)
|
Created:
3 years, 11 months ago by Clemens Hammacher Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Add tests for breakpoints
Test that setting breakpoints works for wasm, and that they are hit
correctly.
This basically tests all the layers involved: Compiling and running
wasm interpreter entries, passing arguments to the interpreter, storing
break point infos in wasm objects, getting the right BreakLocation from
wasm frames, and getting stack information from interpreted frames.
BUG=v8:5822
R=titzer@chromium.org, yangguo@chromium.org
Review-Url: https://codereview.chromium.org/2629883002
Cr-Commit-Position: refs/heads/master@{#42560}
Committed: https://chromium.googlesource.com/v8/v8/+/a1e04ef524e9705da304acd0023e28602d0af978
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix bug in wasm-debug.cc #
Total comments: 8
Patch Set 4 : Address comments #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Total comments: 2
Patch Set 8 : Remove test for argument passing #
Depends on Patchset: Messages
Total messages: 45 (34 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...
Description was changed from ========== [wasm] Add tests for breakpoints Test that setting breakpoints works for wasm, and that they are hit correctly. This basically tests all the layers involved: Compiling and running wasm interpreter entries, passing arguments to the interpreter, storing break point infos in wasm objects, getting the right BreakLocation from wasm frames, and getting stack information from interpreted frames. BUG=v8:5822 R=titzer@chromium.org ========== to ========== [wasm] Add tests for breakpoints Test that setting breakpoints works for wasm, and that they are hit correctly. This basically tests all the layers involved: Compiling and running wasm interpreter entries, passing arguments to the interpreter, storing break point infos in wasm objects, getting the right BreakLocation from wasm frames, and getting stack information from interpreted frames. BUG=v8:5822 R=titzer@chromium.org, yangguo@chromium.org ==========
clemensh@chromium.org changed reviewers: + yangguo@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/20720)
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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/20812)
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_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) 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/19219) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19277) 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_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) 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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/32443) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/20929)
Ignore red bots, will rebase before landing.
https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc File src/wasm/wasm-debug.cc (right): https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc#... src/wasm/wasm-debug.cc:40: reinterpret_cast<byte *>(mem_buffer->backing_store()); byte* instead of byte * (I guess clang-format is accepting both these days?) https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... File test/cctest/wasm/test-wasm-breakpoints.cc (right): https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... test/cctest/wasm/test-wasm-breakpoints.cc:72: static CountBreaks *current_handler; Type* instead of Type *, here and elsewhere. https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... test/cctest/wasm/test-wasm-breakpoints.cc:102: auto SetProperty = [obj, isolate](const char *name, Object *value) { Not sure why you need a lambda here, since this is only called once. https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... test/cctest/wasm/test-wasm-breakpoints.cc:188: TEST(TestArgumentPassing) { This test is a bit too complicated, IMO. It is testing both correctness of parameter passing for multiple types and also that breakpoints are hit the right number of times. Can you split it into two different tests (if the first part is not already redundant with another test)?
Rebase still needed. https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc File src/wasm/wasm-debug.cc (right): https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc#... src/wasm/wasm-debug.cc:40: reinterpret_cast<byte *>(mem_buffer->backing_store()); On 2017/01/16 at 13:55:27, titzer wrote: > byte* instead of byte * > > (I guess clang-format is accepting both these days?) Fixed with http://crrev.com/2635003002 and http://crrev.com/2631183002. https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... File test/cctest/wasm/test-wasm-breakpoints.cc (right): https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... test/cctest/wasm/test-wasm-breakpoints.cc:72: static CountBreaks *current_handler; On 2017/01/16 at 13:55:27, titzer wrote: > Type* instead of Type *, here and elsewhere. Same. https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... test/cctest/wasm/test-wasm-breakpoints.cc:102: auto SetProperty = [obj, isolate](const char *name, Object *value) { On 2017/01/16 at 13:55:27, titzer wrote: > Not sure why you need a lambda here, since this is only called once. Oh, right. It was more properties before. Removed it. https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... test/cctest/wasm/test-wasm-breakpoints.cc:188: TEST(TestArgumentPassing) { On 2017/01/16 at 13:55:27, titzer wrote: > This test is a bit too complicated, IMO. > > It is testing both correctness of parameter passing for multiple types and also that breakpoints are hit the right number of times. Can you split it into two different tests (if the first part is not already redundant with another test)? Counting the number of breaks is basically only there to test that we really went through the interpreter. I changed it to only check that the break count increases by one during Execution::Call.
On 2017/01/16 18:51:31, Clemens Hammacher wrote: > Rebase still needed. Ok, I can't give the green button until I see the rebased version :-) But the below comments sound good. > > https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc > File src/wasm/wasm-debug.cc (right): > > https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc#... > src/wasm/wasm-debug.cc:40: reinterpret_cast<byte > *>(mem_buffer->backing_store()); > On 2017/01/16 at 13:55:27, titzer wrote: > > byte* instead of byte * > > > > (I guess clang-format is accepting both these days?) > > Fixed with http://crrev.com/2635003002 and http://crrev.com/2631183002. > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > File test/cctest/wasm/test-wasm-breakpoints.cc (right): > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > test/cctest/wasm/test-wasm-breakpoints.cc:72: static CountBreaks > *current_handler; > On 2017/01/16 at 13:55:27, titzer wrote: > > Type* instead of Type *, here and elsewhere. > > Same. > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > test/cctest/wasm/test-wasm-breakpoints.cc:102: auto SetProperty = [obj, > isolate](const char *name, Object *value) { > On 2017/01/16 at 13:55:27, titzer wrote: > > Not sure why you need a lambda here, since this is only called once. > > Oh, right. It was more properties before. Removed it. > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > test/cctest/wasm/test-wasm-breakpoints.cc:188: TEST(TestArgumentPassing) { > On 2017/01/16 at 13:55:27, titzer wrote: > > This test is a bit too complicated, IMO. > > > > It is testing both correctness of parameter passing for multiple types and > also that breakpoints are hit the right number of times. Can you split it into > two different tests (if the first part is not already redundant with another > test)? > > Counting the number of breaks is basically only there to test that we really > went through the interpreter. I changed it to only check that the break count > increases by one during Execution::Call.
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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19497) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15884) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21146) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
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.
On 2017/01/17 09:48:37, titzer wrote: > On 2017/01/16 18:51:31, Clemens Hammacher wrote: > > Rebase still needed. > > Ok, I can't give the green button until I see the rebased version :-) > > But the below comments sound good. > > > > > https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc > > File src/wasm/wasm-debug.cc (right): > > > > > https://codereview.chromium.org/2629883002/diff/40001/src/wasm/wasm-debug.cc#... > > src/wasm/wasm-debug.cc:40: reinterpret_cast<byte > > *>(mem_buffer->backing_store()); > > On 2017/01/16 at 13:55:27, titzer wrote: > > > byte* instead of byte * > > > > > > (I guess clang-format is accepting both these days?) > > > > Fixed with http://crrev.com/2635003002 and http://crrev.com/2631183002. > > > > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > > File test/cctest/wasm/test-wasm-breakpoints.cc (right): > > > > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > > test/cctest/wasm/test-wasm-breakpoints.cc:72: static CountBreaks > > *current_handler; > > On 2017/01/16 at 13:55:27, titzer wrote: > > > Type* instead of Type *, here and elsewhere. > > > > Same. > > > > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > > test/cctest/wasm/test-wasm-breakpoints.cc:102: auto SetProperty = [obj, > > isolate](const char *name, Object *value) { > > On 2017/01/16 at 13:55:27, titzer wrote: > > > Not sure why you need a lambda here, since this is only called once. > > > > Oh, right. It was more properties before. Removed it. > > > > > https://codereview.chromium.org/2629883002/diff/40001/test/cctest/wasm/test-w... > > test/cctest/wasm/test-wasm-breakpoints.cc:188: TEST(TestArgumentPassing) { > > On 2017/01/16 at 13:55:27, titzer wrote: > > > This test is a bit too complicated, IMO. > > > > > > It is testing both correctness of parameter passing for multiple types and > > also that breakpoints are hit the right number of times. Can you split it into > > two different tests (if the first part is not already redundant with another > > test)? > > > > Counting the number of breaks is basically only there to test that we really > > went through the interpreter. I changed it to only check that the break count > > increases by one during Execution::Call. lgtm.
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/2629883002/diff/120001/test/cctest/wasm/test-... File test/cctest/wasm/test-wasm-breakpoints.cc (right): https://codereview.chromium.org/2629883002/diff/120001/test/cctest/wasm/test-... test/cctest/wasm/test-wasm-breakpoints.cc:192: TEST(TestArgumentPassing) { Can you split this test up as per previous comment? It seems to be doing too much.
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/2629883002/diff/120001/test/cctest/wasm/test-... File test/cctest/wasm/test-wasm-breakpoints.cc (right): https://codereview.chromium.org/2629883002/diff/120001/test/cctest/wasm/test-... test/cctest/wasm/test-wasm-breakpoints.cc:192: TEST(TestArgumentPassing) { On 2017/01/20 at 13:08:04, titzer wrote: > Can you split this test up as per previous comment? It seems to be doing too much. As discussed offline: Removed argument passing test for now, will refactor and add in a separate CL.
lgtm
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 yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2629883002/#ps120002 (title: "Remove test for argument passing")
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": 120002, "attempt_start_ts": 1484920101020540,
"parent_rev": "fba9047314fabf3f9cd1b68bf67158e544944348", "commit_rev":
"a1e04ef524e9705da304acd0023e28602d0af978"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Add tests for breakpoints Test that setting breakpoints works for wasm, and that they are hit correctly. This basically tests all the layers involved: Compiling and running wasm interpreter entries, passing arguments to the interpreter, storing break point infos in wasm objects, getting the right BreakLocation from wasm frames, and getting stack information from interpreted frames. BUG=v8:5822 R=titzer@chromium.org, yangguo@chromium.org ========== to ========== [wasm] Add tests for breakpoints Test that setting breakpoints works for wasm, and that they are hit correctly. This basically tests all the layers involved: Compiling and running wasm interpreter entries, passing arguments to the interpreter, storing break point infos in wasm objects, getting the right BreakLocation from wasm frames, and getting stack information from interpreted frames. BUG=v8:5822 R=titzer@chromium.org, yangguo@chromium.org Review-Url: https://codereview.chromium.org/2629883002 Cr-Commit-Position: refs/heads/master@{#42560} Committed: https://chromium.googlesource.com/v8/v8/+/a1e04ef524e9705da304acd0023e28602d0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:120002) as https://chromium.googlesource.com/v8/v8/+/a1e04ef524e9705da304acd0023e28602d0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
