Skip to content

Always assert on calling proxied function from wasm worker. NFC #20343

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

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

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Sep 27, 2023

Follow up to #20135.

See #20135 (comment)

Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

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

Looks good. This will catch more of the missing proxy functionality.

@@ -3769,7 +3769,7 @@ function wrapSyscallFunction(x, library, isWasi) {
if (!WASMFS) {
library[x + '__deps'].push('$SYSCALLS');
}
#if PTHREADS
#if SHARED_MEMORY
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this #if could be dropped altogether? The proxying directives can exist independent of the build configuration one is running with, and don't need to be gated to only be specified in multithreaded builds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Done.

@sbc100 sbc100 force-pushed the wasm_worker_proxy_asserts branch from aaae986 to cd8d638 Compare September 27, 2023 19:11
@sbc100 sbc100 force-pushed the wasm_worker_proxy_asserts branch from cd8d638 to 3dbef5a Compare October 2, 2023 18:28
@sbc100 sbc100 closed this Jan 3, 2025
@sbc100 sbc100 deleted the wasm_worker_proxy_asserts branch January 3, 2025 22:45
@sbc100 sbc100 restored the wasm_worker_proxy_asserts branch January 3, 2025 23:02
@sbc100 sbc100 reopened this Jan 3, 2025
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.

2 participants