-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add custom "emscripten_metadata" section to standalone WASM #7815
Conversation
7aa3b58
to
2073e73
Compare
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 (Added a comment in the code that indicates how to increment those constants) |
2073e73
to
0232880
Compare
There was a problem hiding this 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.
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 |
2d91b0a
to
b30f108
Compare
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. |
That's a good point, this is all still experimental and tools can easily tell the user what to do if metadata is missing for now. That makes me feel a lot better. Okay well I guess that's all for me, lmk if I should make any other changes.
|
There was a problem hiding this 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.
b30f108
to
6d10e2b
Compare
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). |
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. |
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. |
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