Skip to content
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.

Sign up for GitHub

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

Jump to bottom

WASI: add args_get and args_sizes_get #308

Merged
merged 1 commit into from
Dec 10, 2024
Merged

WASI: add args_get and args_sizes_get #308

merged 1 commit into from
Dec 10, 2024

Conversation

matetokodi
Copy link
Contributor

@matetokodi matetokodi commented Nov 21, 2024 •
edited
Loading

WASI: add args_get and args_sizes_get
Extend testing script for passing arguments.

Depends on #307 and #309

zherczeg
zherczeg reviewed Nov 22, 2024
View reviewed changes
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*)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have created a TemporaryData class in the #307 patch which should help.

@zherczeg zherczeg mentioned this pull request Nov 25, 2024
Merged
@clover2123
Copy link
Collaborator

Please rebase this patch

zherczeg
zherczeg requested changes Nov 27, 2024
View reviewed changes
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[])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to follow the uvwasi order of API functions for these wasi functions, so put it above, not to the last place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Order here as well.

tools/run-tests.py Show resolved Hide resolved
@matetokodi matetokodi force-pushed the wasi_args branch 2 times, most recently from c824015 to e9a6258 Compare December 2, 2024 13:58
clover2123
clover2123 reviewed Dec 3, 2024
View reviewed changes
tools/run-tests.py Outdated
@@ -136,6 +145,24 @@ def run_wasi_tests(engine):
if fail_total > 0:
raise Exception("basic wasi tests failed")

@runner('wasi-args', default=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about merging this wasi-args runner into wasi test runner?

@zherczeg
Copy link
Collaborator

zherczeg commented Dec 3, 2024

#309 should land first.

@matetokodi matetokodi force-pushed the wasi_args branch 2 times, most recently from acc9e86 to 0271239 Compare December 9, 2024 10:03
zherczeg
zherczeg requested changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

Nice patch!

src/shell/Shell.cpp Outdated
@@ -1083,6 +1084,15 @@ static void parseArguments(int argc, char* argv[], ParseOptions& options)
options.wasi_dirs.push_back(std::make_pair(argv[i + 2], argv[i + 1]));
i += 2;
continue;
} else if (strcmp(argv[i], "--args") == 0) {
if (i + 1 == argc || argv[i + 1][0] == '-') {
fprintf(stderr, "error: --args requires an argument\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment: requires an argument list, or requires one or more arguments

test/wasi/args.wast Outdated
local.tee $argc
i32.const 4
call $wasi_args_sizes_get
drop
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we throw an error on nonzero return code? If you pass a buffer outside the memory area, it should throw error.

test/wasi/args.wast Outdated
(import "wasi_snapshot_preview1" "args_sizes_get" (func $wasi_args_sizes_get (param i32 i32) (result i32)))
(memory 1)

(export "memory" (memory 0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this needs to be exported? (memory 1 1) creates a 64K buffer.

test/wasi/args.wast Outdated
(memory 1)

(export "memory" (memory 0))
(data (i32.const 32) "Hello" ) ;; Memory[32] = "Hello,"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello or Hello,

test/wasi/args.wast Outdated
local.set $mismatched_char_count

;; Check if each arg matches the expect values of "Hello," and "World!"
i32.const 1 ;; start at 1, skip argv[0] (name of currently running file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the function returns with two buffers in an expected layout, we could check them in one step (without a loop for arguments).

zherczeg
zherczeg requested changes Dec 9, 2024
View reviewed changes
test/wasi/args.wast Outdated
(if
(then
i32.const -1
local.set $memory_error
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about return?

@matetokodi
WASI: add args_get and args_sizes_get
33e1adc 
Update the test script to test passing the arguments as well

Signed-off-by: Máté Tokodi mate.tokodi@szteszoftver.hu
zherczeg
zherczeg approved these changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

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

LGTM

@clover2123 clover2123 merged commit 28d164d into Samsung:main Dec 10, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clover2123 clover2123 clover2123 approved these changes

@zherczeg zherczeg zherczeg approved these changes

@ksh8281 ksh8281 Awaiting requested review from ksh8281 ksh8281 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matetokodi @clover2123 @zherczeg