WASI: add args_get and args_sizes_get #308
Conversation
src/wasi/WASI.cpp
Outdated
| uvwasi_size_t bufSize; | ||
| uvwasi_args_sizes_get(WASI::g_uvwasi, &argc, &bufSize); | ||
|
|
||
| char** uvArgv = reinterpret_cast<char**>(get_memory_pointer(instance, argv[0], argc * sizeof(char*))); |
There was a problem hiding this comment.
I don't think this is correct. char* pointers can be 32 or 64 bit long depending on the cpu, while the output is 32 bit long offsets.
There was a problem hiding this comment.
I have created a TemporaryData class in the #307 patch which should help.
matetokodi
force-pushed
the
wasi_args
branch
from
6c6123d
to
6637b32
Compare
|
Please rebase this patch |
matetokodi
force-pushed
the
wasi_args
branch
from
6637b32
to
ed91832
Compare
src/shell/Shell.cpp
Outdated
| @@ -1117,7 +1128,7 @@ static void parseArguments(int argc, char* argv[], ParseOptions& options) | |||
| } | |||
| } | |||
|
|
|||
| int main(int argc, char* argv[]) | |||
| int main(const int argc, const char* argv[]) | |||
There was a problem hiding this comment.
for an int const is pointless.
src/wasi/WASI.cpp
Outdated
| @@ -242,6 +242,42 @@ void WASI::random_get(ExecutionState& state, Value* argv, Value* result, Instanc | |||
| result[0] = Value(uvwasi_random_get(WASI::g_uvwasi, buf, length)); | |||
| } | |||
|
|
|||
| void WASI::args_get(ExecutionState& state, Value* argv, Value* result, Instance* instance) | |||
There was a problem hiding this comment.
I would prefer to follow the uvwasi order of API functions for these wasi functions, so put it above, not to the last place.
There was a problem hiding this comment.
As far as I can tell uvwasi sorts the functions alphabetically; But the order we have them in already is not alphabetical. Would you want me to sort the existing ones as well, or to just put the new args functions at the top?
src/wasi/WASI.h
Outdated
| @@ -47,7 +47,9 @@ class WASI { | |||
| F(fd_seek, I32I64I32I32_RI32) \ | |||
| F(path_open, I32I32I32I32I32I64I64I32I32_RI32) \ | |||
| F(environ_get, I32I32_RI32) \ | |||
| F(environ_sizes_get, I32I32_RI32) | |||
| F(environ_sizes_get, I32I32_RI32) \ | |||
| F(args_get, I32I32_RI32) \ | |||
| fails = 0 | ||
| for file in files: | ||
| if jit: | ||
| filename = os.path.basename(file) | ||
| if filename in JIT_EXCLUDE_FILES: | ||
| continue | ||
|
|
||
| proc = Popen([engine, "--mapdirs", "./test/wasi", "/var", file], stdout=PIPE) if not jit else Popen([engine, "--mapdirs", "./test/wasi", "/var", "--jit", file], stdout=PIPE) | ||
| subprocess_args = [engine, "--mapdirs", "./test/wasi", "/var"] |
There was a problem hiding this comment.
Adding expected output is a great thing, I like it very much! Most wasi test results are not really checked, and this would improve them a lot. However, I would do it in a wabt way:
https://github.com/WebAssembly/wabt/blob/main/test/interp/br.txt#L99
(;; STDOUT ;;;
;;; STDOUT ;;)
(; and ;) are WebAssembly comments, so these parts are ignored. However, it is easy to find them with a regex, and the output can be compared with it.
There was a problem hiding this comment.
I realized this is a bad idea because stdout is cluttered with debug output.
Improve other WASI functions as well. Signed-off-by: Zoltan Herczeg [email protected]
Update the test script to test passing the arguments as well Signed-off-by: Máté Tokodi [email protected]
| @@ -136,6 +145,24 @@ def run_wasi_tests(engine): | |||
| if fail_total > 0: | |||
| raise Exception("basic wasi tests failed") | |||
|
|
|||
| @runner('wasi-args', default=True) | |||
There was a problem hiding this comment.
What about merging this wasi-args runner into wasi test runner?
|
#309 should land first. |
WASI: add args_get and args_sizes_get
Extend testing script for passing arguments and checking stdout.
Depends on #307 and #309