[wasm] Add support for exporting WebAssembly.Table instances.

[wasm] Add support for exporting WebAssembly.Table instances.

R=bradnelson@chromium.org, rossberg@chromium.org
BUG=v8:5507
Committed: https://crrev.com/ad9cf53473a960d925430c452149880ce621ad8f
Cr-Commit-Position: refs/heads/master@{#40554}

[titzer, 2016-10-24 19:41:38]

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

[commit-bot: I haz the power, 2016-10-24 19:41: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-10-24 19:45:19]

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

[commit-bot: I haz the power, 2016-10-24 19:45:19]

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

[titzer, 2016-10-24 19:56:24]

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

[commit-bot: I haz the power, 2016-10-24 19:56:27]

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-10-24 20:25:02]

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

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

Dry run: This issue passed the CQ dry run.

[bradnelson, 2016-10-25 07:33:40]

lgtm with suggestions

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:720: for (int i = 0; i < static_cast<int>(initial); ++i)
(*array)->set(i, null);
We're gonna have a blast when we start caring about >2**31 ArrayBuffers.
This is fine..
(Any chance we should look into making FixedArray and friends move to size_t ?

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1058:
init_table.new_entries.insert(init_table.new_entries.begin(),
There will be one by default correct?

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1663:
static_cast<int>(inited_table.new_entries[i]->func_index);
These can only be in bounds correct? DCHECK?

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
File test/mjsunit/wasm/indirect-tables.js (right):

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-tables.js:50: builder.addFunctionTableInit(g, true,
[f.mul.index, f.add.index, f.sub.index]);
>80

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-tables.js:53: let module = new
WebAssembly.Module(builder.toBuffer());
What about a case for imports into a different module?

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-tables.js:79: assertEquals("add", add.name);
Do we maybe want to have export names that don't match the names?
i.e. we could be getting the wrong field and not notice?

[rossberg, 2016-10-25 08:19:20]

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1527: // If there are any exported tables, cache the
JSFunctions created
Don't you need to do that anyway, in case the same Wasm function is exported
multiple times?

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1565: Handle<Code> export_code =
Like here, you should first check whether exported_functions already has an
entry for that index.

[titzer, 2016-10-25 08:23:13]

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1527: // If there are any exported tables, cache the
JSFunctions created
On 2016/10/25 08:19:20, rossberg wrote:
> Don't you need to do that anyway, in case the same Wasm function is exported
> multiple times?

I don't think we've spec'd that case yet.
As it is now, each export creates a new JSFunction that has a .name that
corresponds to the export name.

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1565: Handle<Code> export_code =
On 2016/10/25 08:19:20, rossberg wrote:
> Like here, you should first check whether exported_functions already has an
> entry for that index.

That would mean that there would be only one exported JSFunction and its name
would be the name of the first export. I'm not sure whether that's more or less
surprising than the current behavior.

[titzer, 2016-10-25 08:27:25]

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

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

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

[titzer, 2016-10-25 08:27:37]

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-js.cc#new...
src/wasm/wasm-js.cc:720: for (int i = 0; i < static_cast<int>(initial); ++i)
(*array)->set(i, null);
On 2016/10/25 07:33:39, bradnelson wrote:
> We're gonna have a blast when we start caring about >2**31 ArrayBuffers.
> This is fine..
> (Any chance we should look into making FixedArray and friends move to size_t ?

Ooooh yeah. I added a TODO here. That's probably a week-long fixit situation.

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1058:
init_table.new_entries.insert(init_table.new_entries.begin(),
On 2016/10/25 07:33:39, bradnelson wrote:
> There will be one by default correct?

This code handles any number of tables with any sizes.

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1663:
static_cast<int>(inited_table.new_entries[i]->func_index);
On 2016/10/25 07:33:39, bradnelson wrote:
> These can only be in bounds correct? DCHECK?

The loop iterates over the inited_table.new_entries, so it better be in bounds
:-)

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
File test/mjsunit/wasm/indirect-tables.js (right):

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-tables.js:50: builder.addFunctionTableInit(g, true,
[f.mul.index, f.add.index, f.sub.index]);
On 2016/10/25 07:33:39, bradnelson wrote:
> >80

Done.

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-tables.js:53: let module = new
WebAssembly.Module(builder.toBuffer());
On 2016/10/25 07:33:39, bradnelson wrote:
> What about a case for imports into a different module?

Add a case for adding an imported function to the indirect table.

I plan to add more tests when I implement importing of tables.

https://codereview.chromium.org/2443353002/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-tables.js:79: assertEquals("add", add.name);
On 2016/10/25 07:33:39, bradnelson wrote:
> Do we maybe want to have export names that don't match the names?
> i.e. we could be getting the wrong field and not notice?

Done.

[rossberg, 2016-10-25 08:28:59]

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

https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1527: // If there are any exported tables, cache the
JSFunctions created
On 2016/10/25 08:23:13, titzer wrote:
> On 2016/10/25 08:19:20, rossberg wrote:
> > Don't you need to do that anyway, in case the same Wasm function is exported
> > multiple times?
> 
> I don't think we've spec'd that case yet.
> As it is now, each export creates a new JSFunction that has a .name that
> corresponds to the export name.

Actually, JS.md explicitly requires it. ;)

[titzer, 2016-10-25 08:33:00]

On 2016/10/25 08:28:59, rossberg wrote:
> https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:1527: // If there are any exported tables, cache the
> JSFunctions created
> On 2016/10/25 08:23:13, titzer wrote:
> > On 2016/10/25 08:19:20, rossberg wrote:
> > > Don't you need to do that anyway, in case the same Wasm function is
exported
> > > multiple times?
> > 
> > I don't think we've spec'd that case yet.
> > As it is now, each export creates a new JSFunction that has a .name that
> > corresponds to the export name.
> 
> Actually, JS.md explicitly requires it. ;)

Ack.

[titzer, 2016-10-25 08:49:49]

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

[commit-bot: I haz the power, 2016-10-25 08:49:57]

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

[titzer, 2016-10-25 08:50:07]

On 2016/10/25 08:33:00, titzer wrote:
> On 2016/10/25 08:28:59, rossberg wrote:
> >
https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc
> > File src/wasm/wasm-module.cc (right):
> > 
> >
>
https://codereview.chromium.org/2443353002/diff/20001/src/wasm/wasm-module.cc...
> > src/wasm/wasm-module.cc:1527: // If there are any exported tables, cache the
> > JSFunctions created
> > On 2016/10/25 08:23:13, titzer wrote:
> > > On 2016/10/25 08:19:20, rossberg wrote:
> > > > Don't you need to do that anyway, in case the same Wasm function is
> exported
> > > > multiple times?
> > > 
> > > I don't think we've spec'd that case yet.
> > > As it is now, each export creates a new JSFunction that has a .name that
> > > corresponds to the export name.
> > 
> > Actually, JS.md explicitly requires it. ;)
> 
> Ack.

Done and added test. PTAL

[rossberg, 2016-10-25 08:59:55]

lgtm

[commit-bot: I haz the power, 2016-10-25 09:15:06]

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

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

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-25 09:42:35]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-10-25 09:42:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2443353002/#ps60001
(title: "Fix identity of exported JSFunctions")

[commit-bot: I haz the power, 2016-10-25 09:42:43]

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-10-25 09:44:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-17 22:12:11]

Description was changed from

==========
[wasm] Add support for exporting WebAssembly.Table instances.

R=bradnelson@chromium.org, rossberg@chromium.org
BUG=v8:5507
==========

to

==========
[wasm] Add support for exporting WebAssembly.Table instances.

R=bradnelson@chromium.org, rossberg@chromium.org
BUG=v8:5507

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

[commit-bot: I haz the power, 2016-11-17 22:12:12]

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