[wasm] Remove raw byte pointers from WasmModule

[wasm] Remove raw byte pointers from WasmModule

These byte pointers (module_start and module_end) were only valid
during decoding. During instantiation or execution, they can get
invalidated by garbage collection.
This CL removes them from the WasmModule struct, and introduces a new
ModuleStorage struct as interface to the wasm wire bytes.
Since the storage is often needed together with the ModuleEnv, a new
ModuleStorageEnv struct holds both a ModuleEnv and a ModuleStorage.
The pointers in the ModuleStorage should never escape the live range of
this struct, as they might point into a SeqOneByteString or ArrayBuffer.
Therefore, the WasmInterpreter needs to create its own copy of the
whole module.
Runtime functions that previously used the raw pointers in WasmModule
(leading to memory errors) now have to use the SeqOneByteString in the
WasmCompiledModule.

R=titzer@chromium.org
BUG=chromium:669518
Committed: https://crrev.com/6572b5622ecb5311d05787a8e5c08795e0a50b57
Cr-Commit-Position: refs/heads/master@{#41388}

[Clemens Hammacher, 2016-11-30 12:18:57]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 12:19:05]

Dry run: 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, 2016-11-30 12:50:39]

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

[commit-bot: I haz the power, 2016-11-30 12:50:40]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-11-30 13:13:42]

lgtm

I like this refactoring, as it now makes everything explicit on where the bytes
are actually needed.

However I think we need to finally figure out a way to keep a single copy of the
module bytes that is nonmoving and therefore always accessible from C++. That
probably requires some special handling in the serializer.

https://codereview.chromium.org/2540133002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2540133002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1763: const ZoneVector<uint8_t> module_bytes_;
I think this is OK for now, but we should find a way to avoid making a copy of
the bytes yet again for the interpreter. E.g. it seems like we should eventually
have a shared non-moving copy of the module bytes living in C++ land and handle
serialization of it specially instead of piggy-backing on
SequentialOneByteString.

https://codereview.chromium.org/2540133002/diff/1/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2540133002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:180: struct ModuleStorage;
Can we call this ModuleWireBytes? or ModuleRawBytes? The idea is that this
represents the bytes we get over the wire or from disk.

[Clemens Hammacher, 2016-11-30 13:35:46]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 13:35:54]

Dry run: 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, 2016-11-30 14:02:00]

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

[commit-bot: I haz the power, 2016-11-30 14:02:01]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-11-30 14:28:44]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-11-30 14:28:45]

https://codereview.chromium.org/2540133002/diff/1/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2540133002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:180: struct ModuleStorage;
On 2016/11/30 at 13:13:42, titzer wrote:
> Can we call this ModuleWireBytes? or ModuleRawBytes? The idea is that this
represents the bytes we get over the wire or from disk.

I renamed ModuleStorage to ModuleWireBytes and ModuleStorageEnv to
ModuleBytesEnv.

[Clemens Hammacher, 2016-11-30 14:28:45]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2540133002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2016-11-30 14:28:50]

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, 2016-11-30 15:02:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480516124650680,
"parent_rev": "f5621952e464a0efee1118dd78f6cd7886ac62ad", "commit_rev":
"c2e8bb339e73337e13f7d2702b4a77f9956dc906"}

[commit-bot: I haz the power, 2016-11-30 15:02:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-30 15:03:21]

Description was changed from

==========
[wasm] Remove raw byte pointers from WasmModule

These byte pointers (module_start and module_end) were only valid
during decoding. During instantiation or execution, they can get
invalidated by garbage collection.
This CL removes them from the WasmModule struct, and introduces a new
ModuleStorage struct as interface to the wasm wire bytes.
Since the storage is often needed together with the ModuleEnv, a new
ModuleStorageEnv struct holds both a ModuleEnv and a ModuleStorage.
The pointers in the ModuleStorage should never escape the live range of
this struct, as they might point into a SeqOneByteString or ArrayBuffer.
Therefore, the WasmInterpreter needs to create its own copy of the
whole module.
Runtime functions that previously used the raw pointers in WasmModule
(leading to memory errors) now have to use the SeqOneByteString in the
WasmCompiledModule.

R=titzer@chromium.org
BUG=chromium:669518
==========

to

==========
[wasm] Remove raw byte pointers from WasmModule

These byte pointers (module_start and module_end) were only valid
during decoding. During instantiation or execution, they can get
invalidated by garbage collection.
This CL removes them from the WasmModule struct, and introduces a new
ModuleStorage struct as interface to the wasm wire bytes.
Since the storage is often needed together with the ModuleEnv, a new
ModuleStorageEnv struct holds both a ModuleEnv and a ModuleStorage.
The pointers in the ModuleStorage should never escape the live range of
this struct, as they might point into a SeqOneByteString or ArrayBuffer.
Therefore, the WasmInterpreter needs to create its own copy of the
whole module.
Runtime functions that previously used the raw pointers in WasmModule
(leading to memory errors) now have to use the SeqOneByteString in the
WasmCompiledModule.

R=titzer@chromium.org
BUG=chromium:669518

Committed: https://crrev.com/6572b5622ecb5311d05787a8e5c08795e0a50b57
Cr-Commit-Position: refs/heads/master@{#41388}
==========

[commit-bot: I haz the power, 2016-11-30 15:03:22]

Patchset 3 (id:??) landed as
https://crrev.com/6572b5622ecb5311d05787a8e5c08795e0a50b57
Cr-Commit-Position: refs/heads/master@{#41388}