wasm: use strings for section names

wasm: use strings for section names

This will require an equivalent sexpr-wasm change.

See:
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#high-level-structure

R=titzer@chromium.org, binji@chronium.org

Committed: https://crrev.com/abbdca947f8b1f4967ff71700cca8cbc326df5cd
Cr-Commit-Position: refs/heads/master@{#34668}

[JF, 2016-03-04 04:30:14]

titzer / binji if this part lgty I'll do the tests. I want to avoid racing with
other CLs on updating all the tests :-)

[titzer, 2016-03-07 08:45:33]

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

https://codereview.chromium.org/1765843002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:82: uint32_t section_bytes = consume_u32v(&length,
"section size");
Should the section size also include the string size? (I think yes). In that
case the below code needs check the string is contained in the section size and
only skip the remaining bytes.

[JF, 2016-03-09 02:38:58]

I think this is mostly done, but we're now all racing on the tests, and they're
becoming impossible to maintain.

I'm not sure fixing the remainder of the tests is productive... We need a better
way to test, and to not race on them. How do you suggest we move forward?

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

https://codereview.chromium.org/1765843002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:82: uint32_t section_bytes = consume_u32v(&length,
"section size");
On 2016/03/07 08:45:33, titzer wrote:
> Should the section size also include the string size? (I think yes). In that
> case the below code needs check the string is contained in the section size
and
> only skip the remaining bytes.

Agreed, fixed.

[titzer, 2016-03-09 13:19:38]

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
src/wasm/module-decoder.cc:83: uint32_t section_bytes = consume_u32v(&length,
"section size");
section_size

*_bytes sounds like it might refer to the actual data.

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
src/wasm/module-decoder.cc:85: uint32_t section_string_bytes;
section_string_size

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
src/wasm/module-decoder.cc:510: if (WasmSection::getNameLength(i) ==
*string_bytes &&
Can you drop in a TODO here? It's a linear search...

[titzer, 2016-03-09 13:19:59]

On 2016/03/09 02:38:58, JF wrote:
> I think this is mostly done, but we're now all racing on the tests, and
they're
> becoming impossible to maintain.
> 
> I'm not sure fixing the remainder of the tests is productive... We need a
better
> way to test, and to not race on them. How do you suggest we move forward?
> 

Please just fix them.

> https://codereview.chromium.org/1765843002/diff/1/src/wasm/module-decoder.cc
> File src/wasm/module-decoder.cc (right):
> 
>
https://codereview.chromium.org/1765843002/diff/1/src/wasm/module-decoder.cc#...
> src/wasm/module-decoder.cc:82: uint32_t section_bytes = consume_u32v(&length,
> "section size");
> On 2016/03/07 08:45:33, titzer wrote:
> > Should the section size also include the string size? (I think yes). In that
> > case the below code needs check the string is contained in the section size
> and
> > only skip the remaining bytes.
> 
> Agreed, fixed.

[JF, 2016-03-09 18:51:52]

I've got some more fixes in, but still need to go through the code again to make
sure it's all correct.

These tests all pass:
  ./tools/run-tests.py -j8 --variants=default --timeout=10 --arch=x64
--mode=debug --no-presubmit unittests

This still fails:
  ./tools/run-tests.py -j8 --variants=default --timeout=10 --arch=x64
--mode=debug --no-presubmit mjsunit/wasm/*
I haven't looked into why yet.


On 2016/03/09 13:19:59, titzer wrote:
> On 2016/03/09 02:38:58, JF wrote:
> > I think this is mostly done, but we're now all racing on the tests, and
> they're
> > becoming impossible to maintain.
> > 
> > I'm not sure fixing the remainder of the tests is productive... We need a
> better
> > way to test, and to not race on them. How do you suggest we move forward?
> > 
> 
> Please just fix them.

Sounds good, as long as we don't race too often :)
I haven't rebased yet, will do when the current version passes all tests.

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
src/wasm/module-decoder.cc:83: uint32_t section_bytes = consume_u32v(&length,
"section size");
On 2016/03/09 13:19:37, titzer wrote:
> section_size
> 
> *_bytes sounds like it might refer to the actual data.

Done.

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
src/wasm/module-decoder.cc:85: uint32_t section_string_bytes;
On 2016/03/09 13:19:37, titzer wrote:
> section_string_size

Done.

https://codereview.chromium.org/1765843002/diff/120001/src/wasm/module-decode...
src/wasm/module-decoder.cc:510: if (WasmSection::getNameLength(i) ==
*string_bytes &&
On 2016/03/09 13:19:37, titzer wrote:
> Can you drop in a TODO here? It's a linear search...

Done.

[JF, 2016-03-10 02:12:12]

I think this is now good to go, trying out a few bots.

[binji, 2016-03-10 02:20:33]

binji@chromium.org changed reviewers:
+ binji@chromium.org

[binji, 2016-03-10 02:20:34]

didn't have time to review it all, but so far lgtm

https://codereview.chromium.org/1765843002/diff/230011/src/wasm/encoder.cc
File src/wasm/encoder.cc (right):

https://codereview.chromium.org/1765843002/diff/230011/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:56: const size_t padded_varint = 4;
should be 5 right?

https://codereview.chromium.org/1765843002/diff/230011/test/mjsunit/wasm/wasm...
File test/mjsunit/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/1765843002/diff/230011/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:138: emit_varint(tmp_bytes,
string.length);
could use emit_string here

https://codereview.chromium.org/1765843002/diff/230011/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:300: for (seg of wasm.data_segments) {
duped

[JF, 2016-03-10 03:20:08]

Ha ha! You fool! You fell victim to one of the classic blunders - The most
famous of which is "never get involved in a land war in Asia" - but only
slightly less well-known is this: "Never type 'lgtm' partway through doing a
code review"! Ha ha ha ha ha ha ha! Ha ha ha ha ha ha ha! Ha ha ha...

https://codereview.chromium.org/1765843002/diff/230011/src/wasm/encoder.cc
File src/wasm/encoder.cc (right):

https://codereview.chromium.org/1765843002/diff/230011/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:56: const size_t padded_varint = 4;
On 2016/03/10 02:20:34, binji wrote:
> should be 5 right?

Done.

https://codereview.chromium.org/1765843002/diff/230011/test/mjsunit/wasm/wasm...
File test/mjsunit/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/1765843002/diff/230011/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:138: emit_varint(tmp_bytes,
string.length);
On 2016/03/10 02:20:34, binji wrote:
> could use emit_string here

Ooh a new toy :-)

https://codereview.chromium.org/1765843002/diff/230011/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:300: for (seg of wasm.data_segments) {
On 2016/03/10 02:20:34, binji wrote:
> duped

Er wtf? I think it was a merge artifact, but it's surprising that tests passed
:-s

[JF, 2016-03-10 03:20:30]

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

[commit-bot: I haz the power, 2016-03-10 03:20:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765843002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765843002/300001

[commit-bot: I haz the power, 2016-03-10 03:31:25]

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

[commit-bot: I haz the power, 2016-03-10 03:31:26]

Dry run: Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[JF, 2016-03-10 04:41:17]

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

[commit-bot: I haz the power, 2016-03-10 04:41:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765843002/230012
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765843002/230012

[commit-bot: I haz the power, 2016-03-10 05:04:52]

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

[commit-bot: I haz the power, 2016-03-10 05:04:54]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-03-10 12:32:31]

On 2016/03/10 05:04:54, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

lgtm

[titzer, 2016-03-10 12:32:37]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-03-10 12:32:38]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org
Link to the patchset: https://codereview.chromium.org/1765843002/#ps230012
(title: "Silence compiler warning")

[commit-bot: I haz the power, 2016-03-10 12:32:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1765843002/230012
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1765843002/230012

[commit-bot: I haz the power, 2016-03-10 12:36:48]

Committed patchset #18 (id:230012)

[commit-bot: I haz the power, 2016-03-10 12:37:26]

Description was changed from

==========
wasm: use strings for section names

This will require an equivalent sexpr-wasm change.

See:
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#high-leve...

R=titzer@chromium.org, binji@chronium.org
==========

to

==========
wasm: use strings for section names

This will require an equivalent sexpr-wasm change.

See:
https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#high-leve...

R=titzer@chromium.org, binji@chronium.org

Committed: https://crrev.com/abbdca947f8b1f4967ff71700cca8cbc326df5cd
Cr-Commit-Position: refs/heads/master@{#34668}
==========

[commit-bot: I haz the power, 2016-03-10 12:37:27]

Patchset 18 (id:??) landed as
https://crrev.com/abbdca947f8b1f4967ff71700cca8cbc326df5cd
Cr-Commit-Position: refs/heads/master@{#34668}