Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2771183002:
[wasm][asm.js] Fix and enable several asm.js tests with the new parser. (Closed)
|
Created:
3 years, 9 months ago by bradnelson Modified:
3 years, 8 months ago CC:
v8-reviews_googlegroups.com, wasm-v8_google.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[wasm][asm.js] Fix and enable several asm.js tests with the new parser.
Fix a few items broken during review of scanner + parser:
* Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit).
* Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO).
Also fix:
* Drop test based on constant expression evaluation in main parser
* Support constant defined based on existing constant.
* Type constants as signed.
* Added a check that all used functions are defined eventually.
* Zone allocate strings for simplicity (TODOs to refactor better).
BUG=v8:6090
BUG=v8:4203
R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org
Review-Url: https://codereview.chromium.org/2771183002
Cr-Original-Commit-Position: refs/heads/master@{#44200}
Committed: https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a218df
Review-Url: https://codereview.chromium.org/2771183002
Cr-Commit-Position: refs/heads/master@{#44203}
Committed: https://chromium.googlesource.com/v8/v8/+/a84da1c3b7fcb44ade1d49d97b8c51ba2d89a8f7
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix #Patch Set 3 : fix #Patch Set 4 : fix #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : fix #Patch Set 8 : fix #Patch Set 9 : fix #
Total comments: 2
Patch Set 10 : fix #Patch Set 11 : fix failure #Patch Set 12 : fix one more item #
Messages
Total messages: 62 (47 generated)
The CQ bit was checked by bradnelson@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...
bradnelson@google.com changed reviewers: + bradnelson@google.com
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/24795) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
Hmmn, seems I've got some red on win/mac, looks like something unstable, as it's generating wasm that's failing validation (but for programs that work in some contexts). It may be GetVarInfo, which is in need of refactoring anyhow. Will look tomorrow.
clemensh@chromium.org changed reviewers: + clemensh@chromium.org
Would it maybe make sense to just remove the flags, and test it via variants? If we hard-code the flags, we essentially disable variants testing.
https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builde... File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builde... src/wasm/wasm-module-builder.cc:195: // TODO(bradnelson): Figure out why the memcpy crashes if len == 0. It is not the memcpy that crashes but the accessor to body_[position] if position == body_.size(). This is an out-of-bounds access. I think the early return is the best fix for this case. On a related note, what about the case where position > body_.size()? Do we care about this case? Could we add a DCHECK_LE(position, body_.size()) into this method?
On 2017/03/24 09:09:17, Clemens Hammacher wrote: > Would it maybe make sense to just remove the flags, and test it via variants? > If we hard-code the flags, we essentially disable variants testing. I agree. One of my CLs changes the test variant. If you rebase this CL then all that is needed to increase test coverage is remove the skips from the status file. This is the CL im referring to: https://chromium-review.googlesource.com/c/459536/
The CQ bit was checked by bradnelson@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][asm.js] Switch most tests to the new asm.js parser. Switch most but not all --validate-asm tests to --fast-validate-asm. Add both to some tests that should have had it (that were added during the time --validate-asm was the default). Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ========== to ========== [wasm][asm.js] Fix several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ==========
The CQ bit was checked by bradnelson@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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by bradnelson@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][asm.js] Fix several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ========== to ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by bradnelson@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][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ========== to ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ==========
The CQ bit was checked by bradnelson@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/23382) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/19874) 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...)
The CQ bit was checked by bradnelson@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 bradnelson@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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/25071) 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 bradnelson@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][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ========== to ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builde... File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builde... src/wasm/wasm-module-builder.cc:195: // TODO(bradnelson): Figure out why the memcpy crashes if len == 0. On 2017/03/24 11:56:05, Michael Starzinger wrote: > It is not the memcpy that crashes but the accessor to body_[position] if > position == body_.size(). This is an out-of-bounds access. I think the early > return is the best fix for this case. > > On a related note, what about the case where position > body_.size()? Do we care > about this case? Could we add a DCHECK_LE(position, body_.size()) into this > method? Done.
Just one comment, leaving the rest to those who know the code already. https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-b... File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-b... src/wasm/wasm-module-builder.cc:196: // Early out here as body_[position] is out of bounds in this case. dst->resize(0) is not needed here? You can avoid this special case for len == 0 by changing "&body_[position]" to "body_.data() + position".
LGTM from my end. Nice. Thanks! Gotta love those erratas. :)
https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-b... File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-b... src/wasm/wasm-module-builder.cc:196: // Early out here as body_[position] is out of bounds in this case. On 2017/03/28 09:52:02, Clemens Hammacher wrote: > dst->resize(0) is not needed here? > > You can avoid this special case for len == 0 by changing "&body_[position]" to > "body_.data() + position". Ah good point. Done.
The CQ bit was checked by bradnelson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2771183002/#ps180001 (title: "fix")
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": 180001, "attempt_start_ts": 1490715077476870,
"parent_rev": "80b26b4f9124a2d96b45d47228cb9b23dceaef16", "commit_rev":
"be0dbdd679b60c31d480d7635e579787a6a218df"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org ========== to ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2771183002 Cr-Commit-Position: refs/heads/master@{#44200} Committed: https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a...
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2782613002/ by bradnelson@chromium.org. The reason for reverting is: Fails on gc-stress..
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2771183002 Cr-Commit-Position: refs/heads/master@{#44200} Committed: https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a... ========== to ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2771183002 Cr-Commit-Position: refs/heads/master@{#44200} Committed: https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a... ==========
The CQ bit was checked by bradnelson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2771183002/#ps200001 (title: "fix failure")
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 bradnelson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2771183002/#ps220001 (title: "fix one more item")
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": 220001, "attempt_start_ts": 1490721567774830,
"parent_rev": "2b86bb74610152f319fd50135481884beb4727b0", "commit_rev":
"a84da1c3b7fcb44ade1d49d97b8c51ba2d89a8f7"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2771183002 Cr-Commit-Position: refs/heads/master@{#44200} Committed: https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a... ========== to ========== [wasm][asm.js] Fix and enable several asm.js tests with the new parser. Fix a few items broken during review of scanner + parser: * Make the scanner retain stale newline state on a rewind (as otherwise it must be able to correctly rewind that too, though it doesn't need it). (Probably should revisit). * Change StashCode in the builder skip to the zero case, as it crashes for some reason (added TODO). Also fix: * Drop test based on constant expression evaluation in main parser * Support constant defined based on existing constant. * Type constants as signed. * Added a check that all used functions are defined eventually. * Zone allocate strings for simplicity (TODOs to refactor better). BUG=v8:6090 BUG=v8:4203 R=mstarzinger@chromium.org,marja@chromium.org,vogelheim@chromium.org Review-Url: https://codereview.chromium.org/2771183002 Cr-Original-Commit-Position: refs/heads/master@{#44200} Committed: https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a... Review-Url: https://codereview.chromium.org/2771183002 Cr-Commit-Position: refs/heads/master@{#44203} Committed: https://chromium.googlesource.com/v8/v8/+/a84da1c3b7fcb44ade1d49d97b8c51ba2d8... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/v8/v8/+/a84da1c3b7fcb44ade1d49d97b8c51ba2d8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
