-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Remove unnecessary export of emscripten_futex_wake
. NFC
#15767
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
Conversation
cc @VirtualTim who added this assert in the first place. |
The assertion was also added a time when there was a call to emscripten_futex_wake directly after it.. so maybe it made more sense back then. |
e636ca1
to
5a87c37
Compare
This was being exported solely for the benefit of an assertions that I don't think served any purpose. Its not clear exactly what this assertion was trying to check for but I think it was checking for case where the Module object was not initialized on the thread. However before this try/catch is even executed we have already run several native methods. For example, see `Module['__emscripten_thread_init']` just above. For this reason I think removing the asserting and the extra export is the correct action.
5a87c37
to
eca7fd8
Compare
See #8481 where this was added. I believe the issue is not specific to |
Right, but if you look at #8481 you can see that the line below is a call to Regarding the For example this call is made before entering the try/catch: Line 191 in ea42b32
|
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.
Ah, sgtm, if there is something else that crashes before that try-catch in the way the old code tried to handle then I guess we don't need that old code. It seems hard to keep code like that up to date without a test, which it lacked. Anyhow if this is an issue we can look into restoring it perhaps.
AFAICT even when this code landed there was a native call to Also, I can see how |
I think this change is doing the right thing. As @sbc100 said this change was about checking for The original (fix in #8481) issue was that if a thread threw during initialisation The code has changed a lot since then, so the _emscripten_futex_wake is not longer necessary. For additional context, this was never something I was seeing during regular use, it was just to help me develop Emscripten. If I broke something it was nice to knwo what I broke, instead of just seeing So TLDR: LGTM. |
…-core#15767) This was being exported solely for the benefit of an assertions that I don't think served any purpose. Its not clear exactly what this assertion was trying to check for but I think it was checking for case where the Module object was not initialized on the thread. However before this try/catch is even executed we have already run several native methods. For example, see `Module['__emscripten_thread_init']` just above. For this reason I think removing the asserting and the extra export is the correct action.
This was being exported solely for the benefit of an assertions that I
don't think served any purpose.
Its not clear exactly what this assertion was trying to check for but I
think it was checking for case where the Module object was not
initialized on the thread. However before this try/catch is even
executed we have already run several native methods. For example, see
Module['__emscripten_thread_init']
just above.For this reason I think removing the asserting and the extra export is
the correct action.