Skip to content
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 emscripten_current_thread_is_wasm_worker return value. #23484

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 23, 2025

This function return ENVIRONMENT_IS_WASM_WORKER which was being initialized to Module['$ww'], i.e. the worker id.

Because emscripten_current_thread_is_wasm_worker, is defined to return the bool type its only valid values are 0 and 1. Returning values larger than 1 will result in undefined behaviour.

Fixes: #23479

@@ -122,7 +122,7 @@ if (ENVIRONMENT_IS_NODE) {
#endif // ENVIRONMENT_MAY_BE_NODE

#if WASM_WORKERS
var ENVIRONMENT_IS_WASM_WORKER = Module['$ww'];
var ENVIRONMENT_IS_WASM_WORKER = !!Module['$ww'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increases code size - is it necessary? Can we not fix the C method on the C side? (if not, perhaps rephrase the PR title/description)

Copy link
Collaborator Author

@sbc100 sbc100 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C function is declared as returning bool which allows the compiler assume its going be be either a 1 or 0. Anything else is undefined behaviour.

We could change the return type to int, but this fix seems better, since it also means that the ENVIRONMENT_IS_WASM_WORKER is only ever true or false which is is what its name suggests. Storing the thread in this variable doesn't make much sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not following. Perhaps my confusion is about Module['$ww']. I cannot find where in the code that is ever written to. Where is that done?

From the context of what you are saying, I am guessing that the output of emscripten_current_thread_is_wasm_worker() is written there. (Otherwise, how does this line connect to the topic of the PR?) Assuming my guess is right, then it should already be either 0 or 1, since it is a bool in C?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to connect the docs in the PR description but perhaps more context is needed.

Module['$ww'] is the wasm workers ID. As each new worker is created its gets its own unique ID in Module['$ww'].

ENVIRONMENT_IS_WASM_WORKER was previously initialized to Module['$ww'] which means its value would laos be the worker ID. This works in most cases because zero is falsely and all other IDs would be truthy.

emscripten_current_thread_is_wasm_worker is the JS library that we expose to native code which is the equivalent of checking for ENVIRONMENT_IS_WASM_WORKER. However in C/C++ its defined as returning a bool which must be either 1 or 0.

@sbc100 sbc100 force-pushed the emscripten_current_thread_is_wasm_worker branch from d2888af to 71bae43 Compare January 23, 2025 23:46
This function was ENVIRONMENT_IS_WASM_WORKER which was being initialized
to Module['$ww'], i.e. the worker id.

Because emscripten_current_thread_is_wasm_worker, is defined to return
the `bool` type its only valid values are 0 and 1.  Returning values
larger than 1 will result in undefined behaviour.

Fixes: emscripten-core#23479
@sbc100 sbc100 force-pushed the emscripten_current_thread_is_wasm_worker branch from 71bae43 to aa00567 Compare January 23, 2025 23:48
@sbc100 sbc100 requested a review from kripken January 24, 2025 18:07
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.

-O0 + js_library + int to boolean = error
2 participants