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

Add custom "emscripten_metadata" section to standalone WASM #7815

Merged

Conversation

rianhunter
Copy link
Contributor

Currently it's not possible execute Emscripten-generated WASM files without parsing certain data from the accompanying JS. This change adds that data to the wasm file itself so that standalone WASM files are executable by third-party WASM runtimes.

This change applies the new metadata section to both LLVM generated WASM and WASM generated by binaryen.

cc: @sunfishcode

@sunfishcode
Copy link
Collaborator

To make sure I understand, this adds a custom section named "emscripten_metadata" to the output wasm file which contains an ABI version number, and then in #7799 I just need to bump the major number, right?

@rianhunter
Copy link
Contributor Author

rianhunter commented Jan 5, 2019

To make sure I understand, this adds a custom section named "emscripten_metadata" to the output wasm file which contains an ABI version number, and then in #7799 I just need to bump the major number, right?

Ordinarily, for your breaking ABI change, the right thing to do would be to bump EMSCRIPTEN_ABI_MAJOR. Since the major version is currently at 0, it's enough to bump EMSCRIPTEN_ABI_MINOR.

(Added a comment in the code that indicates how to increment those constants)

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to make this optional, since it increases binary size and almost all current users don't need it. We can add an EMIT_EMSCRIPTEN_METADATA option (in src/settings.js), off by default (or some better name for the option, just an idea).

We'll also need a test, in say tests/test_other.py. It should at minimum test that we emit the section when the flag is present, doing so increases the wasm size, and the wasm still works.

emcc.py Outdated Show resolved Hide resolved
tools/shared.py Show resolved Hide resolved
tools/shared.py Outdated Show resolved Hide resolved
@rianhunter
Copy link
Contributor Author

rianhunter commented Jan 5, 2019

I'd prefer to make this optional, since it increases binary size and almost all current users don't need it. We can add an EMIT_EMSCRIPTEN_METADATA option (in src/settings.js), off by default (or some better name for the option, just an idea).

For what it's worth it increases the binary size by ~30 bytes which seems negligible but I'm sympathetic to this request. How do you feel about making it default-off when outputting html/js but default-on when outputting standalone WASM (except in the case of side modules)? Different defaults does seem kind of gross/unintuitive from a UI/UX standpoint but I really feel that users should be able to do emcc -o main.wasm main.c and by default be able to run "main.wasm" anywhere.

@rianhunter rianhunter force-pushed the add_wasm_emscripten_metadata branch 3 times, most recently from 2d91b0a to b30f108 Compare January 6, 2019 00:50
@kripken
Copy link
Member

kripken commented Jan 7, 2019

I don't feel strongly about different defaults for js+wasm vs wasm output, I agree there are upsides and downsides both ways. But I slightly prefer the simpler default-off in all cases. The JS or other loading code could have a clear error in the case the metadata is missing, and point people to the right option.

Another reason I'd rather not add this to the output by default is that it is somewhat experimental, and perhaps a more general ABI versioning (not emscripten specific) will become widely used, and we can switch to that. And so as a somewhat experimental thing, having people opt-in to it seems reasonable. But we can reevaluate that later too.

@rianhunter
Copy link
Contributor Author

rianhunter commented Jan 7, 2019 via email

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this basically looks good, just

  • Please add to the test a check that without the option the section is not emitted.
  • Please add yourself to AUTHORS

Currently it's not possible execute Emscripten-generated WASM files
without parsing certain data from the accompanying JS. This change adds
that data to the wasm file itself so that standalone WASM files
are executable by third-party WASM runtimes.
@kripken
Copy link
Member

kripken commented Jan 9, 2019

lgtm, thanks!

Btw, @rianhunter, I'd recommend not force-pushing to PRs: it doesn't send an email notification to people so it could be missed, and it is less incremental (forces people to read the whole diff each time, not just what's new).

@rianhunter
Copy link
Contributor Author

Good to know, didn't realize it wasn't notifying. I usually try to keep my commits structured around the features instead of structured around progression of the work to make it easier for future code archaeologists to figure out why code is the way it is. Next time I'll rebase after lgtm.

@kripken kripken merged commit 0d83546 into emscripten-core:incoming Jan 9, 2019
@kripken
Copy link
Member

kripken commented Jan 9, 2019

Yeah, there's no one right way to do this stuff (sometimes one commit makes sense, sometimes not, depends on the features etc.), but github not notifying on force-pushes is error-prone and surprising, in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants