Skip to content

WASI: implement argsAlloc and argsFree #2364

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

Merged
merged 6 commits into from
Apr 30, 2019
Merged

Conversation

shritesh
Copy link
Contributor

Implementing ArgIterator for WASI would require introducing a free function to the internally allocated buffer so I've simply introduced a compileError for WASI.

@shritesh shritesh changed the title WASI: args WASI: implement argsAlloc and argsFree Apr 27, 2019
std/os.zig Outdated
var count: usize = undefined;
var buf_size: usize = undefined;

_ = os.wasi.args_sizes_get(&count, &buf_size);
Copy link
Member

Choose a reason for hiding this comment

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

This should assert that there is no error, or handle it. If it asserts that there is no error, it should explain why the error is impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The return values are not documented at all. I looked at the source code of wasmtime and Lucet and they are not consistent. I have introduced error.ArgsSizesGetFailed and error.ArgsGetFailed in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

In that case we can follow the pattern set by std/os.zig where an unexpected/undocumented error code is returned. Look for unexpectedErrorPosix and unexpectedErrorWindows.

Generally it's accepted that an undocumented error code might be returned from the OS which translates to error.Unexpected and has a mechanism for logging error codes with a build option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed them to unexpectedErrorPosix. Most of WASI functions (if they are applicable), are similar to POSIX.

@andrewrk andrewrk dismissed their stale review April 29, 2019 22:56

new commits

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

I'm ready to merge this, but I might have noticed a problem - I'll give you a chance to double-check and tell me it's ok.

I'm looking forward to being able to build & run WASI targets so we can start adding test coverage for this stuff!

@@ -2203,6 +2235,16 @@ pub fn argsAlloc(allocator: *mem.Allocator) ![]const []u8 {
}

pub fn argsFree(allocator: *mem.Allocator, args_alloc: []const []u8) void {
if (builtin.os == Os.wasi) {
const last_item = args_alloc[args_alloc.len - 1];
const last_byte_addr = @ptrToInt(last_item.ptr) + last_item.len + 1; // null terminated
Copy link
Member

Choose a reason for hiding this comment

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

If this has a +1 doesn't the other function need a -1?

Copy link
Contributor Author

@shritesh shritesh Apr 30, 2019

Choose a reason for hiding this comment

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

The strings in argv_buf, that is written to by the args_get function, are contiguous null terminated c-strings. My assumption here is that mem.toSlice doesn't include the final null-terminated character and I've added it myself here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see.

@andrewrk andrewrk merged commit 01365be into ziglang:master Apr 30, 2019
@shritesh shritesh deleted the wasi_alloc branch May 1, 2019 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants