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

[interpreter] Support quoted module definitions in .wast #475

Merged
merged 5 commits into from
Jun 6, 2017

Conversation

rossberg
Copy link
Member

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:

(assert_malformed (module quote "(module func)"))
(assert_malformed (module binary "\00\00\00\00"))

(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?

@rossberg rossberg requested a review from binji May 15, 2017 16:07
@binji
Copy link
Member

binji commented May 15, 2017

Hm... I'm pretty sure wabt's parser and lexer are re-entrant. Lemme check. :-)

@rossberg
Copy link
Member Author

@binji, is reentrance actually necessary for this?

@binji
Copy link
Member

binji commented May 17, 2017

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
Copy link
Member

Choose a reason for hiding this comment

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

Is TEST new?

Copy link
Member Author

@rossberg rossberg May 18, 2017

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.

@rossberg
Copy link
Member Author

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?

@rossberg
Copy link
Member Author

@binji, friendly ping.

Copy link
Member

@binji binji left a 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)
Copy link
Member

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.

Copy link
Member Author

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.

@rossberg
Copy link
Member Author

rossberg commented Jun 6, 2017

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.

@rossberg rossberg merged commit 740abc2 into master Jun 6, 2017
@rossberg rossberg deleted the unparseable branch June 6, 2017 12:46
sbc100 added a commit to WebAssembly/waterfall that referenced this pull request Jun 8, 2017
Upstream changed the optimised build to just 'wasm' and
the debug build to 'wasm.debug' in:
WebAssembly/spec#475
sbc100 added a commit to WebAssembly/waterfall that referenced this pull request Jun 8, 2017
Upstream changed the optimised build to just 'wasm' and
the debug build to 'wasm.debug' in:
WebAssembly/spec#475
ngzhian added a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
This instruction was added in WebAssembly#127.

Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
dhil pushed a commit to dhil/webassembly-spec that referenced this pull request Nov 13, 2023
…tting

[spec] Fix formatting of any.convert_extern and extern.convert_any
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