Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] esm: basic wasm module env #27815

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

The current tests for Wasm modules is only testing modules that were compiled from Wast to Wasm.

When trying to get some basic functions working that were compiled via emscripten I quickly found that those modules were expecting a gloabal env that is generated for the .js wrapper.

Here's a link to the upstream source--> https://github.com/emscripten-core/emscripten/blob/incoming/src/preamble.js#L1117-L1158

This is a really naive PR tracking my work trying to get off the shelf wasm working as modules, it seems like there are lots of edge cases. I'm not 100% we want to ship this specific, and arguably internal, details of emscripten in core... hope to use this PR for a discussion in figuring out what makes the most sense.

@nodejs/modules

@nodejs-github-bot
Copy link
Collaborator

@MylesBorins
Copy link
Contributor Author

@MylesBorins MylesBorins added esm Issues and PRs related to the ECMAScript Modules implementation. wasm Issues and PRs related to WebAssembly. wip Issues and PRs that are still a work in progress. labels May 22, 2019
@xtuc
Copy link

xtuc commented May 22, 2019

We would be against shipping that, as it's emscripten specific. It's a well known issue, I filed an issue (on the wrong repo) here: WebAssembly/esm-integration#5.

In order to make it work I used to run a transformation the binary to point to an actual ESM file: https://github.com/wasm-tool/emscripten/blob/master/loader.js#L69-L71. It would make sense to me to do something similar for testing.

I'm happy to help if you need.

@devsnek
Copy link
Member

devsnek commented May 22, 2019

agreed with xtuc. it would also cause confusion for wasms built from rust which uses env for unresolved symbols

@lukewagner
Copy link

Agreed with xtuc as well. Moreover, if you use a pure upstream clang + wasi-sysroot compiler, iiuc, there is no env import; WASI effectively takes over this role. Hopefully we can get Emscripten to emit pure-WASI builds (or have a flag to do so) in the future.

@chjj
Copy link
Contributor

chjj commented May 22, 2019

I think ESM wasm is flawed for this reason. Lots of wasm modules have very complicated setups. IMO, import "./whatever.wasm" should return a compiled Module as the default export rather than an Instance. This would solve the problem of easy instantiation for everything (even emscripten), but Chrome pre-emptively broke this approach by limiting both Modules and Instances to 4kb for synchronous instantiation.

There's so much wrong with wasm module instantiation that it's practically unfixable at this point. I spent a few days trying to write an emscripten wrapper that provided consistent behavior between the browser and node -- it's impossible because of Chrome's arbitrary 4kb limit for sync instantiation. Surely ESM wasm would be the answer? Nope, that's broken too. =/

@ofrobots
Copy link
Contributor

@lukewagner

Moreover, if you use a pure upstream clang + wasi-sysroot compiler, iiuc, there is no env import; WASI effectively takes over this role.

FYI, this doesn't seem to be the case just yet.. wasi-sdk-5.0 still ends up emitting env dependencies for me:

  ...
  (type (;17;) (func (param i32 i32 i32 i32 i32 i32 i32)))
  (import "env" "__indirect_function_table" (table (;0;) 3 funcref))
  (import "env" "__stack_pointer" (global (;0;) (mut i32)))
  (import "env" "__memory_base" (global (;1;) i32))
  (import "env" "__table_base" (global (;2;) i32))
  (import "wasi_unstable" "fd_prestat_get" (func $__wasi_fd_prestat_get (type 0)))
  (import "wasi_unstable" "fd_prestat_dir_name" (func $__wasi_fd_prestat_dir_name (type 3)))
  (import "wasi_unstable" "environ_sizes_get" (func $__wasi_environ_sizes_get (type 0)))
  (import "wasi_unstable" "environ_get" (func $__wasi_environ_get (type 0)))
  (import "wasi_unstable" "args_sizes_get" (func $__wasi_args_sizes_get (type 0)))
  (import "wasi_unstable" "args_get" (func $__wasi_args_get (type 0)))
  (import "env" "main" (func $main (type 0)))
  (import "wasi_unstable" "proc_exit" (func $__wasi_proc_exit (type 4)))
  (import "wasi_unstable" "fd_fdstat_get" (func $__wasi_fd_fdstat_get (type 0)))
  ...

@lukewagner
Copy link

Oops, sorry to give you incorrect information; I had thought that was the goal. Maybe @sunfishcode can shed some light on what the plan is?

@sunfishcode
Copy link

@ofrobots Statically-linked WASI executables don't have env dependencies in them. It looks like you may have used -Wl,-shared or similar here. Dynamic linking is not supported in WASI yet.

@ofrobots
Copy link
Contributor

Indeed, there was confusion on my part about to link a wasm library without a main. -shared wasn't the answer.

@MylesBorins
Copy link
Contributor Author

Feels like there has been good discussion.. closing in lieu of #27850

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. wasm Issues and PRs related to WebAssembly. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants