Skip to content

Update to the next version of the witx crate#234

Merged
alexcrichton merged 5 commits into
WebAssembly:mainfrom
alexcrichton:witx-next
Feb 23, 2021
Merged

Update to the next version of the witx crate#234
alexcrichton merged 5 commits into
WebAssembly:mainfrom
alexcrichton:witx-next

Conversation

@alexcrichton

Copy link
Copy Markdown
Collaborator
  • Generate adapter functions instead of simply a header file to have a
    place where adapter code can go.
  • Implement adapters in terms of the instructions that the witx crate
    tells us about.
  • Update the interface of functions to what witx expects, notably
    string arguments are now only taken as char* and strlen happens in
    the adapter function.
  • Update defined/predefined/undefined symbol lists for types that have
    been updated.

Some precise generated code has changed but the actual APIs should all
be the same except for the change to not take the length of the string
in the raw WASI call, since idiomatically C doesn't pass the length of
strings around.

Eventually it's expected that the shim functions, while sometimes not
necessary today, will implement more checks and more conversions as
necessary for new APIs.

@alexcrichton

Copy link
Copy Markdown
Collaborator Author

Note that this is dependant on WebAssembly/WASI#395, so this should probably wait for that to get settled first.

* Generate adapter functions instead of simply a header file to have a
  place where adapter code can go.
* Implement adapters in terms of the instructions that the `witx` crate
  tells us about.
* Update the interface of functions to what `witx` expects, notably
  string arguments are now only taken as `char*` and `strlen` happens in
  the adapter function.
* Update defined/predefined/undefined symbol lists for types that have
  been updated.

Some precise generated code has changed but the actual APIs should all
be the same except for the change to not take the length of the string
in the raw WASI call, since idiomatically C doesn't pass the length of
strings around.

Eventually it's expected that the shim functions, while sometimes not
necessary today, will implement more checks and more conversions as
necessary for new APIs.

@sbc100 sbc100 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can't read any of the rust but the output looks good and the direction seems good.

Comment thread libc-bottom-half/sources/__wasilibc_real.c
Comment thread libc-bottom-half/sources/__wasilibc_real.c Outdated
int __wasilibc_nocwd_symlinkat(const char *path1, int fd, const char *path2) {
__wasi_errno_t error =
__wasi_path_symlink(path1, strlen(path1), fd, path2, strlen(path2));
__wasi_errno_t error = __wasi_path_symlink(path1, fd, path2);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that these are now library calls its makes me wonder if we should continue to use the __ prefix. Perhaps these should be just wasi_path_symlink and then the real imports could be __wasi_path_symlink? Would most likely mean changing all the types too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm happy to implement either way, but @sunfishcode would probably be best to answer the question (I'm not familiar enough with C idioms to know what best to do here).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The __ prefix continues to make sense :-). For example, users can call WASI APIs directly, and may have their own wasi_* wrappers around things, so the __ prefix here ensures that even if they do that, they can also link in libc at the same time.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That said, if we wanted to make it easier for people to write code that calls directly into WASI, one thing we could do would be to provide weak aliases named wasi_* that point to the __wasi_ symbols.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I just think maybe the __ prefix is not the right way to carve of our own namespace. Of course this is unrelated to this PR, but I think __ is normally reserved for toolchain internal stuff isn't it?

Again, unrelated to this PR.

@sunfishcode sunfishcode Feb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The C standard just says "reserved for any use". In practice, musl, glibc, and other libc impls commonly use __ for their own purposes.

Comment thread expected/wasm32-wasi/undefined-symbols.txt Outdated
Comment thread libc-bottom-half/headers/public/wasi/api.h
Comment thread libc-bottom-half/headers/public/wasi/api.h
));

__wasi_size_t *retptr0,
__wasi_size_t *retptr1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like we lost some information here about what these pointers mean.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah unfortunately the documentation got shuffled a bit since each individual parameter is no longer documented in the source file, although I tried to make sure that the documentation was at least preserved somewhat in the @return section for the function

@sbc100 sbc100 Feb 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any why we can keep the argc and argv_buf_size names some how?

Can these be retptr_argc and retptr_argv_buf_size perhaps? retptr0 and retptr1 are not very useful names.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Unfortuantely this is mostly due to a change in the *.witx file itself -- WebAssembly/WASI@ef8c1a5#diff-2f2ab5a9d79cd5c57a981bf769e772aa60405857803bcdbde3c3e8179315abb8L24-R26

Where previously there were separate result variables it's now modeled more closely with the intended interface type, which is a Result<(usize, usize), Errno>. This means that there's no way really to auto-generate the names as before since from the source it's just "return pointer for first tuple element" and the second one.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's a trade-off here where we could get names for these values back by returning a record rather than a tuple, but that would break the ABI today (records always turn into a C struct, which gets passed by one pointer rather than multiple). Its a little but unfortunate but I think losing those names is OK for now.

Comment thread libc-bottom-half/headers/public/wasi/api.h
Comment thread libc-bottom-half/headers/public/wasi/api.h
@alexcrichton alexcrichton merged commit 2b7e73a into WebAssembly:main Feb 23, 2021
@alexcrichton alexcrichton deleted the witx-next branch February 23, 2021 18:18
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.

4 participants