Fix EXPORT_ES6 + USE_PTHREADS #8306

VirtualTim · 2019-03-19T06:39:42Z

I haven't managed to get this working, and I think I'm stuck, but I figured I'd create this PR so people can pick up where I left off. Or point out if I'm doing something wrong.

See https://html.spec.whatwg.org/multipage/dom.html#dom-document-currentscript

Import returns a promise, so things had to be rearranged. I think I could have used promise.resolve() instead, but that's something to revisit if I can actually get this working.

VirtualTim · 2019-03-20T09:08:42Z

Currently "_emscripten_futex_wake" is being optimised away, so I need to compile with -s EXPORTED_FUNCTIONS='["_emscripten_futex_wake"]'. I'm sure there is a way to force-preserve this, but I don't know where to add that. Anyone want to point me to the right file to add this to?

Edit: Seems to be fixed now. Not sure how that happened.

emcc.py

VirtualTim · 2019-03-25T08:26:13Z

Things seem to be mostly working now. I don't know why the test are failing, those webgl tests seem kind of flaky on firefox, so maybe it's unrelated.
~~It doesn't yet work with WASM=1, it throws with TypeError: Cannot assign to read only property trying to reassign functions.~~ Should be fixed.

Edit: I'll probably also need to update shell_minimal.js at some point.

…n the main thread. I feel like this should break things, but it seems to work properly with my tests.

VirtualTim · 2019-03-26T05:08:15Z

@kripken or @sbc100, since you both commented on the original issue, could one of you take a look at this PR? I think I've got it working, I'm pretty sure the test failures are unrelated.

I also want to add a test case for this, but I'm not really sure how to test something like this. I think I need some JS which loads the compiled ES6 module, but I don't rally know how to integrate this with the existing test framework.

sbc100

@juj probably has more insight into all things pthreads.

sbc100 · 2019-03-26T16:08:49Z

emcc.py

+      if shared.Settings.EXPORT_ES6:
+        GETSCRIPT = "import.meta.url"
+      else:
+        GETSCRIPT = "typeof document !== 'undefined' && document.currentScript ? document.currentScript.src : undefined"


No need to make this local variable uppercase.

Would this be better names "script_url" or something like that?

Why is the js var called _scripteDir? Isn't it the full URL?

Yeah good point. I didn't name that, but it is a bit confusing. _scriptDir can be a blob, or path to a script. Later on _scriptDir is assigned to scriptDirectory, which contains the directory of the script. I'm not sure what scriptDirectory is used for.
Anyway import.meta.url seems to return the same thing as document.currentScript.src, the difference being that import.meta.url works in a module, while document.currentScript.src doesn't.

emcc.py

sbc100 · 2019-03-26T16:11:34Z

emscripten.py

@@ -1666,6 +1666,10 @@ def create_receiving(function_table_data, function_tables_defs, exported_impleme

  shared.Settings.MODULE_EXPORTS = module_exports = exported_implemented_functions + function_tables(function_table_data)

+  # Should only need to do this in the main thread.
+  if shared.Settings.USE_PTHREADS:
+    receiving = "if (!ENVIRONMENT_IS_PTHREAD) {\n" + receiving + "}\n"


But the stuff added to receiving below we do want on non-main-threads? Can you explain, perhaps write more in the comment here?

In my testing it seems like these functions were already shared with the non-main-threads by the main thread. Calling these from a non-main-thread attempted to redefine them (as the same thing). No idea if that's how it's supposed to work.

Based on where this line is, it just puts the runtime assertions stuff behind this check. What's special about them? As @sbc100 said, I think this should all be needed in pthreads too - do you have a test that fails without this change?

When I tried to run the code before this change I was getting errors about the vars in this block being redefined. It seems like when initialising a thread these functions have already been defined (not sure how). Normally var redefinition isn't an issue as everything gets hoisted, so I assumed it was an ES6 module thing. I'll have to do some more testing, I can't remember exactly what error I was trying to avoid.

Although looking at that again I should add a check for if receiving is an empty string.

Edit: The actual error I get is:
Uncaught (in promise) TypeError: Cannot assign to read only property '__ZSt18uncaught_exceptionv' of object '[object Object]'
__ZSt18uncaught_exceptionv just happens to be the first in the list, it seems to error on all of them, causing initialising pthreads to fail.
I had assumed that these were getting defined twice, once in the main thread, once in workers, but it looks like you can't just redefine things like that in an ES6 module. I don't know why. I'll change the settings check to if shared.Settings.USE_PTHREADS and shared.Settings.EXPORT_ES6 and receiving: if I can't figure out how to get this working.

That's a very odd error - I don't think we use read-only properties anywhere?

Thanks, and sorry for my mistake earlier!

Ok, what I think is happening here is: when loaded as an ES6 module, the wasm exports object is an ES6 module export object, or behaves like one, and those are more "static" than normal JS objects. And we try to replace them with wrappers to add extra runtime error handling, and that fails.

But that doesn't explain why this is a problem only on workers but not on the main thread. It sounds like that could be a browser bug, in fact. That asm object is a wasm export object on both locations, so it should behave the same? Unless somehow only the worker is an ES6 context - I'm not too familiar with how those are created?

Regardless, we can split this out from the rest of this PR, if it's separable.

Yeah I don't really know too much about all that stuff either.
The change on that line allows EXPORT_ES6 + USE_PTHREADS to work with ASSERTIONS != 0. I think the best thing is to merge this PR as is, since by default ASSERTIONS=1.

Perhaps after this is merged another bug can be opened to fix those assertions?\

If you'd prefer I'm happy to remove that line, and just say that currently EXPORT_ES6 + USE_PTHREADS only work with ASSERTIONS=0.

Yeah, let's separate it out - a comment that says something like your last line there sounds good enough for now.

OK, I changed it to an assert, since the next line would just throw anyway. At least this way the developer gets a clear message about this not working. Once this is merged I'll open another issue to fix USE_PTHREADS + EXPORT_ES6 +ASSERTIONS.

src/shell_pthreads.js

VirtualTim · 2019-04-02T09:18:57Z

Well the tests failed because the test_openjpeg test didn't find the symbol __memory_base. Not sure why test_the_bullet failed, but that seems to be failing for everyone at the moment.
I'm pretty sure those are unrelated to my changes.

I'm happy for this to be merged, if everyone else is happy with the changes.

emcc.py

kripken · 2019-04-03T18:38:21Z

emscripten.py

@@ -1666,6 +1666,10 @@ def create_receiving(function_table_data, function_tables_defs, exported_impleme

  shared.Settings.MODULE_EXPORTS = module_exports = exported_implemented_functions + function_tables(function_table_data)

+  # Should only need to do this in the main thread.
+  if shared.Settings.USE_PTHREADS:
+    receiving = "if (!ENVIRONMENT_IS_PTHREAD) {\n" + receiving + "}\n"


Based on where this line is, it just puts the runtime assertions stuff behind this check. What's special about them? As @sbc100 said, I think this should all be needed in pthreads too - do you have a test that fails without this change?

src/worker.js

kripken · 2019-04-03T18:40:36Z

The test_openjpeg failures are unrelated, yeah, that was an issue we had (that should now be fixed on incoming I think).

) This helps wasm2js and pthreads currently, as those modes use a mem file.

… USE_PTHREADS is on (emscripten-core#8609)

…core#8595)

…ke_context_current (emscripten-core#8613) And add a complete test for both that and for emscripten_webgl_create_context in that mode (which also tests emscripten-core#8579)

…en-core#8601) Also, add a mechanism for supporting renamed settings. This enables users in non-strict mode to use old name for the settings, but users in strict mode must use the modern name. Part of emscripten-core#8317

emscripten-core#8600) Specifying WASM_OBJECT_FILES=0 on the link line is current needed if one want to select LTO libraries. However this should not exclude some input being wasm object files. Fixes emscripten-core#8599

…pten-core#8618) * Update fetch API documentation because streaming is Firefox only * Added note about EMSCRIPTEN_FETCH_STREAM_DATA being Firefox only

This test now passed with the llvm backend.

)

Returning EINVAL error when trying to call fsync on fd representing UNIX pipe. This fixes http://posixtest.sourceforge.net/ fsync/7-1 test.

Previously these tests weren't actually checking the right thing. This should fix the logic to what I think the author was trying to do. Fixes emscripten-core#8620.

…ore#8631)" This reverts commit 07116f8.

This works with the wasm backend, whether generating wasm or asm.js. Also added `emscripten_return_address` which implements the functionality of `__builtin_return_address` when running both wasm and asm.js.

Once detached threads are finished their execution they emit the 'exit' command. Instead of a noop they should rejoin the pool. Resolves emscripten-core#8201.

* Make sure to note temp files that we create there. We missed one. * Don't eagerly remove temp files there - they will be removed later anyhow. And we do need to keep alive the temp file that we return.

…emscripten-core#8643) We only use this in JS during startup (before compiled code is ready to run). Other usages should call malloc/sbrk directly for the full functionality. Fixes emscripten-core#8637

VirtualTim · 2019-05-23T06:40:01Z

Well I merged everything from Incoming into my PR, but unfortunately I did a rebase instead of a regular merge. I'm not sure how to fix this without creating more mess (or if it even needs fixing?).
What do you think @kripken? Is this mergable, or should I just delete this PR and re-create it?
At least the tests all passed...

kripken

Looks good, thanks, just some minor comments.

It's fine to leave things rebased (+ any more commits), whatever's easier - we'll squash it all when we land it anyhow, so the details of the commits in the PR don't matter.

kripken · 2019-05-23T21:27:17Z

emscripten.py

@@ -1699,6 +1699,10 @@ def create_receiving(function_table_data, function_tables_defs, exported_impleme

  shared.Settings.MODULE_EXPORTS = module_exports = exported_implemented_functions + function_tables(function_table_data)

+  # Should only need to do this in the main thread.


Please clarify this in the comment. I believe the issue is that when assertions are on, we emit code that is incompatible with ES6 currently? Can add a TODO in the comment to fix that.

Good point, I missed the comment what I changed the behaviour.

kripken · 2019-05-23T21:32:40Z

src/worker.js

@@ -35,6 +34,10 @@ var __performance_now_clock_drift = 0;
 // Cannot use console.log or console.error in a web worker, since that would risk a browser deadlock! https://bugzilla.mozilla.org/show_bug.cgi?id=1049091
 // Therefore implement custom logging facility for threads running in a worker, which queue the messages to main thread to print.
 var Module = {};
+#if EXPORT_ES6


i think we should also emit these lines if MODULARIZE as we do assign to them there as well (line 144/158), and for the same reason?

The difference is that in normal (aka sloppy) JS you can define things without var/const/let. So defining PThread like PThread = Module['PThread']; is perfectly valid. However this is not valid inside an ES6 module as that enforces strict mode. That's why I had to declare them here.
I can change #if EXPORT_ES6 to #if MODULARIZE if you want, but there may be a reason that it was initially done like that and I didn't want to break existing modularize code.

kripken · 2019-05-23T21:33:23Z

src/worker.js

@@ -149,6 +163,7 @@ this.onmessage = function(e) {
      if (typeof FS !== 'undefined' && typeof FS.createStandardStreams === 'function') FS.createStandardStreams();
 #endif
      postMessage({ cmd: 'loaded' });
+#endif


please add a comment // MODULARIZE && EXPORT_ES6 so it's easier to match up the if and its end

kripken · 2019-05-24T21:47:51Z

Sounds good. I'll do the modularize changes as a separate commit later, which is probably better anyhow.

I'm rerunning the chrome tests which all failed here - probably that's just bad luck, and the rerun will pass. I'll land this if so.

VirtualTim added 4 commits March 19, 2019 14:37

Fix ENVIRONMENT_IS_PTHREAD being undefined

b78e914

Fix document.currentScript in ES6 modules

0a3915a

See https://html.spec.whatwg.org/multipage/dom.html#dom-document-currentscript

Fix undefined PThread, HEAPU32

2849d36

Replace importScripts with import for ES Modules

0edabe6

Import returns a promise, so things had to be rearranged. I think I could have used promise.resolve() instead, but that's something to revisit if I can actually get this working.

VirtualTim mentioned this pull request Mar 19, 2019

EXPORT_ES6 doesn't work with USE_PTHREADS #8176

Closed

VirtualTim added 5 commits March 19, 2019 15:30

Add an EXPORT_ES6 ifdef

90a4b18

Fix emcc.py build issue

97b0dd2

Fix typo

90e2e59

Fix indentation

f9e04a3

Fix another typo

5a933df

curiousdannii reviewed Mar 20, 2019

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

VirtualTim added 3 commits March 21, 2019 09:42

Better way to determine script path inside module

03c96b8

Fix ENVIRONMENT_IS_PTHREAD check

5404d76

Remove old attempt at fixing ENVIRONMENT_IS_PTHREAD var hoisting

054f4ca

VirtualTim added 3 commits March 25, 2019 17:02

Don't declare functions in a thread that have already been declared i…

23bd473

…n the main thread. I feel like this should break things, but it seems to work properly with my tests.

Remove line with just whitespace

e0125dd

Put a space after my '#' to keep flake8 happy

48d2d77

VirtualTim mentioned this pull request Mar 26, 2019

Test case for multiple instantiation in modularize mode #8035

Closed

sbc100 reviewed Mar 26, 2019

View reviewed changes

VirtualTim added 3 commits April 2, 2019 13:51

Explicitly pass in ENVIRONMENT_IS_PTHREAD when creating a thread

4148743

Attach ENVIRONMENT_IS_PTHREAD to the module, instead of a global

dfd8440

Changed GETSCRIPT to script_url

a503811

VirtualTim marked this pull request as ready for review April 2, 2019 09:18

VirtualTim changed the title ~~[WIP] Attempt to fix EXPORT_ES6 + USE_PTHREADS~~ Fix EXPORT_ES6 + USE_PTHREADS Apr 3, 2019

kripken reviewed Apr 3, 2019

View reviewed changes

kripken and others added 19 commits May 21, 2019 09:35

Strip out trailing zeros from wasm mem init files. (emscripten-core#8607

311ec74

) This helps wasm2js and pthreads currently, as those modes use a mem file.

fix a missed #if for supporting OFFSCREEN_FRAMEBUFFER being off while…

d1aa9d0

… USE_PTHREADS is on (emscripten-core#8609)

[docs] Mention default directories in Filesystem-API.rst (emscripten-…

0f9ccc5

…core#8595)

Fix pthreads without OFFSCREEN_FRAMEBUFFER use of emscripten_webgl_ma…

507d879

…ke_context_current (emscripten-core#8613) And add a complete test for both that and for emscripten_webgl_create_context in that mode (which also tests emscripten-core#8579)

Update fetch API docs as streaming downloads are Firefox Only (emscri…

f8cee72

…pten-core#8618) * Update fetch API documentation because streaming is Firefox only * Added note about EMSCRIPTEN_FETCH_STREAM_DATA being Firefox only

Improve sanity.test_llvm. NFC. (emscripten-core#8626)

80f8c93

This test now passed with the llvm backend.

Improve error messages around missing llvm install (emscripten-core#8625

e1d3dfe

)

Adding proper fsync errors signaling for pipes. (emscripten-core#8631)

23cdcfc

Returning EINVAL error when trying to call fsync on fd representing UNIX pipe. This fixes http://posixtest.sourceforge.net/ fsync/7-1 test.

Fix the dodgy call_sync_on_main_thread tests (emscripten-core#8621)

15cda75

Previously these tests weren't actually checking the right thing. This should fix the logic to what I think the author was trying to do. Fixes emscripten-core#8620.

Revert "Adding proper fsync errors signaling for pipes. (emscripten-c…

79a1955

…ore#8631)" This reverts commit 07116f8.

Allow emscripten to use the minimal ubsan runtime (emscripten-core#8617)

82292b5

This works with the wasm backend, whether generating wasm or asm.js. Also added `emscripten_return_address` which implements the functionality of `__builtin_return_address` when running both wasm and asm.js.

1.38.32

3c9e831

Return detached threads to the pool (emscripten-core#8286)

eb37140

Once detached threads are finished their execution they emit the 'exit' command. Instead of a noop they should rejoin the pool. Resolves emscripten-core#8201.

Fix temp file handling in js_optimizer (emscripten-core#8642)

ad74b35

* Make sure to note temp files that we create there. We missed one. * Don't eagerly remove temp files there - they will be removed later anyhow. And we do need to keep alive the temp file that we return.

Don't try to handle complex things in dynamicAlloc, abort() on errors (…

a8c8706

…emscripten-core#8643) We only use this in JS during startup (before compiled code is ready to run). Other usages should call malloc/sbrk directly for the full functionality. Fixes emscripten-core#8637

Merge branch 'incoming' into patch-3

939695a

Remove accidentally added whitespace

79659e6

kripken reviewed May 23, 2019

View reviewed changes

Changed comment to reflect change in behaviour

6ab9a00

kripken merged commit 1e5317c into emscripten-core:incoming May 24, 2019

VirtualTim mentioned this pull request May 27, 2019

EXPORT_ES6 + USE_PTHREADS not compatible with ASSERTIONS #8678

Closed

VirtualTim deleted the patch-3 branch May 27, 2019 07:27

VirtualTim mentioned this pull request Jun 4, 2019

EXPORT_ES6 mode import.meta.url is not supported #8729

Closed

niklasf mentioned this pull request Jun 23, 2019

-s MODULARIZE=1 does not work with pthreads #7448

Closed

VirtualTim mentioned this pull request Aug 15, 2019

Regression bug: Relative import of ES6 WASM modules from ES6 Modules does not work #9233

Closed

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020

Fix EXPORT_ES6 + USE_PTHREADS (emscripten-core#8306)

3694d24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EXPORT_ES6 + USE_PTHREADS #8306

Fix EXPORT_ES6 + USE_PTHREADS #8306

VirtualTim commented Mar 19, 2019

VirtualTim commented Mar 20, 2019 •

edited

Loading

VirtualTim commented Mar 25, 2019 •

edited

Loading

VirtualTim commented Mar 26, 2019

sbc100 left a comment

sbc100 Mar 26, 2019

VirtualTim Mar 27, 2019

sbc100 Mar 26, 2019

VirtualTim Mar 27, 2019

kripken Apr 3, 2019

VirtualTim Apr 4, 2019 •

edited

Loading

kripken Apr 8, 2019

kripken May 14, 2019 •

edited

Loading

kripken May 14, 2019

VirtualTim May 15, 2019

kripken May 16, 2019

VirtualTim May 17, 2019

VirtualTim commented Apr 2, 2019

kripken Apr 3, 2019

kripken commented Apr 3, 2019

VirtualTim commented May 23, 2019

kripken left a comment

kripken May 23, 2019

VirtualTim May 24, 2019

kripken May 23, 2019

VirtualTim May 24, 2019

kripken May 23, 2019

kripken commented May 24, 2019

		@@ -1699,6 +1699,10 @@ def create_receiving(function_table_data, function_tables_defs, exported_impleme

		shared.Settings.MODULE_EXPORTS = module_exports = exported_implemented_functions + function_tables(function_table_data)

		# Should only need to do this in the main thread.

Fix EXPORT_ES6 + USE_PTHREADS #8306

Fix EXPORT_ES6 + USE_PTHREADS #8306

Conversation

VirtualTim commented Mar 19, 2019

VirtualTim commented Mar 20, 2019 • edited Loading

VirtualTim commented Mar 25, 2019 • edited Loading

VirtualTim commented Mar 26, 2019

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VirtualTim Apr 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VirtualTim commented Apr 2, 2019

Choose a reason for hiding this comment

kripken commented Apr 3, 2019

VirtualTim commented May 23, 2019

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented May 24, 2019

VirtualTim commented Mar 20, 2019 •

edited

Loading

VirtualTim commented Mar 25, 2019 •

edited

Loading

VirtualTim Apr 4, 2019 •

edited

Loading

kripken May 14, 2019 •

edited

Loading