-
Notifications
You must be signed in to change notification settings - Fork 456
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
[interpreter] Support quoted module definitions in .wast #475
Conversation
Hm... I'm pretty sure wabt's parser and lexer are re-entrant. Lemme check. :-) |
@binji, is reentrance actually necessary for this? |
Ah, you're right. The whole point is to defer the module text parsing :-) Yes, this looks like it should be fine in wabt. |
@@ -160,10 +160,11 @@ let inline_type (c : context) ty at = | |||
%token CALL CALL_INDIRECT RETURN | |||
%token GET_LOCAL SET_LOCAL TEE_LOCAL GET_GLOBAL SET_GLOBAL | |||
%token LOAD STORE OFFSET_EQ_NAT ALIGN_EQ_NAT | |||
%token CONST UNARY BINARY COMPARE CONVERT | |||
%token CONST UNARY BINARY TEST COMPARE CONVERT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is TEST
new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was just missing from this list. It seems that it actually is unnecessary to declare a token like this when there already is a %token<...> declaration like below, but to be consistent I added this one here.
If everybody is okay with this extension to the wast format then I'll go forward and convert all .fail. tests to quotes. @binji, would you be okay with reviewing this PR? |
@binji, friendly ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. lgtm, though I'd rather not support recursive quoting.
match def.it with | ||
| Textual m -> m | ||
| Encoded (_, bs) -> Decode.decode "binary" bs | ||
in bind mods x_opt m; | ||
| Quoted (_, s) -> unquote (Parse.string_to_module s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means you could do a recursive quote, right? (module quote "(module quote \"...
It might be nice to disallow that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. No real use case for that, it just turned out to be simpler and more regular in the interpreter to allow it than to rule it out. But I don't mind making it a syntax error. Will look into it.
Okay, disallowed extended module syntax in quotes (and also .wat files). Converted all existing .fail. tests into uses of quotes (with some additional variations, because it's much easier now). Landing. |
Upstream changed the optimised build to just 'wasm' and the debug build to 'wasm.debug' in: WebAssembly/spec#475
Upstream changed the optimised build to just 'wasm' and the debug build to 'wasm.debug' in: WebAssembly/spec#475
This instruction was added in WebAssembly#127. Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
…tting [spec] Fix formatting of any.convert_extern and extern.convert_any
As suggested by @AndrewScheidecker in this comment on #474, this adds the ability to quote a module source (i.e., .wat file) in a string, so that syntax errors for the text format are deferred and thus can be tested in a way analogous to decoding errors for binary format:
(The PR also changes the syntax of binary modules to require a "binary" keyword, in order to disambiguate.)
This change would make separate
.fail.
tests unnecessary. It was an easy change to the interpreter, but I don't know how simple it would be for other tools. @binji, @kripken, @dschuff, any opinions?