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.

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

Add a wasmtime-specific wasmtime_wat2wasm C API #1206

Merged
merged 7 commits into from
Mar 3, 2020

Conversation

alexcrichton
Copy link
Member

This commit implements a wasmtime-specific C API for converting the text
format to the binary format. An upstream spec issue exists for adding
this to the C API, but in the meantime we can experiment with our own
version of this API and use it in the C# extension, for example!

Closes #1000

This commit implements a wasmtime-specific C API for converting the text
format to the binary format. An upstream spec issue exists for adding
this to the C API, but in the meantime we can experiment with our own
version of this API and use it in the C# extension, for example!

Closes bytecodealliance#1000
@alexcrichton alexcrichton requested a review from peterhuene March 2, 2020 16:51
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Looks great! Just the two comments below.

crates/c-api/include/wasmtime.h Show resolved Hide resolved
crates/misc/dotnet/src/Engine.cs Outdated Show resolved Hide resolved
@alexcrichton alexcrichton requested a review from peterhuene March 2, 2020 19:06
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

👍 Thanks for fixing this so we could remove all the .wasm files, they were bugging me too.

@rene-fonseca
Copy link

Question: Would it be possible to support line number and column info for parse failure - ie. separate from error message? Or maybe just prepare API for now - and then add info later if it becomes possible to support. Just to avoid having to change API later.

@peterhuene
Copy link
Member

I believe the wat crate already provides line and column, along with contextual information of where the parse error was.

@rene-fonseca
Copy link

rene-fonseca commented Mar 2, 2020

EDITED:

Looking at https://webassembly.github.io/spec/core/text/lexical.html - it seems that null-terminator is allowed in comments. Or am I misunderstanding something here.

Hmm - thought I saw it using const char* - but looking again it is using a wasm_byte_vec_t which is good for the API itself.

But maybe double check if null-terminator is supported internally.

let wat = match str::from_utf8((*wat).as_slice()) {

https://github.com/bytecodealliance/wasmtime/pull/1206/files#diff-c69f02cd99a3d2831c084254572074e6R98

@peterhuene
Copy link
Member

Oh sorry, I misread that. As separate pieces of information, not how we're doing it currently, no.

@alexcrichton is there an error type we can extract the line and column from?

@alexcrichton
Copy link
Member Author

Yes the wat/wast crates have enough information to return line numbers of errors. The C API in general doesn't do super well with errors and adding rich information like filename/line number (as opposed to just a string) would likely require us to invent new idioms/machinery/etc. I don't think there's any reason why we can't do that, but I would imagine that if we were to focus on errors in the C API we should take a look at them as a whole instead of only situation-solving one API.

@rene-fonseca
Copy link

Maybe add some note in API documentation about if UTF-8 BOM is allowed or not. Not sure if Rust runtime will strip it. If it is supposed to work then it would be good to have a wat test file with a BOM.

@alexcrichton
Copy link
Member Author

Oh I forgot to mention earlier, but your comment about the nul byte was previously correct when it was using char* but now that it's using wasm_byte_vec_t* that should be handled because it's a pointer/length.

For BOM handling Rust doesn't do anything special there, it would be up to the lexer, if anything, to skip it. Currently the wat parser doesn't skip it and will fail on any file that contains the BOM marker. Looking at the lexical structure of the text format I don't think the BOM is allowed, so it sounds like this may be a spec question/update/tweak to bring up with the official repo.

@rene-fonseca
Copy link

Thanks @alexcrichton. But one note - BOM, if handled, should only be done at the decoding/encoding layer and not passed on to the internal Unicode representation. So not direct impact here for lexer.

@alexcrichton
Copy link
Member Author

Ok I'm gonna go ahead and land this and we can continue to iterate on specifics in-tree.

@alexcrichton alexcrichton merged commit 77e17d8 into bytecodealliance:master Mar 3, 2020
@alexcrichton alexcrichton deleted the wat2wasm-cpi branch March 3, 2020 16:29
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.

Add support for WAT module validation/creation and API
4 participants