Skip to content

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

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 13, 2021

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.

@sbc100 sbc100 requested review from kleisauke and kripken December 13, 2021 21:35
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 13, 2021

cc @VirtualTim who added this assert in the first place.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 13, 2021

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.

@sbc100 sbc100 force-pushed the remove_unneeded_export branch from e636ca1 to 5a87c37 Compare December 13, 2021 21:40
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.
@sbc100 sbc100 force-pushed the remove_unneeded_export branch from 5a87c37 to eca7fd8 Compare December 13, 2021 22:32
@kripken
Copy link
Member

kripken commented Dec 14, 2021

See #8481 where this was added. I believe the issue is not specific to emscripten_futex_wake - that is just a function selected to notice the issue. What the check does is try to improve the error message: if we threw, and we failed to create the wasm module, then we can say that we failed there instead of just showing the first "is not a function" error, was the idea in that PR.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2021

See #8481 where this was added. I believe the issue is not specific to emscripten_futex_wake - that is just a function selected to notice the issue. What the check does is try to improve the error message: if we threw, and we failed to create the wasm module, then we can say that we failed there instead of just showing the first "is not a function" error, was the idea in that PR.

Right, but if you look at #8481 you can see that the line below is a call to emscripten_futex_wake .. so emscripten_futex_wake was not selected at random and certainly doesn't seem like it should be used today given that this function is not needed/used anywhere in JS code these days.

Regarding the if we threw, and we failed to create the wasm module, then we can say that we failed there instead of just showing the first "is not a function" comment: This entire try catch comes after the usage of the instantiated module so there is just no way we can reach this try/catch at all if we failed to instantiate the module.

For example this call is made before entering the try/catch:

Module['__emscripten_thread_init'](e.data.threadInfoStruct, /*isMainBrowserThread=*/0, /*isMainRuntimeThread=*/0, /*canBlock=*/1);

Copy link
Member

@kripken kripken left a 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.

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 14, 2021

AFAICT even when this code landed there was a native call to __register_pthread_ptr before the try catch.. so I don't see how it could ever have fired.

Also, I can see how run could ever be called without the wasm module first being instantiated correctly since the run message is called only in repsonse to the worker sending the 'loaded message.

@sbc100 sbc100 merged commit df68a1f into main Dec 14, 2021
@sbc100 sbc100 deleted the remove_unneeded_export branch December 14, 2021 20:30
@VirtualTim
Copy link
Collaborator

I think this change is doing the right thing. As @sbc100 said this change was about checking for _emscripten_futex_wake before calling it.

The original (fix in #8481) issue was that if a thread threw during initialisation _emscripten_futex_wake would not be available. So the original exception would be clobbered by a different exception like Module._emscripten_futex_wake is not a function.

The code has changed a lot since then, so the _emscripten_futex_wake is not longer necessary.
That being said, it may still be possible for Module['invokeEntryPoint'](e.data.start_routine, e.data.arg) to throw, and if Module['keepRuntimeAlive'] is not defined that could clobber the error in a similar way. But I don't know if that's possible with the current code.

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 Module._emscripten_futex_wake is not a function.

So TLDR: LGTM.

mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022
…-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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants