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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -67 lines) Patch
M src/asmjs/asm-parser.h View 1 2 3 4 5 6 7 8 3 chunks +5 lines, -3 lines 0 comments Download
M src/asmjs/asm-parser.cc View 1 2 3 4 5 6 7 8 9 10 12 chunks +64 lines, -19 lines 0 comments Download
M src/asmjs/asm-scanner.cc View 1 chunk +3 lines, -1 line 0 comments Download
M src/wasm/wasm-module.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M src/wasm/wasm-module-builder.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download
M test/mjsunit/asm/asm-validation.js View 1 2 3 1 chunk +0 lines, -17 lines 0 comments Download
M test/mjsunit/mjsunit.status View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -25 lines 0 comments Download

Messages

Total messages: 62 (47 generated)
bradn
3 years, 9 months ago (2017-03-24 06:53:03 UTC) #4
bradn
Hmmn, seems I've got some red on win/mac, looks like something unstable, as it's generating ...
3 years, 9 months ago (2017-03-24 07:25:43 UTC) #7
Clemens Hammacher
Would it maybe make sense to just remove the flags, and test it via variants? ...
3 years, 9 months ago (2017-03-24 09:09:17 UTC) #9
Michael Starzinger
https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builder.cc File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builder.cc#newcode195 src/wasm/wasm-module-builder.cc:195: // TODO(bradnelson): Figure out why the memcpy crashes if ...
3 years, 9 months ago (2017-03-24 11:56:05 UTC) #10
Michael Starzinger
On 2017/03/24 09:09:17, Clemens Hammacher wrote: > Would it maybe make sense to just remove ...
3 years, 9 months ago (2017-03-24 11:58:00 UTC) #11
bradnelson
PTAL https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builder.cc File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/1/src/wasm/wasm-module-builder.cc#newcode195 src/wasm/wasm-module-builder.cc:195: // TODO(bradnelson): Figure out why the memcpy crashes ...
3 years, 9 months ago (2017-03-28 04:34:31 UTC) #42
Clemens Hammacher
Just one comment, leaving the rest to those who know the code already. https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-builder.cc File ...
3 years, 9 months ago (2017-03-28 09:52:02 UTC) #43
Michael Starzinger
LGTM from my end. Nice. Thanks! Gotta love those erratas. :)
3 years, 9 months ago (2017-03-28 10:39:54 UTC) #44
bradnelson
https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-builder.cc File src/wasm/wasm-module-builder.cc (right): https://codereview.chromium.org/2771183002/diff/160001/src/wasm/wasm-module-builder.cc#newcode196 src/wasm/wasm-module-builder.cc:196: // Early out here as body_[position] is out of ...
3 years, 8 months ago (2017-03-28 15:30:50 UTC) #45
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/2771183002/180001
3 years, 8 months ago (2017-03-28 15:31:20 UTC) #48
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/v8/v8/+/be0dbdd679b60c31d480d7635e579787a6a218df
3 years, 8 months ago (2017-03-28 15:53:29 UTC) #51
bradnelson
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2782613002/ by bradnelson@chromium.org. ...
3 years, 8 months ago (2017-03-28 17:03:56 UTC) #52
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/2771183002/200001
3 years, 8 months ago (2017-03-28 17:12:48 UTC) #56
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/2771183002/220001
3 years, 8 months ago (2017-03-28 17:19:37 UTC) #59
commit-bot: I haz the power
3 years, 8 months ago (2017-03-28 17:43:20 UTC) #62
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/v8/v8/+/a84da1c3b7fcb44ade1d49d97b8c51ba2d8...

Powered by Google App Engine
This is Rietveld 408576698