Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640953002:
[wasm] Add a more exhaustive test for unreachable code validation. (Closed)
Description[wasm] Add a more exhaustive test for unreachable code validation.
R=rossberg@chromium.org
BUG=
Review-Url: https://codereview.chromium.org/2640953002
Cr-Commit-Position: refs/heads/master@{#42499}
Committed: https://chromium.googlesource.com/v8/v8/+/097e1ac6c74992b47aedee4cd3c98475438a38e1
Patch Set 1 #
Total comments: 5
Patch Set 2 : [wasm] Add a more exhaustive test for unreachable code validation. #Patch Set 3 : Add more tests #
Dependent Patchsets: Messages
Total messages: 20 (13 generated)
The CQ bit was checked by titzer@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...
On 2017/01/18 19:12:21, titzer wrote: PTAL. The idea is that this test succinctly shows what passes/fails under the various proposals. It also has support to use "X" as an expectation that there is no assertion made about whether it passes or fails. I've just put in the current V8 behavior for now, but it will be easy to see in subsequent CLs how the proposals affect what is accepted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with suggestions https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... File test/mjsunit/wasm/unreachable-validation.js (right): https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... test/mjsunit/wasm/unreachable-validation.js:30: if (expected) assertTrue(valid); Nit: why not assertTrue(valid === expected) ? https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... test/mjsunit/wasm/unreachable-validation.js:71: run(V, "(block U) iadd", [...block_unr, iadd]); Could add "(block (block U)) iadd" https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... test/mjsunit/wasm/unreachable-validation.js:84: run(I, "U F32 iadd", [unr, ...f32, iadd]); You probably want to add a 0 operand before the F32 in all these, so that failure is really due to types, not stack depth (or add that as a separate variant). https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... test/mjsunit/wasm/unreachable-validation.js:88: run(I, "(block (block U)) F32 iadd", [...block_block_unr, ...f32, iadd]); Another relevant category: "0 F32 U iadd" "0 F32 (block U) iadd" "0 F32 (loop U) iadd" etc
https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... File test/mjsunit/wasm/unreachable-validation.js (right): https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... test/mjsunit/wasm/unreachable-validation.js:68: run(V, "U iadd", [unr, iadd]); Oh, and you probably want to add drops here (and for others) as well, so that a return-type mismatch is removed from the equation.
The CQ bit was checked by titzer@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 checked by titzer@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...
On 2017/01/19 08:32:26, rossberg wrote: > https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... > File test/mjsunit/wasm/unreachable-validation.js (right): > > https://codereview.chromium.org/2640953002/diff/1/test/mjsunit/wasm/unreachab... > test/mjsunit/wasm/unreachable-validation.js:68: run(V, "U iadd", [unr, iadd]); > Oh, and you probably want to add drops here (and for others) as well, so that a > return-type mismatch is removed from the equation. Thanks for the suggestions. I added a bunch more tests and realized that my recent cleanup CL actually causes the decoder to reject too much now. Landing with current behavior and will fix in the next CL.
The CQ bit was unchecked by titzer@chromium.org
The CQ bit was checked by titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/2640953002/#ps40001 (title: "Add more tests")
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": 40001, "attempt_start_ts": 1484829219173160,
"parent_rev": "1228306d30be5fdf388cf9f1d0db7fd4d7a3c497", "commit_rev":
"097e1ac6c74992b47aedee4cd3c98475438a38e1"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Add a more exhaustive test for unreachable code validation. R=rossberg@chromium.org BUG= ========== to ========== [wasm] Add a more exhaustive test for unreachable code validation. R=rossberg@chromium.org BUG= Review-Url: https://codereview.chromium.org/2640953002 Cr-Commit-Position: refs/heads/master@{#42499} Committed: https://chromium.googlesource.com/v8/v8/+/097e1ac6c74992b47aedee4cd3c98475438... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/097e1ac6c74992b47aedee4cd3c98475438... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
