Skip to content

Allow binary modules in text format #280

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 2 commits into from
May 9, 2016
Merged

Allow binary modules in text format #280

merged 2 commits into from
May 9, 2016

Conversation

rossberg
Copy link
Member

This PR adds support for defining modules directly in terms of a binary string in the text format:

(module "\00asm\0b\00\00\00")
(assert_invalid (module "\00as") "unexpected end")

To ease formatting, strings can be split up into several literals, which are simply concatenated:

(module
  "\00asm"  ;; magic header
  "\0b\00\00\00"  ;; version
)

The same string splitting is now allowed in segment nodes.

Further changes:

  • Correct string escaping in S-expr output.
  • Tweaked S-expr output to match binary section order.
  • Type and function definitions are named with their index, for easier reference.
  • Functions are now formatted literally using the (type i) syntax for their signature, in order to not implicitly change the content of the type section. (One step closer to (parse o format) being an identity.)
  • The output command can now be used without an argument, which just dumps the current module to stdout.

@lukewagner
Copy link
Member

Cool! So can the test harness then take every text-.wast, convert it to a binary-.wast and run that too?

@rossberg
Copy link
Member Author

You could, but you'd need to implement some infrastructure in the harness to parse & split up scripts and reassemble them (and you could already have done that before, since you can just (input ...) a binary from an aux file).

Not sure that is the best direction to go, though. Moving forward, I think it makes more sense to restructure our tests to be self-contained modules, instead of increasing the infrastructure requirements. Use the power that is already inherently there.

The more immediate benefit of this feature is that we can now write tests exercising specifics of the encoding, or more interestingly, malformed binaries. :)

@lukewagner
Copy link
Member

It seems like it'd be pretty easy, though: parse in the text-.wast, convert each Ast.module into a binary string, then replace the Script.representation Textual with a Binary, then run that. Really, it's not even a harness change, just a flag that says: test every .wast having done a AST->binary->AST roundtrip first.

@rossberg
Copy link
Member Author

@lukewagner, yes, it's straightforward to implement an execution flag that causes each module definition to roundtrip through binary when running the script. But that would only benefit testing ml-proto itself, not other consumers, AFAICS.

Running the test suite should not require every consumer to implement a random combination of an S-expr script interpreter, with a bunch of ad-hoc command line flags, and a harness exploiting those in specific ways. That's too much upfront complexity for my taste.

If we instead move towards self-contained test modules, then every Wasm consumer can immediately run them without additional infrastructure. In particular, consumers that only understand the binary format.

The main reason I originally introduced the script assertion language was that it was not otherwise possible to express self-contained tests at the time, because we did not have a start function yet. Now that's moot, and a technically leaner approach is more attractive, IMO.

@lukewagner
Copy link
Member

You're right, for the purpose of producing tests that run on different engines we need something different; I was mostly thinking about validating ml-proto itself using existing tests "for free".

@lukewagner
Copy link
Member

(anyhow, lgtm!)

@rossberg rossberg merged commit ada120d into master May 9, 2016
@rossberg rossberg deleted the bin-module branch May 9, 2016 12:58
ngzhian added a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
This got lost in the rebase too, and WebAssembly#277 only partially fixed it.
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