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

Emterpreter data not embedded in WASM #5330

Closed
curiousdannii opened this issue Jun 22, 2017 · 13 comments
Closed

Emterpreter data not embedded in WASM #5330

curiousdannii opened this issue Jun 22, 2017 · 13 comments
Labels

Comments

@curiousdannii
Copy link
Contributor

curiousdannii commented Jun 22, 2017

If you're compiling to WASM with the Emterpreter, the Emterpreter data is not embedded within the WASM and is either left as a standalone file, or embedded as an array in the JS.

The WASM spec allows you to have multiple data sections, and the Emterpreter data is just being set in the main heap, so it should be possible to have it embedded in the WASM, right?

Also, the helpful error for when you forget to load the data file Assertion failed: bad emterpreter file in -O0 turns into the much less clear failed to asynchronously prepare wasm: RuntimeError: remainder by zero in -O3.

@kripken
Copy link
Member

kripken commented Jun 22, 2017

Good idea, this would be useful to do.

About those errors, do you have a testcase showing them?

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Jun 23, 2017

What form of testcase would be most useful for you?

I could very easily make two branches to checkout of my moderate sized project with the -O setting changed, and then all you'd need to do is run make, if that would be enough.

@kripken
Copy link
Member

kripken commented Jun 23, 2017

Yeah, that would be enough. Just as long as it's a simple checkout + run make (i.e. I don't need to install a build system or other tools).

@curiousdannii
Copy link
Contributor Author

I was able to make a minimal test case: https://gist.github.com/curiousdannii/07f1d750226befd4cbf6d858f204ac94

The -O3 error message is different however: failed to asynchronously prepare wasm: RuntimeError: memory access out of bounds

@kripken
Copy link
Member

kripken commented Jun 30, 2017

I tried to debug this in the spidermonkey shell, where the error is different. It works without the emterpreter file, but with it, it fails. Looks like it hits this assertion:

  var bytecodeFile = Module['emterpreterFile'];
  assert(bytecodeFile instanceof ArrayBuffer, 'bad emterpreter file');

Looking in the docs, it says

src/settings.js:                            // When emitting HTML, we automatically generate code to load this file and set it to Module.emterpreterFile. If you
src/settings.js:                            // emit JS, you need to make sure that Module.emterpreterFile contains an ArrayBuffer with the bytecode, when the code loads.

So looks like the user needs to set that file, since we compiled to JS and not HTML. Maybe that's the problem you were hitting?

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Jun 30, 2017

So looks like the user needs to set that file, since we compiled to JS and not HTML. Maybe that's the problem you were hitting?

Yeah, but the issue is that the explicit error message is shown in -O0, but in -O3 (I think actually -O1+) the error message isn't shown, and some other WASM loading error is shown.

Or is this by design - error messages like this are only meant to be shown when assertions are on? If so, then I think missing file errors like this deserve being made normal errors and shown even when assertions are off. FWIW the memory loader throws normally.

If #5104 was resolved through a centralised loading system then we wouldn't need to have multiple places checking and throwing errors. :)

@kripken
Copy link
Member

kripken commented Jul 3, 2017

I agree, an error like that should be handled even when assertions are off. We should replace the assert on it with a check and call of abort, PR would be welcome.

@curiousdannii
Copy link
Contributor Author

According to the WASM spec this may not be possible yet:

In the current version of WebAssembly, at most one memory is allowed in a module. Consequently, the only valid memidxmemidx is 00.

@kripken
Copy link
Member

kripken commented Oct 16, 2017

It's true we can't embed it as a new memory, but we can enlarge the existing single memory and add it into there. We also need to update the static-bump stuff so the compiler knows that we are using more memory statically.

@curiousdannii
Copy link
Contributor Author

curiousdannii commented Dec 1, 2017

I'd be interested in working on this, but I think it will probably end up being beyond me.

I took a look, and I couldn't see any sort of general purpose wasm embedder.

Two options I can see:

  1. Alter asm2wasm to take extra options for the emterpreter file and offset, and then add the extra memory segment
  2. Create a full wasm file with basically nothing in it except the emterpreter memory, and then merge them

I think the first is probably better, but it would require me to dig into the binaryen source, whereas the second may potentially be doable just in emcc.py.

@kripken
Copy link
Member

kripken commented Dec 1, 2017

Option 2 would only be practical once we emit wasms with relocation info, as that info would contain the "static bump" metadata that is necessary for such merging, I'm afraid.

And I think there is a better option than Option 1. Look at make_shared_library in tools/shared.py, it reads the STATIC_BUMP info from the JS (going back to the previous paragraph, in the future it could read it from the wasm object file metadata). So it's practical to read and write the bump (currently, at least before the JS is minified), and so the emterpretify code could append the emterpreter binary code into the mem init file, after the current static bump (appending zeros first if necessary), and then increase the static bump in the JS.

@kripken
Copy link
Member

kripken commented Dec 1, 2017

(And then the mem init file would get imported into the wasm normally, in asm2wasm, so there is no wasm-specific code here. But, it might be simpler to do this in a wasm-only way, as when we minify the JS might be different, so ignoring the asm.js case is fine.)

@stale
Copy link

stale bot commented Sep 19, 2019

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Sep 19, 2019
@stale stale bot closed this as completed Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants