[wasm] Support undefined indirect table entries, behind a flag.

[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and dynamic linking by allowing additional space to be filled with an "undefined" function signature.

BUG=v8:5044
LOG=N
TEST=None
R=mtrofin@chromium.org,bradnelson@chromium.org

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

[ritesht, 2016-06-07 22:06:14]

Description was changed from

==========
[wasm] Add a flag to gate experimental support for dynamic code loading and
JITing.
Enhancing functionality of the indirect function table to support JITing and
dynamic linking by pre-allocating space for wasm functions and modules to be
linked in.

BUG=5044
==========

to

==========
[wasm] Add a flag to gate experimental support for dynamic code loading and
JITing.
Enhancing functionality of the indirect function table to support JITing and
dynamic linking by pre-allocating space for wasm functions and modules to be
linked in.

BUG=5044
==========

[ritesht, 2016-06-07 22:06:15]

ritesht@google.com changed reviewers:
+ titzer@chromium.org

[bradn, 2016-06-14 01:53:07]

Description was changed from

==========
[wasm] Add a flag to gate experimental support for dynamic code loading and
JITing.
Enhancing functionality of the indirect function table to support JITing and
dynamic linking by pre-allocating space for wasm functions and modules to be
linked in.

BUG=5044
==========

to

==========
[wasm] Add a flag to gate experimental support for dynamic code loading and
JITing.
Enhancing functionality of the indirect function table to support JITing and
dynamic linking by pre-allocating space for wasm functions and modules to be
linked in.

BUG=5044
LOG=N
TEST=None
R=titzer@chromium.org, ahaas@chromium.org
==========

[bradn, 2016-06-14 01:53:07]

bradnelson@google.com changed reviewers:
+ ahaas@chromium.org

[bradn, 2016-06-14 01:56:49]

Description was changed from

==========
[wasm] Add a flag to gate experimental support for dynamic code loading and
JITing.
Enhancing functionality of the indirect function table to support JITing and
dynamic linking by pre-allocating space for wasm functions and modules to be
linked in.

BUG=5044
LOG=N
TEST=None
R=titzer@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=5044
LOG=N
TEST=None
R=titzer@chromium.org, ahaas@chromium.org
==========

[bradn, 2016-06-14 02:02:45]

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

[bradn, 2016-06-14 02:02:46]

Oops, was going to direct this to ahaas, but actually we should talk first
tomorrow. Sorry should have looked sooner.

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

https://codereview.chromium.org/2049513003/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:1940: module_->instance->padded_entries);
Hey Ritesh.
Actually this isn't quite right.
We don't want to add extra runtime cost to check for an invalid signature.
Instead, we should have it thunk out to something that traps.

https://codereview.chromium.org/2049513003/diff/1/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/2049513003/diff/1/src/flag-definitions.h#newc...
src/flag-definitions.h:505: DEFINE_BOOL(wasm_jit_prototype, false, "enable jit
compiling")
Change the description text to something like:
enable experimental wasm runtime dynamic code generation

https://codereview.chromium.org/2049513003/diff/1/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/2049513003/diff/1/src/messages.h#newcode487
src/messages.h:487: T(WasmTrapDefaultFuncCall, "default function call")
I don't think we want to pay for a separate check.

[titzer, 2016-06-14 20:58:06]

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

https://codereview.chromium.org/2049513003/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:1940: module_->instance->padded_entries);
On 2016/06/14 02:02:46, bradn wrote:
> Hey Ritesh.
> Actually this isn't quite right.
> We don't want to add extra runtime cost to check for an invalid signature.
> Instead, we should have it thunk out to something that traps.

Agree.

[bradn, 2016-06-14 21:00:26]

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

https://codereview.chromium.org/2049513003/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:1940: module_->instance->padded_entries);
On 2016/06/14 20:58:05, titzer wrote:
> On 2016/06/14 02:02:46, bradn wrote:
> > Hey Ritesh.
> > Actually this isn't quite right.
> > We don't want to add extra runtime cost to check for an invalid signature.
> > Instead, we should have it thunk out to something that traps.
> 
> Agree.

Actually since it can't run (since we don't know the caller signature will
match), we should just lean on the signature mismatch exception instead.
(Which keeps the cost / callsite the same)

[ritesht, 2016-06-15 21:31:06]

There is now only one run-time check for whether the index is in bounds or not.
An 'invalid function' trap is generated if a call is made into the default
section of the indirect function table.

[bradn, 2016-06-16 08:00:20]

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

https://codereview.chromium.org/2049513003/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1935: int indirect_table_size =
Can we move the computation of the size to FunctionTableSize so this is the same
in both cases?

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

https://codereview.chromium.org/2049513003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:145: indirect_table_size = 4 * table_size <
kMinimumIndirectTableSize
I'd thought we'd talked about adding a field to one of the sections?

https://codereview.chromium.org/2049513003/diff/20001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:247: indirect_table_size = 4 * table_size <
kMinimumIndirectTableSize
Definitely don't want to repeat this calculation all over.

https://codereview.chromium.org/2049513003/diff/20001/test/mjsunit/wasm/defau...
File test/mjsunit/wasm/default-func-call.js (right):

https://codereview.chromium.org/2049513003/diff/20001/test/mjsunit/wasm/defau...
test/mjsunit/wasm/default-func-call.js:2: //
Extra line.
Maybe in a separate file?

https://codereview.chromium.org/2049513003/diff/20001/test/mjsunit/wasm/defau...
File test/mjsunit/wasm/default-func-call2.js (right):

https://codereview.chromium.org/2049513003/diff/20001/test/mjsunit/wasm/defau...
test/mjsunit/wasm/default-func-call2.js:2: //
Extra line.
Separate file?

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

https://codereview.chromium.org/2049513003/diff/20001/test/mjsunit/wasm/indir...
test/mjsunit/wasm/indirect-calls.js:6: // Flags: --wasm-jit-prototpe
probably a separate file here too

[ritesht, 2016-06-21 17:36:41]

Added a field that specifies the number of entries padded to the indirect
function table.

[bradn, 2016-06-21 17:42:37]

https://codereview.chromium.org/2049513003/diff/120001/test/mjsunit/wasm/defa...
File test/mjsunit/wasm/default-func-call.js (right):

https://codereview.chromium.org/2049513003/diff/120001/test/mjsunit/wasm/defa...
test/mjsunit/wasm/default-func-call.js:51: assertTraps(kTrapFuncSigMismatch,
"module.exports.main(2, 12, 33)");
Maybe make this just test the trapping directly?
I.e. make the wrapper function more direct?

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

https://codereview.chromium.org/2049513003/diff/120001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-module-builder.js:334: if (debug) print("emitting
indirect function table pad @ " + bytes.length);
>80 columns

[bradn, 2016-06-21 18:43:02]

lgtm with those fixes.

https://codereview.chromium.org/2049513003/diff/120001/test/mjsunit/wasm/defa...
File test/mjsunit/wasm/default-func-call.js (right):

https://codereview.chromium.org/2049513003/diff/120001/test/mjsunit/wasm/defa...
test/mjsunit/wasm/default-func-call.js:16: builder.addPad(512);
But how about we make this:
addPadFunctionTable ?

https://codereview.chromium.org/2049513003/diff/120001/test/mjsunit/wasm/defa...
test/mjsunit/wasm/default-func-call.js:51: assertTraps(kTrapFuncSigMismatch,
"module.exports.main(2, 12, 33)");
On 2016/06/21 17:42:37, bradn wrote:
> Maybe make this just test the trapping directly?
> I.e. make the wrapper function more direct?

Sorry confused with the test from yesterday.
This is fine.

[bradn, 2016-06-21 18:45:18]

Description was changed from

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=5044
LOG=N
TEST=None
R=titzer@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=v8:5044
LOG=N
TEST=None
R=mtrofin@chromium.org,bradnelson@chromium.org
==========

[bradn, 2016-06-21 18:45:18]

bradnelson@google.com changed reviewers:
+ bradnelson@chromium.org, mtrofin@chromium.org
- ahaas@chromium.org, bradnelson@google.com, titzer@chromium.org

[bradn, 2016-06-21 18:46:21]

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

[bradn, 2016-06-21 18:46:21]

Mircea, can you also take a look at this one?
And then we'll pass on to adamk/daniel top level.

[ritesht, 2016-06-21 19:01:41]

final formatting fix

[Mircea Trofin, 2016-06-21 19:46:15]

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

https://codereview.chromium.org/2049513003/diff/140001/src/wasm/module-decode...
src/wasm/module-decoder.cc:278: int length;
please initialize length.

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

https://codereview.chromium.org/2049513003/diff/140001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:148: int table_size = module->FunctionTableSize();
uint32_t table_size

https://codereview.chromium.org/2049513003/diff/140001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:152: int populated_table_size =
static_cast<int>(module->function_table.size());
could you avoid the cast to int here? in fact, you can avoid the temp variable
and just 

for (size_t i = 0; i < module->function_table.size(); ++i)

also, please use the prefix increments in loops, i.e. ++i, not i++. The latter
produces an intermediate value which is unnecessary.

https://codereview.chromium.org/2049513003/diff/140001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:681: int table_size =
instance->module->FunctionTableSize();
same note as above about avoiding the cast - I realize this was this way before,
but the cleanup would be appreciated. Thanks!

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

https://codereview.chromium.org/2049513003/diff/140001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:297: uint32_t FunctionTableSize() { return
module->FunctionTableSize(); }
uint32_t FunctionTableSize() const

[ritesht, 2016-06-21 21:42:11]

ritesht@google.com changed reviewers:
+ ahaas@chromium.org

[ritesht, 2016-06-21 22:25:47]

Replaced all instances of "i++" with "++i" in the wasm code.
Fixed the casting of the table sizes.

[bradn, 2016-06-21 22:27:52]

Thought we said those fixes would be a different cl.

[ritesht, 2016-06-21 23:16:33]

Fixed casting and formatting

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

https://codereview.chromium.org/2049513003/diff/200001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:155: i <
static_cast<uint32_t>(module->function_table.size());
I have to cast this as the "set" function requires an int32.

[Mircea Trofin, 2016-06-21 23:55:45]

On 2016/06/21 23:16:33, ritesht wrote:
> Fixed casting and formatting
> 
> https://codereview.chromium.org/2049513003/diff/200001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2049513003/diff/200001/src/wasm/wasm-module.c...
> src/wasm/wasm-module.cc:155: i <
> static_cast<uint32_t>(module->function_table.size());
> I have to cast this as the "set" function requires an int32.

LGTM, assuming adding "const" where previously noted, and assuming separate CL
for the
cleanup for increments and the uninitialized length parameter (that turns out
isn't needed to start
with, and we agreed to remove)

[ritesht, 2016-06-22 00:05:05]

ritesht@google.com changed reviewers:
+ dehrenberg@chromium.org

[ritesht, 2016-06-22 00:10:14]

@dehrenberg Could you check the "src/flag-definitions.h" file for new wasm flag?

[adamk, 2016-06-22 00:26:30]

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

[adamk, 2016-06-22 00:26:31]

lgtm for src/flag-definitions.h

[ritesht, 2016-06-22 00:35:31]

The CQ bit was checked by ritesht@google.com

[ritesht, 2016-06-22 00:35:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@google.com
Link to the patchset: https://codereview.chromium.org/2049513003/#ps220001
(title: "Fixed static casting and formatting.")

[commit-bot: I haz the power, 2016-06-22 00:35:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2049513003/220001

[commit-bot: I haz the power, 2016-06-22 00:39:32]

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

[commit-bot: I haz the power, 2016-06-22 00:39:33]

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

[bradnelson, 2016-06-22 00:50:02]

Lgtm from right account

[bradnelson, 2016-06-22 00:50:12]

The CQ bit was checked by bradnelson@chromium.org

[commit-bot: I haz the power, 2016-06-22 00:50:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2049513003/220001

[commit-bot: I haz the power, 2016-06-22 00:52:05]

Description was changed from

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=v8:5044
LOG=N
TEST=None
R=mtrofin@chromium.org,bradnelson@chromium.org
==========

to

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=v8:5044
LOG=N
TEST=None
R=mtrofin@chromium.org,bradnelson@chromium.org
==========

[commit-bot: I haz the power, 2016-06-22 00:52:06]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-22 00:53:38]

Description was changed from

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=v8:5044
LOG=N
TEST=None
R=mtrofin@chromium.org,bradnelson@chromium.org
==========

to

==========
[wasm] Support undefined indirect table entries, behind a flag.

Add a flag to gate experimental support for dynamic code loading and JITing (at
runtime in a wasm module).

Enhancing functionality of the indirect function table to support JITing and
dynamic linking by allowing additional space to be filled with an "undefined"
function signature.

BUG=v8:5044
LOG=N
TEST=None
R=mtrofin@chromium.org,bradnelson@chromium.org

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

[commit-bot: I haz the power, 2016-06-22 00:53:39]

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