[wasm] Catch attempt to export non-existent memory

[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

[titzer, 2017-01-16 15:54:16]

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?

[rossberg, 2017-01-16 17:03:00]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:03:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 17:03:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:03:12]

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.

[rossberg, 2017-01-16 17:03:12]

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

[commit-bot: I haz the power, 2017-01-16 17:03:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:03:23]

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.

[titzer, 2017-01-16 17:09:05]

lgtm

[titzer, 2017-01-16 17:18:07]

The CQ bit was checked by titzer@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:18:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 17:44:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484587087103840,
"parent_rev": "38088853de4c88acfae548d249de29992a993f0b", "commit_rev":
"39e455db0d9fe5a57fb2eccc8811ffb7b52985d9"}

[commit-bot: I haz the power, 2017-01-16 17:44:52]

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...
==========

[commit-bot: I haz the power, 2017-01-16 17:44:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/39e455db0d9fe5a57fb2eccc8811ffb7b52...

[Mircea Trofin, 2017-01-16 21:45:02]

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.

[rossberg, 2017-01-17 10:37:40]

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).