[wasm] Master CL for Binary 0xC changes.

[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Committed: https://crrev.com/28392ab1968199b7d448c1868aebabdea99318d4
Cr-Original-Commit-Position: refs/heads/master@{#39678}
Cr-Commit-Position: refs/heads/master@{#39795}

[titzer, 2016-09-15 12:51:39]

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

[commit-bot: I haz the power, 2016-09-15 12:51:46]

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-09-15 12:53:33]

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

[commit-bot: I haz the power, 2016-09-15 12:53:34]

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/12822)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[titzer, 2016-09-15 14:16:33]

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

[commit-bot: I haz the power, 2016-09-15 14:16:37]

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-09-15 14:26:26]

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

[commit-bot: I haz the power, 2016-09-15 14:26:27]

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

[titzer, 2016-09-15 14:32:44]

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

[commit-bot: I haz the power, 2016-09-15 14:32:47]

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-09-15 14:40:37]

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

[commit-bot: I haz the power, 2016-09-15 14:40:38]

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

[titzer, 2016-09-15 14:50:17]

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

[commit-bot: I haz the power, 2016-09-15 14:50:21]

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-09-15 14:55:09]

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

[commit-bot: I haz the power, 2016-09-15 14:55:10]

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

[Mircea Trofin, 2016-09-15 15:12:21]

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

[Mircea Trofin, 2016-09-15 15:12:23]

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

https://codereview.chromium.org/2345593003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1261: // There is already an owner, clone everything.
See go/wasminstantiation. You need to GC if it's not null, then try again.

https://codereview.chromium.org/2345593003/diff/40001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1310:
//--------------------------------------------------------------------------
I'm worried about it all being in one place, and evolving and maintaining such a
large (textually) function.

[titzer, 2016-09-16 08:05:40]

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

[commit-bot: I haz the power, 2016-09-16 08:05:45]

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-09-16 08:07:33]

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

[commit-bot: I haz the power, 2016-09-16 08:07:34]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24233)

[titzer, 2016-09-16 09:35:43]

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

[commit-bot: I haz the power, 2016-09-16 09:35:48]

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-09-16 09:50:58]

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

[commit-bot: I haz the power, 2016-09-16 09:50:59]

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

[titzer, 2016-09-16 10:48:20]

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

[commit-bot: I haz the power, 2016-09-16 10:48:24]

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

[ahaas, 2016-09-16 11:21:31]

Some feedback for the wasm-compiler and the ast-decoder

https://codereview.chromium.org/2345593003/diff/10067/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2345593003/diff/10067/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2069: Node** rets = Buffer(sig->return_count());
Can you even return a trap value if return_count() == 0?

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:368: for (unsigned i = 0; i < c->merge.arity; i++) {
is merge.arity really of type unsigned and not of type uint32_t?

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:581: for (unsigned i = 0; i < c->merge.arity; i++) {
same here.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:821: BreakTo(&control_[control_.size() - operand.depth -
1]);
Could you extract a BreakTo which takes operand.depth as a parameter, so that
the {control_.size() - operand.depth - 1} calculation is hidden?

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1277: Push(sig->GetReturn(i), rets ? rets[i] : nullptr);
Why do you push nullptr here? Please add a comment which explains it. I do not
see where the null case is handled.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1363: int arity = static_cast<int>(c->merge.arity);
is stack_.size really of type int? I think arity should be of the same type as
stack_.size(), which I think should not be int.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1383: Value& val = stack_[stack_.size() - c->merge.arity
+ i];
I think it would be good to wrap this stack access

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1400: Value& val = stack_[stack_.size() - c->merge.arity
+ i];
same here.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:97: const byte* types;
What does {types} actually contain? I'm quite sure it does not contain types, so
could you give the variable a more descriptive name?

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:109: } else {
Could you add a comment about what is happening in this else block?

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:120: arity = count + 2;
please add a comment why arity = count + 2

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:135: bool decode_local_type(uint8_t val, LocalType*
result) {
Could you add a comment which describes the return value of this function? What
is left for the default case below?

[commit-bot: I haz the power, 2016-09-16 11:38:15]

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

[commit-bot: I haz the power, 2016-09-16 11:38:16]

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

[titzer, 2016-09-16 11:41:08]

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

[commit-bot: I haz the power, 2016-09-16 11:41:16]

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

[ahaas, 2016-09-16 11:42:18]

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

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:201: if (sig) functions_.back()->SetSignature(sig);
Could you add a comment about what it means if sig == nullptr?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:261: bool names = false;
I think it would be better if this variable were declared just before it is
used. Maybe it should also be called {has_names}.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:292: buffer.write_u32v(import.name_length);  // module name
length
Are these comments really consistent with the code?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:340: for (auto global : globals_) {
I think auto makes code harder to read. Would it be possible not to use auto?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:341:
buffer.write_u8(WasmOpcodes::LocalTypeCodeFor(std::get<0>(global)));
Define a constant instead of using 0 here.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:342: buffer.write_u8(std::get<2>(global));
same here

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:343: switch (std::get<0>(global)) {
same here.

[titzer, 2016-09-16 12:13:21]

https://codereview.chromium.org/2345593003/diff/10067/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2345593003/diff/10067/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2069: Node** rets = Buffer(sig->return_count());
On 2016/09/16 11:21:30, ahaas wrote:
> Can you even return a trap value if return_count() == 0?

Good point. No, not really. But it shouldn't matter because nothing will be
pushed onto the decoder stack for void calls.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:368: for (unsigned i = 0; i < c->merge.arity; i++) {
On 2016/09/16 11:21:30, ahaas wrote:
> is merge.arity really of type unsigned and not of type uint32_t?

Good question. The field is of type uint32_t, but if I use a uint32_t as the
counter variable here, then C++ will not use a native-sized integer for the
loop, which might be a tiny bit less efficient. The comparison should still be
efficient, though.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:581: for (unsigned i = 0; i < c->merge.arity; i++) {
On 2016/09/16 11:21:30, ahaas wrote:
> same here.

Acknowledged.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:821: BreakTo(&control_[control_.size() - operand.depth -
1]);
On 2016/09/16 11:21:30, ahaas wrote:
> Could you extract a BreakTo which takes operand.depth as a parameter, so that
> the {control_.size() - operand.depth - 1} calculation is hidden?

Good idea. In fact, all uses were of this form, so I made this method take the
depth.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1277: Push(sig->GetReturn(i), rets ? rets[i] : nullptr);
On 2016/09/16 11:21:30, ahaas wrote:
> Why do you push nullptr here? Please add a comment which explains it. I do not
> see where the null case is handled.

I added a comment. It's because the decoder can be used without a graph builder,
in which case it only does verification and doesn't create nodes.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1363: int arity = static_cast<int>(c->merge.arity);
On 2016/09/16 11:21:30, ahaas wrote:
> is stack_.size really of type int? I think arity should be of the same type as
> stack_.size(), which I think should not be int.

Yeah, we should probably just make that size_t, but I'd like to do that in a
future cleanup CL.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1383: Value& val = stack_[stack_.size() - c->merge.arity
+ i];
On 2016/09/16 11:21:30, ahaas wrote:
> I think it would be good to wrap this stack access

Not sure what you mean by that. This loop basically goes over the last
{merge.arity} entries in the stack, so it has to do an indexing trick.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1400: Value& val = stack_[stack_.size() - c->merge.arity
+ i];
On 2016/09/16 11:21:31, ahaas wrote:
> same here.

Acknowledged.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:97: const byte* types;
On 2016/09/16 11:21:31, ahaas wrote:
> What does {types} actually contain? I'm quite sure it does not contain types,
so
> could you give the variable a more descriptive name?

It points to the first byte where the encoded types are stored within this
bytecode.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:109: } else {
On 2016/09/16 11:21:31, ahaas wrote:
> Could you add a comment about what is happening in this else block?

Done.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:120: arity = count + 2;
On 2016/09/16 11:21:31, ahaas wrote:
> please add a comment why arity = count + 2

Done.

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:135: bool decode_local_type(uint8_t val, LocalType*
result) {
On 2016/09/16 11:21:31, ahaas wrote:
> Could you add a comment which describes the return value of this function?
What
> is left for the default case below?

Done.

[titzer, 2016-09-16 12:23:21]

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

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:201: if (sig) functions_.back()->SetSignature(sig);
On 2016/09/16 11:42:18, ahaas wrote:
> Could you add a comment about what it means if sig == nullptr?

Done.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:261: bool names = false;
On 2016/09/16 11:42:18, ahaas wrote:
> I think it would be better if this variable were declared just before it is
> used. Maybe it should also be called {has_names}.

Done.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:292: buffer.write_u32v(import.name_length);  // module name
length
On 2016/09/16 11:42:18, ahaas wrote:
> Are these comments really consistent with the code?

Done.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:340: for (auto global : globals_) {
On 2016/09/16 11:42:18, ahaas wrote:
> I think auto makes code harder to read. Would it be possible not to use auto?

All the loops here are doing that. It's a matter of how much context there is to
determine the type. In these cases, we are iterating over vectors that contain
functions, globals, etc.

Sadly in this case, it's a tuple. And C++ tuples are such a hassle...

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:341:
buffer.write_u8(WasmOpcodes::LocalTypeCodeFor(std::get<0>(global)));
On 2016/09/16 11:42:18, ahaas wrote:
> Define a constant instead of using 0 here.

Done.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:342: buffer.write_u8(std::get<2>(global));
On 2016/09/16 11:42:18, ahaas wrote:
> same here

Done.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:343: switch (std::get<0>(global)) {
On 2016/09/16 11:42:18, ahaas wrote:
> same here.

Done.

[ahaas, 2016-09-16 13:32:51]

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

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:340: for (auto global : globals_) {
On 2016/09/16 at 12:23:20, titzer wrote:
> On 2016/09/16 11:42:18, ahaas wrote:
> > I think auto makes code harder to read. Would it be possible not to use
auto?
> 
> All the loops here are doing that. It's a matter of how much context there is
to determine the type. In these cases, we are iterating over vectors that
contain functions, globals, etc.
> 
> Sadly in this case, it's a tuple. And C++ tuples are such a hassle...

Could you add a comment which says that global is a tuple?

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

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:35: return expr.val.global_index <
module->globals.size()
Is that the right error handling here for an index out of bounds?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:65: return section_code_ != kUnknownSectionCode &&
decoder_.more();
Is more() the same as has_next()? If yes, should it not be called has_next()?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:134: section_code_ =
static_cast<WasmSectionCode>(section_code);
This assignment will overwrite the assignment in line 132.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:458: uint32_t len = a.name_length;
why do you store a.name_length in a variable but not b.name_length? This
confused me.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:466: for (auto end = sorted_exports.end(); it != end;
last = &*it++) {
I think you should declare {last} in the for-initialization and {end} before the
for, not the other way around.

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:482: if (module->start_function_index >= 0) {
I think it's not even possible anymore to pass this condition. Change it to a
DCHECK?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:531: pc_ += size;
can there be an overflow problem here?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:707: // TODO(titzer): this only works without
overlapping initializations from
Can you add DCHECKs for this?

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/module-decode...
src/wasm/module-decoder.cc:828: if (flags & 1) {
What does flags & 1 mean?

[ahaas, 2016-09-16 13:48:19]

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2345593003/diff/150001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:740: auto pcdiff = static_cast<pcdiff_t>(target -
from_pc);
pcdiff_t is hardly any longer than auto.

[ahaas, 2016-09-19 08:43:05]

On 2016/09/16 at 13:48:19, ahaas wrote:
>
https://codereview.chromium.org/2345593003/diff/150001/src/wasm/wasm-interpre...
> File src/wasm/wasm-interpreter.cc (right):
> 
>
https://codereview.chromium.org/2345593003/diff/150001/src/wasm/wasm-interpre...
> src/wasm/wasm-interpreter.cc:740: auto pcdiff = static_cast<pcdiff_t>(target -
from_pc);
> pcdiff_t is hardly any longer than auto.

I think you didn't run tools/update-wasm-fuzzers.sh with your changes.

[ahaas, 2016-09-19 11:02:39]

https://codereview.chromium.org/2345593003/diff/150001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2345593003/diff/150001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:24: #define B1(a) WASM_BLOCK(a)
Is it worth it to have B*?

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

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1047: static_cast<int>(functions.size() +
num_exported_functions);
Why does this sum not include the number of imported functions and the start
function?

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

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:74: struct WasmInitExpr {
This looks like an WasmInitValue and not like a WasmInitExpr. Will it become a
expression eventually?

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:93: #define NO_INIT                 \
Wouldn't this definition fit better into wasm-macro-gen.h?

https://codereview.chromium.org/2345593003/diff/230001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:50: BUILD(r, WASM_I8(-99), WASM_DROP,
WASM_I8(kExpectedValue));
Wouldn't r.Call() return kExpectedValue even without the WASM_DROP?

https://codereview.chromium.org/2345593003/diff/230001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:2411: #if !V8_TARGET_ARCH_32_BIT ||
V8_TARGET_ARCH_X64
isn't V8_TARGET_ARCH_32_BIT always false if V8_TARGET_ARCH_X64 is true?

https://codereview.chromium.org/2345593003/diff/230001/test/fuzzer/wasm-names...
File test/fuzzer/wasm-names-section.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/fuzzer/wasm-names...
test/fuzzer/wasm-names-section.cc:8: return
fuzz_wasm_section(v8::internal::wasm::kNameSectionCode, data, size);
As far as I understood the changes in module-decoder.cc, the names section needs
a preceding functions section with the 0xc changes. Could you add a TODO that we
have to fix this because otherwise this fuzzer does not do anything?

https://codereview.chromium.org/2345593003/diff/230001/test/fuzzer/wasm-secti...
File test/fuzzer/wasm-section-fuzzers.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/fuzzer/wasm-secti...
test/fuzzer/wasm-section-fuzzers.cc:40: if (section == kNameSectionCode) {
I think it is better if we handle the names section in wasm-names-section.cc
completely. We have to do additional changes anyways because the names section
seems to depend on the functions section now.

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:64: builder.addFunction("pop", sig_i_i)
Please give this function a proper name, Popcnt does not do what you would
typically expect from a pop function.

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/inst...
File test/mjsunit/wasm/instantiate-module-basic.js (right):

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/inst...
test/mjsunit/wasm/instantiate-module-basic.js:188:
print("InstanceMemoryIsIsolated...");
I like these prints.

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/wasm...
File test/mjsunit/wasm/wasm-constants.js (right):

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-constants.js:24: var kWasmV0 = 12;  // 0xC
why do you not use 0xc directly as a  value?

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

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:268: switch (global_type) {
What about kGlobalIndex? Isn't that also a potential global_type?

https://codereview.chromium.org/2345593003/diff/230001/test/unittests/wasm/as...
File test/unittests/wasm/ast-decoder-unittest.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/unittests/wasm/as...
test/unittests/wasm/ast-decoder-unittest.cc:50: #define EXPECT_VERIFIES_C(sig,
x) \
What does the C stand for?

https://codereview.chromium.org/2345593003/diff/230001/test/unittests/wasm/as...
test/unittests/wasm/ast-decoder-unittest.cc:314: 
Add a test with an oob leb?

[titzer, 2016-09-19 11:09:50]

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

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1047: static_cast<int>(functions.size() +
num_exported_functions);
On 2016/09/19 11:02:38, ahaas wrote:
> Why does this sum not include the number of imported functions and the start
> function?

functions.size() includes the number of imported functions (there are entries in
the functions vector for imports.
I was previously also storing the start function at the end of the code table,
but I changed that to only use the start function index. If I missed something
there, it's a bug.

[ahaas, 2016-09-19 11:28:08]

https://codereview.chromium.org/2345593003/diff/230001/src/asmjs/asm-wasm-bui...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:715: auto indices = GetEntry(v);
Is it necessary to use auto here? (What's const to you is auto to me)

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

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1047: static_cast<int>(functions.size() +
num_exported_functions);
On 2016/09/19 at 11:09:50, titzer wrote:
> On 2016/09/19 11:02:38, ahaas wrote:
> > Why does this sum not include the number of imported functions and the start
> > function?
> 
> functions.size() includes the number of imported functions (there are entries
in the functions vector for imports.
> I was previously also storing the start function at the end of the code table,
but I changed that to only use the start function index. If I missed something
there, it's a bug.

Could you say in the comment that functions.size() contains the number of
imported functions?

[ahaas, 2016-09-19 11:36:02]

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1383: Value& val = stack_[stack_.size() - c->merge.arity
+ i];
On 2016/09/16 at 12:13:21, titzer wrote:
> On 2016/09/16 11:21:30, ahaas wrote:
> > I think it would be good to wrap this stack access
> 
> Not sure what you mean by that. This loop basically goes over the last
{merge.arity} entries in the stack, so it has to do an indexing trick.

I meant, could you create an inline function which takes stack_ and
(c->merge.arity - 1) as a parameter and returns the corresponding stack value?

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.h#...
src/wasm/ast-decoder.h:97: const byte* types;
On 2016/09/16 at 12:13:21, titzer wrote:
> On 2016/09/16 11:21:31, ahaas wrote:
> > What does {types} actually contain? I'm quite sure it does not contain
types, so
> > could you give the variable a more descriptive name?
> 
> It points to the first byte where the encoded types are stored within this
bytecode.

Could you add that as a comment, or call it start_of_types?

[titzer, 2016-09-19 11:36:50]

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

[commit-bot: I haz the power, 2016-09-19 11:37:01]

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

[titzer, 2016-09-19 11:37:03]

https://codereview.chromium.org/2345593003/diff/150001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2345593003/diff/150001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:24: #define B1(a) WASM_BLOCK(a)
On 2016/09/19 11:02:38, ahaas wrote:
> Is it worth it to have B*?

I've gone back and forth, and removed some uses, especially where
block types were necessary.
I think it makes sense to clean them all up in a later CL.

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

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:74: struct WasmInitExpr {
On 2016/09/19 11:02:38, ahaas wrote:
> This looks like an WasmInitValue and not like a WasmInitExpr. Will it become a
> expression eventually?

That's up for discussion in the design repo, but as things are now, it's
definitely set up for future evolution into more forms of expressions.

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:93: #define NO_INIT                 \
On 2016/09/19 11:02:38, ahaas wrote:
> Wouldn't this definition fit better into wasm-macro-gen.h?

It's actually used in the module decoder. wasm-macro-gen is mostly for
generating byte arrays of wasm code and modules.

https://codereview.chromium.org/2345593003/diff/230001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:50: BUILD(r, WASM_I8(-99), WASM_DROP,
WASM_I8(kExpectedValue));
On 2016/09/19 11:02:38, ahaas wrote:
> Wouldn't r.Call() return kExpectedValue even without the WASM_DROP?

Thanks for looking closely! The new semantics of function ends require that
falling through to an "end" bytecode must leave the same number and type of
values on the stack as the block's signature. The function block's signature is
the return type of the function.

https://codereview.chromium.org/2345593003/diff/230001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:2411: #if !V8_TARGET_ARCH_32_BIT ||
V8_TARGET_ARCH_X64
On 2016/09/19 11:02:38, ahaas wrote:
> isn't V8_TARGET_ARCH_32_BIT always false if V8_TARGET_ARCH_X64 is true?

Sigh, if only :-(
It's because of the defunct X32 port where X64 is true but 32_BIT is also true.

https://codereview.chromium.org/2345593003/diff/230001/test/fuzzer/wasm-names...
File test/fuzzer/wasm-names-section.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/fuzzer/wasm-names...
test/fuzzer/wasm-names-section.cc:8: return
fuzz_wasm_section(v8::internal::wasm::kNameSectionCode, data, size);
On 2016/09/19 11:02:38, ahaas wrote:
> As far as I understood the changes in module-decoder.cc, the names section
needs
> a preceding functions section with the 0xc changes. Could you add a TODO that
we
> have to fix this because otherwise this fuzzer does not do anything?

Done. Would you also be OK deferring the inlining of the fuzz_wasm_section
function into this one (as per a later comment) until that time?

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:64: builder.addFunction("pop", sig_i_i)
On 2016/09/19 11:02:38, ahaas wrote:
> Please give this function a proper name, Popcnt does not do what you would
> typically expect from a pop function.

Done.

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/wasm...
File test/mjsunit/wasm/wasm-constants.js (right):

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-constants.js:24: var kWasmV0 = 12;  // 0xC
On 2016/09/19 11:02:38, ahaas wrote:
> why do you not use 0xc directly as a  value?

Done.

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

https://codereview.chromium.org/2345593003/diff/230001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:268: switch (global_type) {
On 2016/09/19 11:02:38, ahaas wrote:
> What about kGlobalIndex? Isn't that also a potential global_type?

Ah, kGlobalIndex is a member of the WasmExternalKind enum (i.e. it is used in
import and export entries). JavaScript's weak typing doesn't make this obvious,
but it's clearer in wasm-module.h.

https://codereview.chromium.org/2345593003/diff/230001/test/unittests/wasm/as...
File test/unittests/wasm/ast-decoder-unittest.cc (right):

https://codereview.chromium.org/2345593003/diff/230001/test/unittests/wasm/as...
test/unittests/wasm/ast-decoder-unittest.cc:50: #define EXPECT_VERIFIES_C(sig,
x) \
On 2016/09/19 11:02:38, ahaas wrote:
> What does the C stand for?

_C = "with code", as opposed to within inline bytes, which is the common case.

I admit these macros are a little cryptic, but I think they improve on the
previous naming conventions.

https://codereview.chromium.org/2345593003/diff/230001/test/unittests/wasm/as...
test/unittests/wasm/ast-decoder-unittest.cc:314: 
On 2016/09/19 11:02:38, ahaas wrote:
> Add a test with an oob leb?

Did you mean an LEB that doesn't encode correctly into 5 bytes (i.e. stray upper
bits)? If so, done.

[ahaas, 2016-09-19 11:41:10]

lgtm after comments are addressed.

[Mircea Trofin, 2016-09-19 16:16:26]

some drive-by comments

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2318: FFIUseSignature* sig;
please initialize sig

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2324: for (size_t i = 0; i < args.size(); i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.h
File src/asmjs/asm-typer.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.h#...
src/asmjs/asm-typer.h:92: const ZoneVector<FFIUseSignature>* FFIUseSignatures()
{
could this be const ZoneVector<FFIUseSignature>&, since it's a const.

Also, why is FFIUseSignature stored by value and not as a pointer? Seems odd,
esp. when compared to forward_definitions_, which is as a pointer.

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:115: for (AsmType* arg : ffi.arg_types_)
b.AddParam(TypeFrom(arg));
Mind changing TypeFrom to taking a const as parameter, and then here const
AsmType*arg <etc>?

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:243: void DoBreakOrContinue(BreakableStatement*
target, bool is_continue) {
target could be const

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:245: for (int i =
static_cast<int>(breakable_blocks_.size()) - 1; i >= 0; i--) {
--i

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:324: for (int v = node->begin; v <= node->end;
v++) {
++v

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:359: for (int i = case_count - 1; i >= 0; i--) {
--i

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:934: MachineType mtype;
mind initializing mtype?

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/int64-low...
File src/compiler/int64-lowering.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/int64-low...
src/compiler/int64-lowering.cc:786: for (size_t i = 0; i <
descriptor->ReturnCount(); i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:205: return
jsgraph()->Float32Constant(bit_cast<float>(0xdeadbeef));
don't we have constants for these somewhere (like globals.h)?

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:995: Node* WasmGraphBuilder::Return(unsigned
count, Node** vals) {
uint32_t count?

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2070: for (size_t i = 0; i < sig->return_count();
i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:114: inline bool is_try() const { return kind ==
kControlTry; }
all the above 4 is_xyz are consts, not just is_try

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:362: for (unsigned i = 0; i < c->merge.arity; i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:764: stack_.push_back(c->merge.vals.array[i]);
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1157: for (unsigned i = 0; i < c->merge.arity; i++) {
++i

also, uint32_t, not unsigned

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1394: for (unsigned i = 0; i < c->merge.arity; i++) {
++i, also, uint32_t

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1411: for (unsigned i = 0; i < c->merge.arity; i++) {
++i, uint32_t

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1823: for (unsigned i = 0; i < operand.arity; i++) {
++i, uint32_t

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:102: LocalType type;
can type be initialized?

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:129: for (uint32_t i = 0; i < arity; i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:233: unsigned cur_index() { return index_; }
uint32_t cur_index() const

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:234: bool has_next() { return index_ <= table_count_; }
bool has_next() const

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:238: unsigned length = 0;
uint32_t length

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:246: unsigned length() {
uint32_t

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:250: const byte* pc() { return pc_; }
const byte* pc() const

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:35: size_t EmitSection(WasmSectionCode code, ZoneBuffer&
buffer) {
const ZoneBuffer&, or ZoneBuffer*

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:144: void WasmFunctionBuilder::WriteSignature(ZoneBuffer&
buffer) const {
const ZoneBuffer&, or ZoneBuffer*

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:148: void WasmFunctionBuilder::WriteExport(ZoneBuffer&
buffer) const {
const ZoneBuffer&, or ZoneBuffer*

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:179: void WasmDataSegmentEncoder::Write(ZoneBuffer& buffer)
const {
const ZoneBuffer&, or ZoneBuffer*

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:341: for (auto global : globals_) {
If we're not using global as an iterator, it'd be more readable to explicitly
type it here.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:380: for (auto function : functions_)
function->WriteExport(buffer);
same comment about auto

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:392: if (indirect_functions_.size() > 0) {
if !indirect_functions.empty()

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h#new...
src/wasm/encoder.h:136: uint32_t func_index() { return func_index_; }
const? also for exported() above.

worth checking signature(), too

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h#new...
src/wasm/encoder.h:250: ZoneVector<std::tuple<LocalType, bool, bool>> globals_;
For readability (like, what's this second and third bool?), would it be worth
typedef-ing this tuple, or creating a struct?

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:30: LocalType TypeOf(WasmModule* module,
WasmInitExpr& expr) {
const WasmModule* module, const WasmInitExpr& expr

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:239: static_cast<int>(pc_ - start_));
it'd be nice if all accessing happened with pc(), that way, it'd be easier to
find all places pc were read. 

same with mutating - if we had a set_pc().

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:359: for (uint32_t i = 0; ok() && i < table_count;
i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:379: for (uint32_t i = 0; ok() && i < memory_count;
i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:380: consume_resizable_limits("memory", "pages",
65536,
is 65536 a const somewhere? or std::numeric_limits<int16_t> + 1 or something...
or 1 << 16

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:505: for (uint32_t j = 0; ok() && j < num_elem; j++)
{
++j

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:566: for (uint32_t j = 0; j < local_names_count; j++)
{
++j

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:707: for (size_t i = 0; i < table.size; i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:711: for (WasmTableInit& init : module->table_inits)
{
const WasmTableInit&?

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:716: for (size_t i = 0; i < init.entries.size(); i++)
{
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:851: WasmInitExpr expr;
I'm assuming this is initialized here (I was trying to find WasmInitExpr's
declaration at the time I wrote this, and couldn't find it. Please disregard if
there's a default constructor).

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:1073: FunctionOffsets table;
I'm assuming FunctionOffsets has a default constructor.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:1081: for (uint32_t i = 0; i <
num_imported_functions; i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1047: virtual WasmVal GetReturnValue(int index) {
should this be typed size_t or uint32_t?

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1159: int LookupTarget(InterpreterCode* code, pc_t
pc) {
why not type it as size_t

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1174: while (blocks_.size() > 0 &&
blocks_.back().fp == frames_.size()) {
!blocks.empty()

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1215: for (size_t i = 0; i < arity; i++) {
++i

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1052: for (uint32_t i = 0; i < functions.size(); i++) {
++i

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1115: index++;
++index;

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1379: for (int index = 0; index <
num_imported_functions; index++) {
++index

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1485: for (int i = 0; i < code_table->length(); i++) {
++i

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:405: uint32_t GetNumImportedFunctions(Handle<JSObject>
wasm_object);
is this the instance object or the module object?

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-expr.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:111: //    print(asm_source);
remove commented line (I assume it's leftover from early debugging?)

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:125: //    print(asm_source);
please remove commented line

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/grow...
File test/mjsunit/wasm/grow-memory.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:21: .addBody([kExprGetLocal, 0, kExprGetLocal,
1, kExprI32StoreMem, 0, 0, kExprGetLocal, 1])
80 char limit (below, too)

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:80: })();
80 char

[titzer, 2016-09-21 08:58:45]

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/10067/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:1383: Value& val = stack_[stack_.size() - c->merge.arity
+ i];
On 2016/09/19 11:36:02, ahaas wrote:
> On 2016/09/16 at 12:13:21, titzer wrote:
> > On 2016/09/16 11:21:30, ahaas wrote:
> > > I think it would be good to wrap this stack access
> > 
> > Not sure what you mean by that. This loop basically goes over the last
> {merge.arity} entries in the stack, so it has to do an indexing trick.
> 
> I meant, could you create an inline function which takes stack_ and
> (c->merge.arity - 1) as a parameter and returns the corresponding stack value?

Ah, ok. Done.

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

https://codereview.chromium.org/2345593003/diff/230001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1047: static_cast<int>(functions.size() +
num_exported_functions);
On 2016/09/19 11:28:08, ahaas wrote:
> On 2016/09/19 at 11:09:50, titzer wrote:
> > On 2016/09/19 11:02:38, ahaas wrote:
> > > Why does this sum not include the number of imported functions and the
start
> > > function?
> > 
> > functions.size() includes the number of imported functions (there are
entries
> in the functions vector for imports.
> > I was previously also storing the start function at the end of the code
table,
> but I changed that to only use the start function index. If I missed something
> there, it's a bug.
> 
> Could you say in the comment that functions.size() contains the number of
> imported functions?

Done.

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2318: FFIUseSignature* sig;
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> please initialize sig

Ah, I just moved it down a line, now. Done.

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2324: for (size_t i = 0; i < args.size(); i++) {
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> ++i

Google style guide says
(https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement)
that pre/post increment doesn't matter, so I'll just leave it.

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:115: for (AsmType* arg : ffi.arg_types_)
b.AddParam(TypeFrom(arg));
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> Mind changing TypeFrom to taking a const as parameter, and then here const
> AsmType*arg <etc>?

I actually originally did that, but then the const starts spreading; e.g. into
the AsmType class, etc.

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:243: void DoBreakOrContinue(BreakableStatement*
target, bool is_continue) {
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> target could be const

The AST is essentially const file-wide, so I think we shouldn't waste time on
adding const everywhere here. Besides, if we do, we should add it to the visit
methods, but we can't because they're virtual.

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:934: MachineType mtype;
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> mind initializing mtype?

Done.

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:205: return
jsgraph()->Float32Constant(bit_cast<float>(0xdeadbeef));
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> don't we have constants for these somewhere (like globals.h)?

No, not yet. Can clean them up later. This CL is just extracting this code out
into a helper function.

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:995: Node* WasmGraphBuilder::Return(unsigned
count, Node** vals) {
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> uint32_t count?

Generally I've used "unsigned" where the quantity is not constrained by WASM
binary limitations (but only limitations of underlying C++, like running out of
memory for std::vector, etc). It's not a hard convention, but if we're going to
do something about it, I'd prefer to do it in a dedicated CL instead of hiding
it in this 6000+ line CL.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:114: inline bool is_try() const { return kind ==
kControlTry; }
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> all the above 4 is_xyz are consts, not just is_try

Done.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1157: for (unsigned i = 0; i < c->merge.arity; i++) {
On 2016/09/19 16:16:24, Mircea Trofin wrote:
> ++i
> 
> also, uint32_t, not unsigned

Same as before reason. There is nothing in WASM that inherently limits the merge
arity to 32 bits.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:102: LocalType type;
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> can type be initialized?

Done.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:233: unsigned cur_index() { return index_; }
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> uint32_t cur_index() const

Same reason as above. There's really no reason to limit the branch table
iterator to 32 bits.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:250: const byte* pc() { return pc_; }
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> const byte* pc() const

I'm not going to bother with const on this class since the main use is to
iterate over the branch targets, which is an inherently stateful operation.

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:35: size_t EmitSection(WasmSectionCode code, ZoneBuffer&
buffer) {
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> const ZoneBuffer&, or ZoneBuffer*

You're right that this should be a pointer instead of a reference, since the
reference cannot be made const.

I think it makes sense to do that cleanup in a specific CL for a single purpose.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:380: for (auto function : functions_)
function->WriteExport(buffer);
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> same comment about auto

Should be clear from context, IMHO. Iterating over a vector of functions
produces one function at a time, and we call the WriteExport() method fo r each
one. Would {WasmFunctionBuilder* function : functions_} significantly increase
the readability of this code? IMO no.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:392: if (indirect_functions_.size() > 0) {
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> if !indirect_functions.empty()

We should probably fix all the others above. No need to do that now.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h#new...
src/wasm/encoder.h:136: uint32_t func_index() { return func_index_; }
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> const? also for exported() above.
> 
> worth checking signature(), too

I don't think const annotations on builder classes make sense

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:239: static_cast<int>(pc_ - start_));
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> it'd be nice if all accessing happened with pc(), that way, it'd be easier to
> find all places pc were read. 
> 
> same with mutating - if we had a set_pc().

That's orthogonal to this CL and can be fixed later.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:380: consume_resizable_limits("memory", "pages",
65536,
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> is 65536 a const somewhere? or std::numeric_limits<int16_t> + 1 or
something...
> or 1 << 16

It's the maximum number of legal pages. I've added a constant.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:851: WasmInitExpr expr;
On 2016/09/19 16:16:26, Mircea Trofin wrote:
> I'm assuming this is initialized here (I was trying to find WasmInitExpr's
> declaration at the time I wrote this, and couldn't find it. Please disregard
if
> there's a default constructor).

No, it's initialized below in all the cases.
If I were to initialize it, that would mask bugs that would be caught by ASAN
(i.e. use of uninitialized value).

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/module-decode...
src/wasm/module-decoder.cc:1073: FunctionOffsets table;
On 2016/09/19 16:16:25, Mircea Trofin wrote:
> I'm assuming FunctionOffsets has a default constructor.

It's a typedef for std::vector, so yes.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1159: int LookupTarget(InterpreterCode* code, pc_t
pc) {
On 2016/09/19 16:16:26, Mircea Trofin wrote:
> why not type it as size_t

Because the return value (offset) can be negative.
Why int? Because nothing inherently limits it to int32_t.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-interpre...
src/wasm/wasm-interpreter.cc:1174: while (blocks_.size() > 0 &&
blocks_.back().fp == frames_.size()) {
On 2016/09/19 16:16:26, Mircea Trofin wrote:
> !blocks.empty()

Done.

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:405: uint32_t GetNumImportedFunctions(Handle<JSObject>
wasm_object);
On 2016/09/19 16:16:26, Mircea Trofin wrote:
> is this the instance object or the module object?

It's the instance. But I realize now the implementation can be different, as it
can get to the compiled module and get the number of imported functions from
there.

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-expr.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:111: //    print(asm_source);
On 2016/09/19 16:16:26, Mircea Trofin wrote:
> remove commented line (I assume it's leftover from early debugging?)

I'd prefer to keep it, since once in a while I add patterns and want to see the
output from test, which is easier to do by just uncommenting a line.

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:80: })();
On 2016/09/19 16:16:26, Mircea Trofin wrote:
> 80 char

There is actually a newline there.

[titzer, 2016-09-21 09:04:11]

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

[commit-bot: I haz the power, 2016-09-21 09:04:23]

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-09-21 09:59:19]

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

[commit-bot: I haz the power, 2016-09-21 09:59:20]

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

[titzer, 2016-09-21 10:28:38]

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

[commit-bot: I haz the power, 2016-09-21 10:28:45]

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-09-21 10:58:52]

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

[commit-bot: I haz the power, 2016-09-21 10:58:53]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-09-21 16:38:23]

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2324: for (size_t i = 0; i < args.size(); i++) {
On 2016/09/21 08:58:44, titzer wrote:
> On 2016/09/19 16:16:24, Mircea Trofin wrote:
> > ++i
> 
> Google style guide says
>
(https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement)
> that pre/post increment doesn't matter, so I'll just leave it.

OK, but now we have it sometimes pre and sometimes post, across the codebase.
It's a bit unfortunate.

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:995: Node* WasmGraphBuilder::Return(unsigned
count, Node** vals) {
On 2016/09/21 08:58:44, titzer wrote:
> On 2016/09/19 16:16:24, Mircea Trofin wrote:
> > uint32_t count?
> 
> Generally I've used "unsigned" where the quantity is not constrained by WASM
> binary limitations (but only limitations of underlying C++, like running out
of
> memory for std::vector, etc). It's not a hard convention, but if we're going
to
> do something about it, I'd prefer to do it in a dedicated CL instead of hiding
> it in this 6000+ line CL.

I may be mis/over-reading the style guide, I thought it recommends using the
stdint.h - defined types instead of "unsigned" ?

https://google.github.io/styleguide/cppguide.html#Integer_Types

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:250: const byte* pc() { return pc_; }
On 2016/09/21 08:58:44, titzer wrote:
> On 2016/09/19 16:16:25, Mircea Trofin wrote:
> > const byte* pc() const
> 
> I'm not going to bother with const on this class since the main use is to
> iterate over the branch targets, which is an inherently stateful operation.

There is readability and maintainability value in const-ing APIs: I don't have
to go to definition to understand what mutates. Understanding what doesn't
mutate keeps cognitive load lower than otherwise.

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

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.cc#ne...
src/wasm/encoder.cc:380: for (auto function : functions_)
function->WriteExport(buffer);
On 2016/09/21 08:58:44, titzer wrote:
> On 2016/09/19 16:16:25, Mircea Trofin wrote:
> > same comment about auto
> 
> Should be clear from context, IMHO. Iterating over a vector of functions
> produces one function at a time, and we call the WriteExport() method fo r
each
> one. Would {WasmFunctionBuilder* function : functions_} significantly increase
> the readability of this code? IMO no.

I don't know from local context what type functions_ has. Adding the type
annotation on function is more reader-friendly.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h#new...
src/wasm/encoder.h:136: uint32_t func_index() { return func_index_; }
On 2016/09/21 08:58:44, titzer wrote:
> On 2016/09/19 16:16:25, Mircea Trofin wrote:
> > const? also for exported() above.
> > 
> > worth checking signature(), too
> 
> I don't think const annotations on builder classes make sense

See my previous point about the value of const for readability and
maintainability.

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-expr.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:111: //    print(asm_source);
On 2016/09/21 08:58:45, titzer wrote:
> On 2016/09/19 16:16:26, Mircea Trofin wrote:
> > remove commented line (I assume it's leftover from early debugging?)
> 
> I'd prefer to keep it, since once in a while I add patterns and want to see
the
> output from test, which is easier to do by just uncommenting a line.

This seems like something more tests may benefit from. Perhaps we can add in a
separate CL a flag-enabled API to mjsunit.js, something like log(blah), and then
you just need to pass that flag in local explorations. We then avoid having
commented code, and the use case is easier (you just pass a flag rather than
going around commenting/uncommenting).

If the above seems reasonable, mind adding a TODO here, so we don't lose track
of it?

[titzer, 2016-09-21 18:49:05]

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

[commit-bot: I haz the power, 2016-09-21 18:49:13]

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

[titzer, 2016-09-21 18:49:28]

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/270001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2324: for (size_t i = 0; i < args.size(); i++) {
On 2016/09/21 16:38:23, Mircea Trofin wrote:
> On 2016/09/21 08:58:44, titzer wrote:
> > On 2016/09/19 16:16:24, Mircea Trofin wrote:
> > > ++i
> > 
> > Google style guide says
> >
>
(https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement)
> > that pre/post increment doesn't matter, so I'll just leave it.
> 
> OK, but now we have it sometimes pre and sometimes post, across the codebase.
> It's a bit unfortunate.

I've corrected some uses so that the changes in asmjs/* are at least
file-consistent, but if we want to change everything at once, let's do that in a
separate CL.

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:250: const byte* pc() { return pc_; }
On 2016/09/21 16:38:23, Mircea Trofin wrote:
> On 2016/09/21 08:58:44, titzer wrote:
> > On 2016/09/19 16:16:25, Mircea Trofin wrote:
> > > const byte* pc() const
> > 
> > I'm not going to bother with const on this class since the main use is to
> > iterate over the branch targets, which is an inherently stateful operation.
> 
> There is readability and maintainability value in const-ing APIs: I don't have
> to go to definition to understand what mutates. Understanding what doesn't
> mutate keeps cognitive load lower than otherwise.

The style guide recommends using const where it makes sense, and I don't think
it makes sense here (in an iterator).

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/270001/src/wasm/encoder.h#new...
src/wasm/encoder.h:136: uint32_t func_index() { return func_index_; }
On 2016/09/21 16:38:23, Mircea Trofin wrote:
> On 2016/09/21 08:58:44, titzer wrote:
> > On 2016/09/19 16:16:25, Mircea Trofin wrote:
> > > const? also for exported() above.
> > > 
> > > worth checking signature(), too
> > 
> > I don't think const annotations on builder classes make sense
> 
> See my previous point about the value of const for readability and
> maintainability.

I prefer a very light and judicial application of const. In my experience code
laced with too many const annotations is harder to read and frustrating to
modify.

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-expr.js (right):

https://codereview.chromium.org/2345593003/diff/270001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:111: //    print(asm_source);
On 2016/09/21 16:38:23, Mircea Trofin wrote:
> On 2016/09/21 08:58:45, titzer wrote:
> > On 2016/09/19 16:16:26, Mircea Trofin wrote:
> > > remove commented line (I assume it's leftover from early debugging?)
> > 
> > I'd prefer to keep it, since once in a while I add patterns and want to see
> the
> > output from test, which is easier to do by just uncommenting a line.
> 
> This seems like something more tests may benefit from. Perhaps we can add in a
> separate CL a flag-enabled API to mjsunit.js, something like log(blah), and
then
> you just need to pass that flag in local explorations. We then avoid having
> commented code, and the use case is easier (you just pass a flag rather than
> going around commenting/uncommenting).
> 
> If the above seems reasonable, mind adding a TODO here, so we don't lose track
> of it?

That's not a bad idea, since wasm-module-builder.js already does take a debug
parameter that causes it to print verbose output. I'll add a TODO

[commit-bot: I haz the power, 2016-09-21 18:51:41]

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

[commit-bot: I haz the power, 2016-09-21 18:51:43]

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

[titzer, 2016-09-21 18:59:39]

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

[commit-bot: I haz the power, 2016-09-21 18:59:52]

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-09-21 19:06:08]

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

[commit-bot: I haz the power, 2016-09-21 19:06:10]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/13191)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13161)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9354)

[titzer, 2016-09-21 19:10:25]

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

[commit-bot: I haz the power, 2016-09-21 19:10:38]

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-09-21 19:52:20]

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

[commit-bot: I haz the power, 2016-09-21 19:52:22]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-09-22 14:31:19]

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

[commit-bot: I haz the power, 2016-09-22 14:31:30]

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-09-22 14:33:47]

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

[commit-bot: I haz the power, 2016-09-22 14:33:49]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/13269)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24744)

[titzer, 2016-09-22 14:49:28]

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

[commit-bot: I haz the power, 2016-09-22 14:49: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, 2016-09-22 15:33:16]

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

[commit-bot: I haz the power, 2016-09-22 15:33:18]

Dry run: This issue passed the CQ dry run.

[bradnelson, 2016-09-23 11:36:02]

This change is WAY too big, but I expect you know that.
This is where a branch to keep reviews tractable would be good, but our infra
can't really support it (yet).

LGTM, or as close as we'll get.
Try to land it today.

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2317: // all uses up-front.
This will break my unlanded change to do validation + conversion without keeping
the whole ast in memory. :-(

Had feeling mixing the imports and others would be a pain (requires knowing how
big something is up front).

Options include:
* Fixup each function after generation once we know how many imports.
  - touches everything a second time
  - indices are variant length so could grow (would have to emit fixed length
ones)
* Presuppose all have no more than N arguments and guess signature at
declaration time
  - could be wrong, then what

It seems this would also be a huge hassle for anyone else doing a code
generator. It forces things to be two pass, unless you know all imports and
signatures up front (or are willing to re-write indices for calls at the end).
Did we want to revisit with partners?

As this change is so large it either needs to land now or never, I'd rather sort
it out myself than see this not go in very soon.

I'll probably yank this out and change the encoder to do fixed sized indices and
fixups.

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-wasm-bui...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-wasm-bui...
src/asmjs/asm-wasm-builder.cc:35: enum ValueFate { kDrop, kLeaveOnStack };
Delightful name :-)

https://codereview.chromium.org/2345593003/diff/470001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2703: Node* WasmGraphBuilder::CurrentMemory() {
CurrentMemoryPages ?

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:730: if (c->is_if()) {
} else if ?

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:132: decode_local_type(val, &type);
This function has a weird calling convention.
Might be cleaner if decode_local_type just had unreachable for the one case it
doesn't grok and returns a LocalType and then all the callers check for
kMultivalBlock separately before calling it?

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:141: // byte was invalid.
Rather invalid or kMultivalBlock ?

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/encoder.h#new...
src/wasm/encoder.h:224: WasmFunctionBuilder* AddFunction(FunctionSig* sig =
nullptr);
So I'll need to add a list of fixups per callsite to this. So we can patch them
once indices are known.

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

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:490: i::Handle<i::JSFunction> memory_cons(
Unfortunate abreviation in a context with s-exprs

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

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1316: // TODO(titzer): handle imported memory properly.
I don't grok this comment?

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

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:36: kTypeSectionCode = 1,      // Function signature
declarations
Yeah! bye strings...

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-opcodes....
src/wasm/wasm-opcodes.h:26: static const uint8_t kMultivalBlock = 0x41;
That seems fairly arbitrary :-)

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-relocation.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-relocation.cc:50: CHECK_LE(address_index, 4U);   
                                         \
Why isn't this still an eq check?
Should have a _LE check in the loop though.
Same below.

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:2386: WasmRunner<T> r2(&module, machine_type,
machine_type, machine_type,
I take it the tester always has multi-return on?

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-expr.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:8: //selectedTest = 16;
Leftover?

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:15: "if (E) =",
Indented oddly.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:126: //    print(asm_source);
leftover

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:132: 
Drop line

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:149: DoTheTests("(i32 | 0)", "i32 = 0",
"void_func()");
Good to have more coverage.
Interested what you hit from these?

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm.js:1026: print("getVal");
Did you still want these two?

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:51: 
Little odd to redef the module like this?
Maybe wrap each in a scope? (Or give some kind of name to each part)

https://codereview.chromium.org/2345593003/diff/470001/test/unittests/wasm/co...
File test/unittests/wasm/control-transfer-unittest.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/unittests/wasm/co...
test/unittests/wasm/control-transfer-unittest.cc:179: 0,              // @1 +1
I'm unclear if you're counting stack depth or what with these +1's now?
Counting bytes made sense when you didn't label each position (though that seems
clearer now.

[bradnelson, 2016-09-23 11:46:55]

I suppose we could at least avoid touching every callsite by introducing a thunk
function in between with the same signature, and then fixing just each of those
up.
TF won't inline those at the moment (correct?), but the ffi boundary is
expensive anyway.

[titzer, 2016-09-23 12:06:28]

On 2016/09/23 11:46:55, bradnelson wrote:
> I suppose we could at least avoid touching every callsite by introducing a
thunk
> function in between with the same signature, and then fixing just each of
those
> up.
> TF won't inline those at the moment (correct?), but the ffi boundary is
> expensive anyway.

See previous comment. AFAIK, it's legal to call the same foreign function with
different signatures. Each new signature introduces a new imports, shifting all
function indices down :(

We wouldn't have this problem if imports were at the end of the function space,
or if imports were mixed in (as they were just prior to 0xA).

Probably too late to change the import/function space at this point :(

[titzer, 2016-09-23 12:07:09]

On 2016/09/23 12:06:28, titzer wrote:
> On 2016/09/23 11:46:55, bradnelson wrote:
> > I suppose we could at least avoid touching every callsite by introducing a
> thunk
> > function in between with the same signature, and then fixing just each of
> those
> > up.
> > TF won't inline those at the moment (correct?), but the ffi boundary is
> > expensive anyway.
> 

(previous comment to come when I've sent it!)

> See previous comment. AFAIK, it's legal to call the same foreign function with
> different signatures. Each new signature introduces a new imports, shifting
all
> function indices down :(
> 
> We wouldn't have this problem if imports were at the end of the function
space,
> or if imports were mixed in (as they were just prior to 0xA).
> 
> Probably too late to change the import/function space at this point :(

[titzer, 2016-09-23 12:07:31]

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2317: // all uses up-front.
On 2016/09/23 11:36:00, bradnelson wrote:
> This will break my unlanded change to do validation + conversion without
keeping
> the whole ast in memory. :-(
> 
> Had feeling mixing the imports and others would be a pain (requires knowing
how
> big something is up front).
> 
> Options include:
> * Fixup each function after generation once we know how many imports.
>   - touches everything a second time
>   - indices are variant length so could grow (would have to emit fixed length
> ones)
> * Presuppose all have no more than N arguments and guess signature at
> declaration time
>   - could be wrong, then what
> 
> It seems this would also be a huge hassle for anyone else doing a code
> generator. It forces things to be two pass, unless you know all imports and
> signatures up front (or are willing to re-write indices for calls at the end).
> Did we want to revisit with partners?
> 
> As this change is so large it either needs to land now or never, I'd rather
sort
> it out myself than see this not go in very soon.
> 
> I'll probably yank this out and change the encoder to do fixed sized indices
and
> fixups.

Yeah, I know. This was the most annoyingly intrusive part of this change.

I also considered the patching approach, and agree that that is probably what we
should do in the future. I did it this way because that was the shorter path to
get it to work. We can do the patching in a followup.

Also note that a foreign function can be called with multiple different
signatures, so I am not sure that we can just assume a single one up front.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:730: if (c->is_if()) {
On 2016/09/23 11:36:00, bradnelson wrote:
> } else if ?

The previous if (c->is_loop()) breaks out of the switch case.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:132: decode_local_type(val, &type);
On 2016/09/23 11:36:00, bradnelson wrote:
> This function has a weird calling convention.
> Might be cleaner if decode_local_type just had unreachable for the one case it
> doesn't grok and returns a LocalType and then all the callers check for
> kMultivalBlock separately before calling it?

Yeah, it is weird. The problem is that it can't have UNREACHABLE, since it
should fail with some kind of decoder error on invalid user input, instead of
crashing.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:141: // byte was invalid.
On 2016/09/23 11:36:01, bradnelson wrote:
> Rather invalid or kMultivalBlock ?

Done.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/encoder.h#new...
src/wasm/encoder.h:224: WasmFunctionBuilder* AddFunction(FunctionSig* sig =
nullptr);
On 2016/09/23 11:36:01, bradnelson wrote:
> So I'll need to add a list of fixups per callsite to this. So we can patch
them
> once indices are known.

Do you mean a list of fixups per imported function?

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

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-js.cc#ne...
src/wasm/wasm-js.cc:490: i::Handle<i::JSFunction> memory_cons(
On 2016/09/23 11:36:01, bradnelson wrote:
> Unfortunate abreviation in a context with s-exprs

Changed to _ctor.

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

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:36: kTypeSectionCode = 1,      // Function signature
declarations
On 2016/09/23 11:36:01, bradnelson wrote:
> Yeah! bye strings...

Acknowledged.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-opcodes.h
File src/wasm/wasm-opcodes.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/wasm-opcodes....
src/wasm/wasm-opcodes.h:26: static const uint8_t kMultivalBlock = 0x41;
On 2016/09/23 11:36:01, bradnelson wrote:
> That seems fairly arbitrary :-)

Agree. It's a prototype, so we don't have to standardize it quite yet :-)

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-relocation.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-relocation.cc:50: CHECK_LE(address_index, 4U);   
                                         \
On 2016/09/23 11:36:01, bradnelson wrote:
> Why isn't this still an eq check?
> Should have a _LE check in the loop though.
> Same below.

Because for 64-bit operations on 32-bit platforms, it becomes two accesses
through int64-lowering.

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm.cc:2386: WasmRunner<T> r2(&module, machine_type,
machine_type, machine_type,
On 2016/09/23 11:36:01, bradnelson wrote:
> I take it the tester always has multi-return on?

I should set FLAG_wasm_mv_prototype = true;

Done.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-expr.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:8: //selectedTest = 16;
On 2016/09/23 11:36:01, bradnelson wrote:
> Leftover?

Left it there intentionally so that you can select a single test that's failing
easily.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:15: "if (E) =",
On 2016/09/23 11:36:01, bradnelson wrote:
> Indented oddly.

Done.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:126: //    print(asm_source);
On 2016/09/23 11:36:01, bradnelson wrote:
> leftover

Mircea asked about the same thing. I left it for easier debugging of this test
when it fails.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:132: 
On 2016/09/23 11:36:01, bradnelson wrote:
> Drop line

Done.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-expr.js:149: DoTheTests("(i32 | 0)", "i32 = 0",
"void_func()");
On 2016/09/23 11:36:01, bradnelson wrote:
> Good to have more coverage.
> Interested what you hit from these?

I hit a few cases of missing or extra kDrop instructions, which only showed up
deep in benchmarks, so I added more extensive tests here.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm.js:1026: print("getVal");
On 2016/09/23 11:36:01, bradnelson wrote:
> Did you still want these two?

Done.

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:51: 
On 2016/09/23 11:36:01, bradnelson wrote:
> Little odd to redef the module like this?
> Maybe wrap each in a scope? (Or give some kind of name to each part)

Meh, scopes won't help in JS. I could wrap in an anonymous function?

https://codereview.chromium.org/2345593003/diff/470001/test/unittests/wasm/co...
File test/unittests/wasm/control-transfer-unittest.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/unittests/wasm/co...
test/unittests/wasm/control-transfer-unittest.cc:179: 0,              // @1 +1
On 2016/09/23 11:36:01, bradnelson wrote:
> I'm unclear if you're counting stack depth or what with these +1's now?
> Counting bytes made sense when you didn't label each position (though that
seems
> clearer now.

They made more sense when these tests had 4 byte quantities inline, but now they
don't. Removed.

[titzer, 2016-09-23 12:09:00]

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

[commit-bot: I haz the power, 2016-09-23 12:09:18]

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-09-23 14:15:59]

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

[commit-bot: I haz the power, 2016-09-23 14:16:02]

Dry run: This issue passed the CQ dry run.

[bradn, 2016-09-23 15:18:40]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2016-09-23 15:18:44]

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:2317: // all uses up-front.
On 2016/09/23 12:07:30, titzer wrote:
> On 2016/09/23 11:36:00, bradnelson wrote:
> > This will break my unlanded change to do validation + conversion without
> keeping
> > the whole ast in memory. :-(
> > 
> > Had feeling mixing the imports and others would be a pain (requires knowing
> how
> > big something is up front).
> > 
> > Options include:
> > * Fixup each function after generation once we know how many imports.
> >   - touches everything a second time
> >   - indices are variant length so could grow (would have to emit fixed
length
> > ones)
> > * Presuppose all have no more than N arguments and guess signature at
> > declaration time
> >   - could be wrong, then what
> > 
> > It seems this would also be a huge hassle for anyone else doing a code
> > generator. It forces things to be two pass, unless you know all imports and
> > signatures up front (or are willing to re-write indices for calls at the
end).
> > Did we want to revisit with partners?
> > 
> > As this change is so large it either needs to land now or never, I'd rather
> sort
> > it out myself than see this not go in very soon.
> > 
> > I'll probably yank this out and change the encoder to do fixed sized indices
> and
> > fixups.
> 
> Yeah, I know. This was the most annoyingly intrusive part of this change.
> 
> I also considered the patching approach, and agree that that is probably what
we
> should do in the future. I did it this way because that was the shorter path
to
> get it to work. We can do the patching in a followup.
> 
> Also note that a foreign function can be called with multiple different
> signatures, so I am not sure that we can just assume a single one up front.

Yeah, only thought it work cause 4am.
Though I suppose we could use the tables plumbing to punch them in later. Yeck.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:730: if (c->is_if()) {
On 2016/09/23 12:07:30, titzer wrote:
> On 2016/09/23 11:36:00, bradnelson wrote:
> > } else if ?
> 
> The previous if (c->is_loop()) breaks out of the switch case.

Didn't mean drop the break.
But, it was an option suggestion.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h
File src/wasm/ast-decoder.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/ast-decoder.h...
src/wasm/ast-decoder.h:132: decode_local_type(val, &type);
On 2016/09/23 12:07:30, titzer wrote:
> On 2016/09/23 11:36:00, bradnelson wrote:
> > This function has a weird calling convention.
> > Might be cleaner if decode_local_type just had unreachable for the one case
it
> > doesn't grok and returns a LocalType and then all the callers check for
> > kMultivalBlock separately before calling it?
> 
> Yeah, it is weird. The problem is that it can't have UNREACHABLE, since it
> should fail with some kind of decoder error on invalid user input, instead of
> crashing.

Yeah true.
Many to few is hard.

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/encoder.h
File src/wasm/encoder.h (right):

https://codereview.chromium.org/2345593003/diff/470001/src/wasm/encoder.h#new...
src/wasm/encoder.h:224: WasmFunctionBuilder* AddFunction(FunctionSig* sig =
nullptr);
On 2016/09/23 12:07:30, titzer wrote:
> On 2016/09/23 11:36:01, bradnelson wrote:
> > So I'll need to add a list of fixups per callsite to this. So we can patch
> them
> > once indices are known.
> 
> Do you mean a list of fixups per imported function?

Oddly phrased.
I mean to say each callsite in every function will need to be changed.
Each list of fixups probably belongs on the FunctionBuilder.
So all the pages get touched. But then they weren't being built in place due to
other constraints of the format, so no huge loss.

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
File test/cctest/wasm/test-run-wasm-relocation.cc (right):

https://codereview.chromium.org/2345593003/diff/470001/test/cctest/wasm/test-...
test/cctest/wasm/test-run-wasm-relocation.cc:50: CHECK_LE(address_index, 4U);   
                                         \
On 2016/09/23 12:07:30, titzer wrote:
> On 2016/09/23 11:36:01, bradnelson wrote:
> > Why isn't this still an eq check?
> > Should have a _LE check in the loop though.
> > Same below.
> 
> Because for 64-bit operations on 32-bit platforms, it becomes two accesses
> through int64-lowering.

Ah. Might still be good to assert it in the loop (buffer overrun otherwise).
Could also still do the hard bound based on target.
Seems a weak test without it (it's mainly counting there were the expected
number of rewrites).

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/indi...
File test/mjsunit/wasm/indirect-calls.js (right):

https://codereview.chromium.org/2345593003/diff/470001/test/mjsunit/wasm/indi...
test/mjsunit/wasm/indirect-calls.js:51: 
On 2016/09/23 12:07:31, titzer wrote:
> On 2016/09/23 11:36:01, bradnelson wrote:
> > Little odd to redef the module like this?
> > Maybe wrap each in a scope? (Or give some kind of name to each part)
> 
> Meh, scopes won't help in JS. I could wrap in an anonymous function?

I meant a function to get scope (sorry 4 am), but mainly some comment / name
about why the two modules.

[titzer, 2016-09-23 15:53:27]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-09-23 15:53:28]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2345593003/#ps490001
(title: "Address review comments")

[commit-bot: I haz the power, 2016-09-23 15:53:37]

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-09-23 15:56:53]

Committed patchset #26 (id:490001)

[commit-bot: I haz the power, 2016-09-23 15:57:03]

Description was changed from

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

to

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}
==========

[commit-bot: I haz the power, 2016-09-23 15:57:06]

Patchset 26 (id:??) landed as
https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}

[Michael Achenbach, 2016-09-23 17:09:12]

On 2016/09/23 15:57:06, commit-bot: I haz the power wrote:
> Patchset 26 (id:??) landed as
> https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
> Cr-Commit-Position: refs/heads/master@{#39678}

Could you please fix:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder...

[Michael Achenbach, 2016-09-23 17:57:35]

A revert of this CL (patchset #26 id:490001) has been created in
https://codereview.chromium.org/2361053004/ by machenbach@chromium.org.

The reason for reverting is: Main suspect for tsan:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/11893

Also changes layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

+mips builder:
https://build.chromium.org/p/client.v8.ports/builders/V8%20Mips%20-%20builder....

[titzer, 2016-09-27 14:10:04]

Description was changed from

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}
==========

to

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}
==========

[titzer, 2016-09-27 14:10:25]

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

[commit-bot: I haz the power, 2016-09-27 14:10:43]

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-09-27 15:01:02]

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

[commit-bot: I haz the power, 2016-09-27 15:01:05]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-09-27 20:42:27]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-09-27 20:42:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2345593003/#ps510001
(title: "Fix test failures and TSAN races.")

[commit-bot: I haz the power, 2016-09-27 20:42:44]

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-09-27 20:46:24]

Description was changed from

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}
==========

to

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}
==========

[commit-bot: I haz the power, 2016-09-27 20:46:27]

Committed patchset #27 (id:510001)

[commit-bot: I haz the power, 2016-09-27 20:46:46]

Description was changed from

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Cr-Commit-Position: refs/heads/master@{#39678}
==========

to

==========
[wasm] Master CL for Binary 0xC changes.

[0xC] Convert to stack machine semantics.
[0xC] Use section codes instead of names.
[0xC] Add elements section decoding.
[0xC] Decoding of globals section.
[0xC] Decoding of memory section.
[0xC] Decoding of imports section.
[0xC] Decoding of exports section.
[0xC] Decoding of data section.
[0xC] Remove CallImport bytecode.
[0xC] Function bodies have an implicit block.
[0xC] Remove the bottom label from loops.
[0xC] Add signatures to blocks.
[0xC] Remove arities from branches.
Add tests for init expression decoding.
Rework compilation of import wrappers and how they are patched.
Rework function indices in debugging.
Fix ASM->WASM builder for stack machine.
Reorganize asm.js foreign functions due to import indices change.

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

Committed: https://crrev.com/76eb976a67273b8c03c744f64ad850b0432554b9
Committed: https://crrev.com/28392ab1968199b7d448c1868aebabdea99318d4
Cr-Original-Commit-Position: refs/heads/master@{#39678}
Cr-Commit-Position: refs/heads/master@{#39795}
==========

[commit-bot: I haz the power, 2016-09-27 20:46:49]

Patchset 27 (id:??) landed as
https://crrev.com/28392ab1968199b7d448c1868aebabdea99318d4
Cr-Commit-Position: refs/heads/master@{#39795}