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

Add API for disabling native stack boundary checks #4060

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

Conversation

loganek
Copy link
Collaborator

@loganek loganek commented Jan 31, 2025

This API can be helpful if e.g. module is working in a reactor mode, and the module is being used from multiple threads (where the caller guarantees that no two threads execute code on the same execution environment). Currently this usecase fails because the native stack boundaries for each thread are different.

Alternative to this solution would be to update the boundary on every thread switch but that might not be desired due to performance.

I've added a new API but just having a logic for UINTPTR_MAX and using wasm_runtime_set_native_stack_boundary would be good enough. If the preference is to not have a new API, I'm ok to remove it and add a comment in the wasm_runtime_set_native_stack_boundary about the special UINTPTR_MAX value.

@loganek loganek force-pushed the loganek/no-native-stack-checks branch 2 times, most recently from 78063f5 to e6c729b Compare January 31, 2025 18:55
Copy link
Collaborator

@lum1n0us lum1n0us left a comment

Choose a reason for hiding this comment

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

LGTM w/ minor comments.

Update the comments for user_native_stack_boundary in the WASMExecEnv in wasm_exec_env.h to reflect the specific value accurately.

This API can be helpful if e.g. module is working in a reactor mode, and
the module is being used from multiple threads (where the caller guarantees
that no two threads execute code on the same execution environment).
Currently this usecase fails because the native stack boundaries for each
thread are different.
Alternative to this solution would be to update the boundary on every thread
switch but that might not be desired due to performance.
@loganek loganek force-pushed the loganek/no-native-stack-checks branch from e6c729b to b676522 Compare February 4, 2025 07:44
@yamt
Copy link
Collaborator

yamt commented Feb 5, 2025

This API can be helpful if e.g. module is working in a reactor mode, and the module is being used from multiple threads (where the caller guarantees that no two threads execute code on the same execution environment). Currently this usecase fails because the native stack boundaries for each thread are different.

what you really want is an api to stop associating exec env to a host thread, right?
native stack boundary check is merely an example of possible functionalities which might rely on the association to a host thread.

Alternative to this solution would be to update the boundary on every thread switch but that might not be desired due to performance.

have you measured the performance?

@loganek
Copy link
Collaborator Author

loganek commented Feb 5, 2025

what you really want is an api to stop associating exec env to a host thread, right?
native stack boundary check is merely an example of possible functionalities which might rely on the association to a host thread.

Yes, indeed. And I agree best way to do it by not tying the exec env to the thread. This is however a bit of extra work that we might do later but the native boundary check is currently the only blocker (from my usecase perspective) to apply a workaround.

The proposed change is generic enough and not strictly for my own usecase, so even though there are better ways to solve my particular problem, it shouldn't impact the WAMR architecture.

@yamt
Copy link
Collaborator

yamt commented Feb 6, 2025

what you really want is an api to stop associating exec env to a host thread, right?
native stack boundary check is merely an example of possible functionalities which might rely on the association to a host thread.

Yes, indeed. And I agree best way to do it by not tying the exec env to the thread. This is however a bit of extra work that we might do later but the native boundary check is currently the only blocker (from my usecase perspective) to apply a workaround.

The proposed change is generic enough and not strictly for my own usecase, so even though there are better ways to solve my particular problem, it shouldn't impact the WAMR architecture.

by providing this api for this purpose, we effectively give a promise "disabling native stack boundary check via this api makes it safe to switch calling threads" to the api users, don't we?

your alternative solution ("update the boundary on every thread switch") looks simpler and more straightforward to me.

@loganek
Copy link
Collaborator Author

loganek commented Feb 6, 2025

by providing this api for this purpose, we effectively give a promise "disabling native stack boundary check via this api makes it safe to switch calling threads" to the api users, don't we?

No, we should never officially state that by doing it users are enabled to switch calling threads, even though that might be the case. If users choose to do that, they do it at their own risk; the API's intention is to disable the native stack boundary checks, no more than that.

your alternative solution ("update the boundary on every thread switch") looks simpler and more straightforward to me.

Please note that this also needs a new API; also I think that more explicitly tells user that the API can be used for switching calling threads than what we have in this PR (because otherwise I think there's very few usecases for changing the stack boundary during a single-threaded execution).

@yamt
Copy link
Collaborator

yamt commented Feb 7, 2025

by providing this api for this purpose, we effectively give a promise "disabling native stack boundary check via this api makes it safe to switch calling threads" to the api users, don't we?

No, we should never officially state that by doing it users are enabled to switch calling threads, even though that might be the case. If users choose to do that, they do it at their own risk; the API's intention is to disable the native stack boundary checks, no more than that.

well, in that case, i'm not sure why a user would want to disable the native stack boundary check.

your alternative solution ("update the boundary on every thread switch") looks simpler and more straightforward to me.

Please note that this also needs a new API; also I think that more explicitly tells user that the API can be used for switching calling threads than what we have in this PR (because otherwise I think there's very few usecases for changing the stack boundary during a single-threaded execution).

yes. maybe a "disassociate from the calling thread" api.

@loganek
Copy link
Collaborator Author

loganek commented Feb 7, 2025

by providing this api for this purpose, we effectively give a promise "disabling native stack boundary check via this api makes it safe to switch calling threads" to the api users, don't we?

No, we should never officially state that by doing it users are enabled to switch calling threads, even though that might be the case. If users choose to do that, they do it at their own risk; the API's intention is to disable the native stack boundary checks, no more than that.

well, in that case, i'm not sure why a user would want to disable the native stack boundary check.

your alternative solution ("update the boundary on every thread switch") looks simpler and more straightforward to me.

Please note that this also needs a new API; also I think that more explicitly tells user that the API can be used for switching calling threads than what we have in this PR (because otherwise I think there's very few usecases for changing the stack boundary during a single-threaded execution).

yes. maybe a "disassociate from the calling thread" api.

Yes, that's a completely different API and would need to do more than just dealing with boundary checks. Having said that, Setting the stack boundary to 0x01 (which is possible today) is good enough for me so if there's a strong objection for adding an option to disable the stack boundary checks, can don't mind closing the PR.

@lum1n0us
Copy link
Collaborator

@yamt @wenyongh @TianlongLiang @xujuntwt95329 Please let us know your thoughts. Should we proceed with merging or closing the pull request?

@yamt
Copy link
Collaborator

yamt commented Feb 17, 2025

@yamt @wenyongh @TianlongLiang @xujuntwt95329 Please let us know your thoughts. Should we proceed with merging or closing the pull request?

i prefer not to merge this as there seems to be no real use cases.

what the author wants to do is kinda abuse of the new proposed api.
also, the author has another way to achieve his goal as he said. (wasm_runtime_set_native_stack_boundary)

@xujuntwt95329
Copy link
Collaborator

The use case @loganek mentioned is very common, the introduced API in this PR is an interesting approach to meet the requirement.
However, I agree with @yamt that the API itself doesn't explicitly explain its real use case, this may cause confusion. So in my opinion, I think it's not reasonable to add this API. If we want to explicitly detach a thread environment, we had better design a dedicated API for it.

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.

4 participants