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
kripken
left a comment
There was a problem hiding this comment.
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.
|
kripken
left a comment
There was a problem hiding this comment.
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