Add wasm_runtime_get_wasi_exit_code #1748
Conversation
|
i plan to use this for WebAssembly/wasi-testsuite#16 |
| char **argv_list; | ||
| char *env_buf; | ||
| char **env_list; | ||
| uint32_t exit_code; |
There was a problem hiding this comment.
I don't see where we initialize exit_code; is there something like memset(ctx, 0, sizeof(WASIContext)) in the startup code to set the value of exit_code to 0? Otherwise it'll contain garbage if the module doesn't call proc_exit.
There was a problem hiding this comment.
it's allocated with runtime_malloc, which does memset.
| typedef struct WASIContext { | ||
| uvwasi_t uvwasi; | ||
| uint32_t exit_code; | ||
| } WASIContext; |
There was a problem hiding this comment.
That's probably out of scope of this PR as it needs a bit larger refactoring, but I think we should strive to have one declaration per context, instead of duplicating it. I've opened an issue to track it: #1749
There was a problem hiding this comment.
i tend to agree. but it's how this code base is currently structured.
| WASM_RUNTIME_API_EXTERN WASMFunctionInstanceCommon * | ||
| wasm_runtime_lookup_wasi_start_function(WASMModuleInstanceCommon *module_inst); | ||
|
|
||
| /* See wasm_export.h for description */ |
There was a problem hiding this comment.
Could you please add documentation for this function in wasm_export.h?
| #if WASM_ENABLE_LIBC_WASI != 0 | ||
| if (ret == 0) { | ||
| /* propagate wasi exit code. */ | ||
| ret = wasm_runtime_get_wasi_exit_code(wasm_module_inst); |
There was a problem hiding this comment.
Had better also apply similar changes to windows main.c?
There was a problem hiding this comment.
done. i haven't tested it locally at all though.
| WASM_RUNTIME_API_EXTERN wasm_function_inst_t | ||
| wasm_runtime_lookup_wasi_start_function(wasm_module_inst_t module_inst); | ||
|
|
||
| WASM_RUNTIME_API_EXTERN uint32_t |
Preserve wasi exit code so that an embedder can query it.
…t code It seems like a common practice for engines with a cli. Also, use non-zero exit code on an exeception.
| } | ||
|
|
||
| ret = 0; | ||
| #if WASM_ENABLE_LIBC_WASI != 0 |
There was a problem hiding this comment.
Just wanted to doublecheck - won't that break any user scenario right now? I guess many scripts assumed the return code is ignored by iwasm, so with the update it's possible this breaks some of the usecases. Just want to highlight we have to definitely include it in the release notes and probably highlight as backward incompatible change - any thoughts @wenyongh @yamt ?
There was a problem hiding this comment.
Seems that it won't break any user scenarios for us - normally we only use iwasm to run spec cases and standalone cases. Agree to highlight the change in the release notes. And not sure whether it is very important to the users? iwasm just gives a sample, the host embedders can embed WAMR into their application and decide how to handle the exit code.
| } | ||
|
|
||
| ret = 0; | ||
| #if WASM_ENABLE_LIBC_WASI != 0 |
There was a problem hiding this comment.
Seems that it won't break any user scenarios for us - normally we only use iwasm to run spec cases and standalone cases. Agree to highlight the change in the release notes. And not sure whether it is very important to the users? iwasm just gives a sample, the host embedders can embed WAMR into their application and decide how to handle the exit code.
3 participants
#1738