Fix EXPORT_NAME + MODULARIZE_INSTANCE + pthreads #10449
Conversation
| @@ -4954,3 +4954,18 @@ def test_wasm2js_fallback(self): | |||
| open('test.html', 'w').write(html) | |||
| os.remove('test.wasm') # Also delete the Wasm file to test that it is not attempted to be loaded. | |||
| self.run_browser('test.html', 'hello!', '/report_result?0') | |||
|
|
|||
| # Test that basic thread creation works in combination with MODULARIZE_INSTANCE=1 and EXPORT_NAME=MyModule | |||
| @no_fastcomp('more work would be needed for this to work in deprecated fastcomp') | |||
There was a problem hiding this comment.
what is the difference with fastcomp that causes this to fail? Iiuc MODULARIZE_INSTANCE is an identical feature in both backends?
There was a problem hiding this comment.
Fastcomp emits a bunch of code in the HTML to deal with the asm.js memory init file, and that has hardcoded Module in there (instead of the custom export name). From the stack trace I think that's what goes wrong, but I didn't investigate.
| @@ -258,7 +272,7 @@ this.onmessage = function(e) { | |||
| #if ASSERTIONS | |||
| } else { | |||
| // else e == 'unwind', and we should fall through here and keep the pthread alive for asynchronous events. | |||
| out('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.'); | |||
| err('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.'); | |||
There was a problem hiding this comment.
| err('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.'); | |
| out('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.'); |
There was a problem hiding this comment.
Since it looks like it's only used for debugging purposes. Correct me if I'm wrong.
There was a problem hiding this comment.
It is for debugging, yeah. So it actually seems natural to use err instead of out to me? But I guess we don't have a very clear policy on such logging (which doesn't matter much in practice on the web console). I'm happy to do it either way people prefer.
There was a problem hiding this comment.
I see. The out / threadPrintOut functions however seems to be unused now in worker.js, so perhaps these can be removed?
There was a problem hiding this comment.
I guess they can. I added them for completeness, as they may be needed in the future. But I believe we don't DCE this file, so probably best to remove, I'll do that.
| // in the global scope. | ||
| #if MODULARIZE_INSTANCE && EXPORT_NAME != 'Module' | ||
| var {{{ EXPORT_NAME }}} = Module; | ||
| #endif |
There was a problem hiding this comment.
Would it work instead of
var Module = {};
// If we use a custom export name, refer to Module from it as well, so that
// we connect properly to the modularized instance which is the only thing
// in the global scope.
#if MODULARIZE_INSTANCE && EXPORT_NAME != 'Module'
var {{{ EXPORT_NAME }}} = Module;
#endif
to do
var {{{ EXPORT_NAME }}} = {};
and in all places in the worker below, access {{{ EXPORT_NAME }}} instead of Module?
There was a problem hiding this comment.
That might work too, but I think it makes the code a little less readable, and there's a risk of future changes to the file forgetting and using Module (and then working except for the corner case fixed here).
This is a regression from #10566 which renamed that setting to INITIAL_MEMORY, and #10449 which added a test using that setting in that way, and landed close enough that we didn't notice it was doing so. It should have worked since we tried to support the old name of the setting, but this uncovered a bug, so it's actually good that it broke! The bug is that we have special handling for expanding things like 64MB into a number, and that handling did not work on aliases. As a solution, map aliases very early in parsing.
In that case we have just the custom EXPORT_NAME in
the global scope, and must connect to that properly in
the worker.js file which only sees the global scope.
Also we cannot use
out()/err()for logging as those areinternals of the emitted code, which in MODULARIZE*
mode are not available to worker.js as well.
fixes #9091