-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Do wasm/js glue in emscripten #4859
Conversation
…stuff like async compilation
@@ -2032,4 +2032,343 @@ Module['FS_createPreloadedFile'] = FS.createPreloadedFile; | |||
var cyberDWARFFile = '{{{ BUNDLED_CD_DEBUG_FILE }}}'; | |||
#endif | |||
|
|||
#if BINARYEN | |||
function integrateWasmJS(Module) { |
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.
Is there any change to this integration code aside from just moving it here?
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.
Almost none, except for the necessary changes in formatting (using emscripten's preprocessor instead of the hackish stuff we had before, so now we just use BINARYEN_METHOD
directly for example instead of python code doing a special search&replace).
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.
OK, great, this LGTM then. Also, (just so I understand) before this was at the very top of the JS file, and now it's in the preamble that gets pasted after environment setup but before everything else, right?
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.
Yeah, the location changed a little, but it should not matter, it's just a function declaration, and the call just needs to happen before we try to access the asm/wasm module (so doing it in preamble is ok, but postamble would have been wrong).
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.
Also the previous location was nonstandard in that it was before even pre-js files, which confused people. So good to fix for that reason too. The previous location was there just because it was an easy hack in the very early days.
In preparation for more advanced glue stuff like async compilation. This just moves it here from binaryen, with almost no changes but what is necessary to make it work.
Originally I had it in binaryen in hopes of it being a general thing useful for more projects, but as we get more complicated compilation stuff that needs deeper integration, that doesn't make sense anymore.