Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 11 months ago by rossberg Modified:
3 years, 11 months ago Reviewers:
titzer CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Catch attempt to export non-existent memory
R=titzer@chromium.org
BUG=v8:5840
Review-Url: https://codereview.chromium.org/2633153002
Cr-Commit-Position: refs/heads/master@{#42384}
Committed: https://chromium.googlesource.com/v8/v8/+/39e455db0d9fe5a57fb2eccc8811ffb7b52985d9
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added TODO #
Created: 3 years, 11 months ago
Messages
Total messages: 17 (6 generated)
https://codereview.chromium.org/2633153002/diff/1/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2633153002/diff/1/src/wasm/module-decoder.cc#... src/wasm/module-decoder.cc:460: if (!module->has_memory || index != 0) { Should we just replace this with a counter of the number of memorys in the WasmModule struct?
The CQ bit was checked by rossberg@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2633153002/diff/1/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2633153002/diff/1/src/wasm/module-decoder.cc#... src/wasm/module-decoder.cc:460: if (!module->has_memory || index != 0) { On 2017/01/16 15:54:16, titzer wrote: > Should we just replace this with a counter of the number of memorys in the > WasmModule struct? Added TODO
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by titzer@chromium.org
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": 20001, "attempt_start_ts": 1484587087103840,
"parent_rev": "38088853de4c88acfae548d249de29992a993f0b", "commit_rev":
"39e455db0d9fe5a57fb2eccc8811ffb7b52985d9"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Catch attempt to export non-existent memory R=titzer@chromium.org BUG=v8:5840 ========== to ========== [wasm] Catch attempt to export non-existent memory R=titzer@chromium.org BUG=v8:5840 Review-Url: https://codereview.chromium.org/2633153002 Cr-Commit-Position: refs/heads/master@{#42384} Committed: https://chromium.googlesource.com/v8/v8/+/39e455db0d9fe5a57fb2eccc8811ffb7b52... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/39e455db0d9fe5a57fb2eccc8811ffb7b52...
Message was sent while issue was closed.
On 2017/01/16 17:44:53, commit-bot: I haz the power wrote: > Committed patchset #2 (id:20001) as > https://chromium.googlesource.com/v8/v8/+/39e455db0d9fe5a57fb2eccc8811ffb7b52... Could you point me to the place in the spec where this is captured - because I think we have an inconsistency; look at https://github.com/WebAssembly/design/blob/master/JS.md#webassemblyinstance-o..., see the "exports" part, from "3" on - it seems to suggest (the iii part) that in the non-existent case, we create a memory (that's a bit weird, frankly) The change here would trigger an error before that would happen. My guess is we want what your change does, but then we need to reconcile the API spec.
Message was sent while issue was closed.
On 2017/01/16 21:45:02, Mircea Trofin wrote: > On 2017/01/16 17:44:53, commit-bot: I haz the power wrote: > > Committed patchset #2 (id:20001) as > > > https://chromium.googlesource.com/v8/v8/+/39e455db0d9fe5a57fb2eccc8811ffb7b52... > > Could you point me to the place in the spec where this is captured - because I > think we have an inconsistency; look at > https://github.com/WebAssembly/design/blob/master/JS.md#webassemblyinstance-o..., > see the "exports" part, from "3" on - it seems to suggest (the iii part) that in > the non-existent case, we create a memory (that's a bit weird, frankly) > > The change here would trigger an error before that would happen. > > My guess is we want what your change does, but then we need to reconcile the API > spec. It wasn't an API bug, it was a validation bug. Obviously, you shouldn't be able to export something that's not bound. The spec interpreter requires the check here, by looking up the item in the context: https://github.com/WebAssembly/spec/blob/master/interpreter/spec/valid.ml#L403 The part from JS.md you cite is about creating a JS `WebAssembly.Memory` wrapper object if none exists yet, not the memory itself (that would be `m` in the algo, which already exists). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
