Add Foreign Functions to asm to wasm

Add Foreign Functions to asm to wasm

R=bradnelson@chromium.org
BUG=https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm.js
LOG=N

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

[aseemgarg, 2016-02-05 01:00:01]

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

https://codereview.chromium.org/1667253003/diff/1/test/mjsunit/wasm/asm-wasm....
test/mjsunit/wasm/asm-wasm.js:1061: function ffii() {
If I change the name of this to ffi, the test above (TestForeignFunctions) fails
with:
test/mjsunit/wasm/asm-wasm.js:1035: WasmModule::Instantiate(): FFI function
#3:setVal is not a JSFunction.

[bradnelson, 2016-02-05 01:19:43]

lgtm with changes

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.cc
File src/wasm/asm-wasm-builder.cc (right):

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.c...
src/wasm/asm-wasm-builder.cc:516: ZoneHashMap table_;
Make these private

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.c...
src/wasm/asm-wasm-builder.cc:526: ZoneHashMap::Entry* entry =
table_.LookupOrInsert(
Huh, hadn't considered that you need to track the mapping from variable to name,
that's a pain.

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.c...
src/wasm/asm-wasm-builder.cc:566: (vp->var()->index() == 1)) {
drop extra parens

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

https://codereview.chromium.org/1667253003/diff/1/test/mjsunit/wasm/asm-wasm....
test/mjsunit/wasm/asm-wasm.js:1061: function ffii() {
On 2016/02/05 01:00:01, aseemgarg wrote:
> If I change the name of this to ffi, the test above (TestForeignFunctions)
fails
> with:
> test/mjsunit/wasm/asm-wasm.js:1035: WasmModule::Instantiate(): FFI function
> #3:setVal is not a JSFunction.

Javascript resolves scoping after parsing everything.
So the last definition wins (but affects calls even before it.

For example:
function foo() { return 3; } console.log(foo()); function foo() { return 4; }
Prints 4

In light of that maybe give these a better name, or for bonus points, wrap the
whole test (both functions) in a function, to give them a separate scope.

[aseemgarg, 2016-02-05 01:33:18]

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.cc
File src/wasm/asm-wasm-builder.cc (right):

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.c...
src/wasm/asm-wasm-builder.cc:516: ZoneHashMap table_;
On 2016/02/05 01:19:43, bradnelson wrote:
> Make these private

Done. Though seems like if not exposed in .h file, it is not required to care
about visibility.

https://codereview.chromium.org/1667253003/diff/1/src/wasm/asm-wasm-builder.c...
src/wasm/asm-wasm-builder.cc:566: (vp->var()->index() == 1)) {
On 2016/02/05 01:19:43, bradnelson wrote:
> drop extra parens

Done.

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

https://codereview.chromium.org/1667253003/diff/1/test/mjsunit/wasm/asm-wasm....
test/mjsunit/wasm/asm-wasm.js:1061: function ffii() {
On 2016/02/05 01:19:43, bradnelson wrote:
> On 2016/02/05 01:00:01, aseemgarg wrote:
> > If I change the name of this to ffi, the test above (TestForeignFunctions)
> fails
> > with:
> > test/mjsunit/wasm/asm-wasm.js:1035: WasmModule::Instantiate(): FFI function
> > #3:setVal is not a JSFunction.
> 
> Javascript resolves scoping after parsing everything.
> So the last definition wins (but affects calls even before it.
> 
> For example:
> function foo() { return 3; } console.log(foo()); function foo() { return 4; }
> Prints 4
> 
> In light of that maybe give these a better name, or for bonus points, wrap the
> whole test (both functions) in a function, to give them a separate scope.

Done. enclosed in a function

[aseemgarg, 2016-02-05 01:33:59]

The CQ bit was checked by aseemgarg@chromium.org

[aseemgarg, 2016-02-05 01:33:59]

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/1667253003/#ps20001
(title: " ")

[commit-bot: I haz the power, 2016-02-05 01:34:03]

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

[commit-bot: I haz the power, 2016-02-05 02:07:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-05 02:08:05]

Description was changed from

==========
Add Foreign Functions to asm to wasm

R=bradnelson@chromium.org
BUG=https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm.js
LOG=N
==========

to

==========
Add Foreign Functions to asm to wasm

R=bradnelson@chromium.org
BUG=https://bugs.chromium.org/p/v8/issues/detail?id=4203
TEST=asm-wasm.js
LOG=N

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

[commit-bot: I haz the power, 2016-02-05 02:08:06]

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