Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix EXPORT_ES6 + USE_PTHREADS #8306
Fix EXPORT_ES6 + USE_PTHREADS #8306
Changes from 18 commits
b78e914
0a3915a
2849d36
0edabe6
90a4b18
97b0dd2
90e2e59
f9e04a3
5a933df
03c96b8
5404d76
054f4ca
23bd473
e0125dd
48d2d77
4148743
dfd8440
a503811
de25547
208daf5
8b86f6e
9c53450
c25c5dd
970a22c
0583f79
e5584ad
c184ae3
3715670
36e9a41
a1e7f6f
261c291
d1a9fc3
6af55a6
d3fd009
1a22f4c
f54f621
e6b1f3e
3c178bf
3bd09f6
745c31b
69c2e90
9c0fccb
03c2739
65544da
2b2ab6b
08105ed
b8717e4
36b94e4
bcb5474
009c8f6
04a83ff
fabdd2a
c7323da
b56a8c7
420b5e9
29a7afa
dd29b3b
f4d4d8b
18ebc84
8204c0e
9e837aa
a291091
1423e30
c4b5e3d
5c25fc5
15f323a
586dd66
c3ba604
4772328
9e2efb0
fc3dbcb
f666f9f
dc90308
a8164ac
d0a43bf
8504599
bcf4f5f
4309b0c
c2bb415
8623572
178174f
9b79ed1
3a2c80b
e60da12
69a35b4
7ebca1b
7e73471
f05c3d0
fb9b1dd
829a4db
8725504
12a5ae5
dd50abc
a13f12d
0439bc1
a365dba
629390f
645bdcc
6ceb773
28606af
32d409f
cd07644
45427af
6ace2e9
375e65a
44b7f4a
be1e99f
7bb4aff
38b0ea3
32d5af6
9d58245
34cebc7
6f080ae
2f6e69f
a795155
60210f7
c805f33
57395a9
10ba2cb
e92a55f
a89389d
d6b1d39
38882fa
8d3b178
5e27ed1
4a9a69a
da96266
4a1ab9e
db9d9dc
a4eea28
117ae30
c25f8bc
c814f17
618caaa
4099f5b
aed6acd
7ca7823
567697e
7d65c0c
f6a0888
6cce904
2dd1b7b
cd4f62e
5d7247b
b8d2bf1
3d856e2
fa0074e
0d0a3dd
c51f715
50b2dfe
2641ea8
dc54a7e
b9a594b
3c17301
5a3d982
83bca46
2398daa
f054894
91b6972
ef686fb
0fd80a5
1a85480
ae9ddc4
5521ad9
87770d1
02f7e58
10ae6e2
311ec74
d1aa9d0
0f9ccc5
507d879
9fe43d6
4aa04c5
f8cee72
80f8c93
e1d3dfe
23cdcfc
15cda75
79a1955
82292b5
3c9e831
eb37140
ad74b35
a8c8706
939695a
79659e6
6ab9a00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
Good point, I missed the comment what I changed the behaviour.
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.
But the stuff added to receiving below we do want on non-main-threads? Can you explain, perhaps write more in the comment 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.
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.
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.
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?
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.
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.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.
That's a very odd error - I don't think we use read-only properties anywhere?
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.
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?
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.
Regardless, we can split this out from the rest of this PR, if it's separable.
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 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.
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, let's separate it out - a comment that says something like your last line there sounds good enough for now.
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, 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.
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.
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?
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.
The difference is that in normal (aka sloppy) JS you can define things without var/const/let. So defining
PThread
likePThread = 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.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.
please add a comment
// MODULARIZE && EXPORT_ES6
so it's easier to match up the if and its end