Fix --post-js scripts to execute after main() in MODULARIZE mode in MINIMAL_RUNTIME #25313
Conversation
juj
changed the title
| be executed after main() has run. | ||
| Like "--pre-js", but emits a file *after* the emitted code. In addition to | ||
| being located after the emitted JS library code, --post-js code will also | ||
| execute only after main() has run. |
There was a problem hiding this comment.
Is this actually the behavior we want though?
Is this true today? I would have though that WASM_ASYNC_COMPILATION=1 (which is the default) means that the code runs before we run main (while the wasm file is loading).
Which behaviour do we think is more useful / expected?
There was a problem hiding this comment.
In https://github.com/emscripten-core/emscripten/pull/23157/files#r1889243405 you had commented
This means that postjs code, which comes after main will now run after main like it would do with sync instantiation.
I think this is a pretty reasonable change.
so I wanted to highlight that as it is the behavior that the test suite depends upon.
There was a problem hiding this comment.
I would also be ok to leaving this unspecified (it is a code size increase in MINIMAL_RUNTIME after all, and not all users might need that guarantee). Although I wonder if that fights against the ChangeLog note in that PR
One side effect of this is that code in --post-js files will now be delayed until after module creation and after main runs. This matches the existing behaviour when using sync instantation (-sWASM_ASYNC_COMPILATION=0) but is an observable difference. (#23157)
that mentions this to be the new behavior.
There was a problem hiding this comment.
I guess maybe we should be consistent, but for now it seems like -sMODULARIZE has already been updated to have the well-defined behaviour of running post-js after main.
For code built without -sMODULARIZE it currently depends if you build with async compilation or not, so its not as simple as saying that post-js code always runs after main.
So, perhaps revert the documentation change here until and we can separately consider how to document the current differences between modes (or remove them and then document the consistent new behaviour)?
Documenting this as always being one way would currently be inaccurate I think?
juj
force-pushed
the
minimal_runtime_post_js
branch
from
b3aebfa to
eb58b29
Compare
Yes, the test If this is the only test in the suite that depends on this behavior, then I think another way to resolve this test failure is to change the test or verify a |
…ODULARIZE mode. Fixes test minimal0.test_unicode_js_library after this change: https://github.com/emscripten-core/emscripten/pull/23157/files#r1889243405
juj
force-pushed
the
minimal_runtime_post_js
branch
from
eb58b29 to
033194e
Compare
Can you add this test at line 708 of I agree the intent of this test does not seem to be explicitly checking/verifying this ordering, but I'd be happy to land this with that tests as confirmation. Alternatively you write a specialized new test for just this, but we can also do that as a followup when/if we decide to lock down the beaviour in all configurations. |
| #if PTHREADS || WASM_WORKERS | ||
|
|
||
| #if MODULARIZE | ||
| await instantiatePromise; |
There was a problem hiding this comment.
Maybe one day we can find a way to clean this up and just use async/await instead of have this global instantiatePromise.
lgtm for now though.
| self.cflags += ['-sMODULARIZE', '--js-library', test_file('unicode_library.js'), '--extern-post-js', test_file('modularize_post_js.js'), '--post-js', test_file('unicode_postjs.js')] | ||
| # In addition to verifying that --post-js files can contain UTF8 characters, | ||
| # this test verifies a specific semantic that in -sMODULARIZE mode, the | ||
| # content in --post-js files should be executed only after main() runs. |
There was a problem hiding this comment.
Can you reference the bug I just created here: #25346
|
Do you want to land this or wait for us to resolve #25346 (since it may change the behaviour here) |
Fix --post-js scripts to execute after main() in MODULARIZE mode. Fixes test
minimal0.test_unicode_js_libraryafter this change: https://github.com/emscripten-core/emscripten/pull/23157/files#r1889243405