Remove unnecessary export of emscripten_futex_wake. NFC#15767
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 |
kripken
left a comment
There was a problem hiding this comment.
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.