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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/jsifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ if (ENVIRONMENT_IS_PTHREAD)
${body}
}\n`
});
} else if (WASM_WORKERS && ASSERTIONS) {
}
if (WASM_WORKERS && ASSERTIONS) {
// In ASSERTIONS builds add runtime checks that proxied functions are not attempted to be called in Wasm Workers
// (since there is no automatic proxying architecture available)
snippet = modifyJSFunction(snippet, (args, body) => `
Expand Down
6 changes: 5 additions & 1 deletion src/library.js
Original file line number Diff line number Diff line change
Expand Up @@ -3794,7 +3794,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.

// Most syscalls need to happen on the main JS thread (e.g. because the
// filesystem is in JS and on that thread). Proxy synchronously to there.
// There are some exceptions, syscalls that we know are ok to just run in
Expand All @@ -3804,6 +3804,10 @@ function wrapSyscallFunction(x, library, isWasi) {
// instead of synchronously, and marked with
// __proxy: 'async'
// (but essentially all syscalls do have return values).
//
// Note that we add this even in the WASM_WORKERS case since we want calls
// to these functions from wasm workers to assert in debug builds (since wasm
// workers lack proxying support).
if (library[x + '__proxy'] === undefined) {
library[x + '__proxy'] = 'sync';
}
Expand Down
6 changes: 6 additions & 0 deletions src/library_wasi.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,11 @@ var WasiLibrary = {
},
fd_write__deps: ['$flush_NO_FILESYSTEM', '$printChar'],
fd_write__postset: () => addAtExit('flush_NO_FILESYSTEM()'),
// Avoid proxying when we are not using the filesystem
fd_write__proxy: 'none',
#else
fd_write__deps: ['$printChar'],
fd_write__proxy: 'none',
#endif
fd_write: (fd, iov, iovcnt, pnum) => {
#if SYSCALLS_REQUIRE_FILESYSTEM
Expand Down Expand Up @@ -305,6 +308,9 @@ var WasiLibrary = {
#endif
},

#if !SYSCALLS_REQUIRE_FILESYSTEM
fd_close__sync: 'none',
#endif
fd_close: (fd) => {
#if SYSCALLS_REQUIRE_FILESYSTEM
var stream = SYSCALLS.getStreamFromFD(fd);
Expand Down