add all wasi hostcalls used by Go SDK #433
Conversation
johnlanni
requested review from
PiotrSikora,
martijneken and
mpwarres
as code owners
|
Thank you for sending this PR! I will take a look at the code a bit later today, but I was wondering if any of the wasm plugins importing these host calls are public so I can take a look. I'd be interested to see what logic causes the inclusion of them. |
There was a problem hiding this comment.
LGTM. @PiotrSikora do you have any reservations against implementing the rest of the wasip1 surface?
Sorry for the late reply. Here are some examples of plugin code that has additional WASI dependencies. I've been encountering new WASI dependencies with different plugins, so I included all of them: Convert to wat and see the dependencies on some previously unsupported wasi: |
|
@leonm1 There are two more important changes:
|
| Word wasi_unstable_sock_accept(Word, Word, Word); | ||
| Word wasi_unstable_sock_recv(Word, Word, Word, Word, Word, Word); | ||
| Word wasi_unstable_sock_send(Word, Word, Word, Word, Word); | ||
| Word wasi_unstable_sock_shutdown(Word, Word); |
There was a problem hiding this comment.
Presumably, those socket functions are not "randomly" imported by the Go runtime, and if we add stubs for them, then we're replacing load time failure with a runtime failure.
There is a difference between existing functions (bare minimum required by runtimes to load targets compiled to a WASI architecture) vs adding everything without implementing it.
Why are those needed?
There was a problem hiding this comment.
The Go dependencies might include these, but they are not actually executed in the plugin logic. However, without these stubs, the plugin cannot be loaded correctly.
include/proxy-wasm/exports.h
Outdated
| Word wasi_unstable_random_get(Word, Word); | ||
| Word pthread_equal(Word left, Word right); | ||
| void emscripten_notify_memory_growth(Word); | ||
| Word wasi_unstable_path_filestat_get(Word fd, Word flags, Word path, Word path_len, Word buf); |
There was a problem hiding this comment.
Nit: this should be grouped with other wasi_unstable_path_* functions.
src/exports.cc
Outdated
| // Set the number of bytes read to 0 | ||
| if (!context->wasmVm()->setWord(retptr0, Word(0))) { | ||
| return 21; // __WASI_EFAULT | ||
| } |
There was a problem hiding this comment.
The general convention is that return values aren't expected to be legitimate if the function returns error, so this shouldn't be needed.
src/exports.cc
Outdated
| // fd_datasync is used to synchronize the data of a file to disk. | ||
| // Since we don't have a real file system in proxy-wasm, we can just return success for stdout/stderr | ||
| // and an error for other file descriptors. | ||
|
|
||
| // We only support stdout and stderr in proxy-wasm | ||
| if (fd != 1 /* stdout */ && fd != 2 /* stderr */) { | ||
| return 8; // __WASI_ERRNO_BADF - Bad file descriptor | ||
| } | ||
|
|
||
| // For stdout and stderr, there's no need to sync as they're handled by the host system | ||
| return 0; // __WASI_ESUCCESS |
There was a problem hiding this comment.
This code is repeated a lot, but are those function actually called for stdout and stderr or is it simply copy & paste? Some of those would be rather surprising.
|
In my example use cases here - https://github.com/shukitchan/ats-wasm-ai-inference/ |
Signed-off-by: zty98751 <[email protected]> Signed-off-by: johnlanni <[email protected]>
Signed-off-by: johnlanni <[email protected]>
johnlanni
force-pushed
the
add-all-wasi-hostcalls
branch
from
e94334a to
f3d5c5a
Compare
|
Thanks for the updates @johnlanni ! It looks like the current presubmit check failures are due to the Github action runners exhausting their allocated disk space. I am looking into obtaining more storage for those. |
In #427, a portion of the wasi hostcalls was added, but not all. Now, all the wasi hostcalls have been included, and their stability has been verified in our multiple go 1.24 compiled wasm plugins