[wasm] Implement WebAssembly.Module.customSections.

[wasm] Implement WebAssembly.Module.customSections.

R=ahaas@chromium.org,rossberg@chromium.org
BUG=chromium:575167
Review-Url: https://codereview.chromium.org/2626263004
Cr-Commit-Position: refs/heads/master@{#42634}
Committed: https://chromium.googlesource.com/v8/v8/+/68efecff6009b7cae0b2ac1ddedce7810ec86e00

[ahaas, 2017-01-13 15:01:26]

https://codereview.chromium.org/2626263004/diff/1/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2626263004/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:1251: byte section_code = decoder.consume_u8("section
code");
The section code should be a varuint7.

[ahaas, 2017-01-13 16:51:39]

On 2017/01/13 at 15:01:26, ahaas wrote:
> https://codereview.chromium.org/2626263004/diff/1/src/wasm/module-decoder.cc
> File src/wasm/module-decoder.cc (right):
> 
>
https://codereview.chromium.org/2626263004/diff/1/src/wasm/module-decoder.cc#...
> src/wasm/module-decoder.cc:1251: byte section_code =
decoder.consume_u8("section code");
> The section code should be a varuint7.

lgtm after comment is resolved.

[titzer, 2017-01-23 18:23:22]

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

[commit-bot: I haz the power, 2017-01-23 18:23:30]

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

[titzer, 2017-01-23 18:24:13]

On 2017/01/13 16:51:39, ahaas wrote:
> On 2017/01/13 at 15:01:26, ahaas wrote:
> > https://codereview.chromium.org/2626263004/diff/1/src/wasm/module-decoder.cc
> > File src/wasm/module-decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/2626263004/diff/1/src/wasm/module-decoder.cc#...
> > src/wasm/module-decoder.cc:1251: byte section_code =
> decoder.consume_u8("section code");
> > The section code should be a varuint7.
> 
> lgtm after comment is resolved.

We don't have such a type in the decoder; varuint7 is equivalent to a byte
(cannot have continuation bytes as per binary encoding spec).

[titzer, 2017-01-23 18:24:49]

Please take another look, as the semantics of WebAssembly.Module.customSections
has been clarified since the original CL.

[commit-bot: I haz the power, 2017-01-23 18:55:03]

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

[commit-bot: I haz the power, 2017-01-23 18:55:04]

Dry run: This issue passed the CQ dry run.

[rossberg, 2017-01-24 11:34:42]

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:2738: // TODO(titzer): throw out of memory?
Yes please.

https://codereview.chromium.org/2626263004/diff/20001/test/mjsunit/wasm/js-ap...
File test/mjsunit/wasm/js-api.js (right):

https://codereview.chromium.org/2626263004/diff/20001/test/mjsunit/wasm/js-ap...
test/mjsunit/wasm/js-api.js:300: builder.addExplicitSection([
Can you add another section not named "foo", to test that it is not included?

[ahaas, 2017-01-24 11:35:13]

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:266: if (!maybe_module.is_null()) {
You do not set the return value when maybe_module.is_null(). Is the return value
always initialized correctly?

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:282: if (!maybe_module.is_null()) {
Same here.

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:310: if (!maybe_module.is_null()) {
same here.

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:327: if (!maybe_module.is_null()) {
same here.

[titzer, 2017-01-24 12:04:42]

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

[commit-bot: I haz the power, 2017-01-24 12:04:53]

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, 2017-01-24 12:37:12]

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

[commit-bot: I haz the power, 2017-01-24 12:37:12]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-24 12:38:39]

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:266: if (!maybe_module.is_null()) {
On 2017/01/24 11:35:13, ahaas wrote:
> You do not set the return value when maybe_module.is_null(). Is the return
value
> always initialized correctly?

In these cases the ErrorThrower has an error and will throw it, so the return
value does not matter.

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:2738: // TODO(titzer): throw out of memory?
On 2017/01/24 11:34:42, rossberg wrote:
> Yes please.

Done.

https://codereview.chromium.org/2626263004/diff/20001/test/mjsunit/wasm/js-ap...
File test/mjsunit/wasm/js-api.js (right):

https://codereview.chromium.org/2626263004/diff/20001/test/mjsunit/wasm/js-ap...
test/mjsunit/wasm/js-api.js:300: builder.addExplicitSection([
On 2017/01/24 11:34:42, rossberg wrote:
> Can you add another section not named "foo", to test that it is not included?

Well, the section above is called 'x', but I added another one called 'fox'.

[ahaas, 2017-01-24 12:42:21]

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc
File src/wasm/wasm-js.cc (right):

https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:266: if (!maybe_module.is_null()) {
On 2017/01/24 at 12:38:39, titzer wrote:
> On 2017/01/24 11:35:13, ahaas wrote:
> > You do not set the return value when maybe_module.is_null(). Is the return
value
> > always initialized correctly?
> 
> In these cases the ErrorThrower has an error and will throw it, so the return
value does not matter.

In that case, could we add a DCHECK that there really was an error, as you do in
line 332?

[titzer, 2017-01-24 12:47:17]

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

[commit-bot: I haz the power, 2017-01-24 12:47:21]

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

[titzer, 2017-01-24 12:48:18]

On 2017/01/24 12:42:21, ahaas wrote:
> https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc
> File src/wasm/wasm-js.cc (right):
> 
>
https://codereview.chromium.org/2626263004/diff/20001/src/wasm/wasm-js.cc#new...
> src/wasm/wasm-js.cc:266: if (!maybe_module.is_null()) {
> On 2017/01/24 at 12:38:39, titzer wrote:
> > On 2017/01/24 11:35:13, ahaas wrote:
> > > You do not set the return value when maybe_module.is_null(). Is the return
> value
> > > always initialized correctly?
> > 
> > In these cases the ErrorThrower has an error and will throw it, so the
return
> value does not matter.
> 
> In that case, could we add a DCHECK that there really was an error, as you do
in
> line 332?

Done.

[ahaas, 2017-01-24 12:55:37]

lgtm

[commit-bot: I haz the power, 2017-01-24 13:05:40]

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

[commit-bot: I haz the power, 2017-01-24 13:05:41]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19874)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[rossberg, 2017-01-24 13:06:53]

lgtm

[titzer, 2017-01-24 14:28:28]

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

[commit-bot: I haz the power, 2017-01-24 14:28:39]

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, 2017-01-24 15:03:04]

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

[commit-bot: I haz the power, 2017-01-24 15:03:05]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-24 15:20:58]

The CQ bit was checked by titzer@chromium.org

[titzer, 2017-01-24 15:20:59]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
rossberg@chromium.org
Link to the patchset: https://codereview.chromium.org/2626263004/#ps80001
(title: "Remove DCHECKs again")

[commit-bot: I haz the power, 2017-01-24 15:21:05]

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-24 15:23:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485271258748890,
"parent_rev": "7524e523d7d64dd63ae6d9a3f3e13bb46398606d", "commit_rev":
"68efecff6009b7cae0b2ac1ddedce7810ec86e00"}

[commit-bot: I haz the power, 2017-01-24 15:23:07]

Description was changed from

==========
[wasm] Implement WebAssembly.Module.customSections.

R=ahaas@chromium.org,rossberg@chromium.org
BUG=chromium:575167
==========

to

==========
[wasm] Implement WebAssembly.Module.customSections.

R=ahaas@chromium.org,rossberg@chromium.org
BUG=chromium:575167

Review-Url: https://codereview.chromium.org/2626263004
Cr-Commit-Position: refs/heads/master@{#42634}
Committed:
https://chromium.googlesource.com/v8/v8/+/68efecff6009b7cae0b2ac1ddedce7810ec...
==========

[commit-bot: I haz the power, 2017-01-24 15:23:08]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/68efecff6009b7cae0b2ac1ddedce7810ec...