Skip to content

Binary encoder & decoder #268

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 15 commits into from
Apr 20, 2016
Merged

Binary encoder & decoder #268

merged 15 commits into from
Apr 20, 2016

Conversation

rossberg
Copy link
Member

Add working (post-order) encoder & decoder. Supersedes PR #261.

Extends the test runner with encoding & decoding of all test files via a wast->wasm transcoding and loading the resulting modules. Works for all current tests.

There are still a number of smaller TODOs left, but I think it makes more sense to address them in follow-ups. Other follow-up work: wasm->wast reverse transcoding, more aggressive tests and testing capabilities.

We'll probably also need to revise the use of different integer types throughout the spec, which is not always consistent with what the binary format supports.

@ghost
Copy link

ghost commented Mar 24, 2016

Could you squash it into one patch to make it easier to view.

@rossberg
Copy link
Member Author

@JSStats, the github diff view already squashes it, doesn't it?

@ghost
Copy link

ghost commented Mar 24, 2016

@rossberg-chromium Yes, thanks. Can checkout the 'decode' branch too.

expr e; op 0x03; list expr es1;
if es2 <> [] then op 0x04; list expr es2; op 0x17
| Select (e1, e2, e3) -> expr e1; expr e2; expr e3; op 0x05
| Br (x, eo) -> opt expr eo; op 0x06; arity1 eo; var x
Copy link

Choose a reason for hiding this comment

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

The expression is not optional in the binary encoding, one expression is expected which may be a nop. The arity is a constant 1 and not necessary but include as a concession.

Copy link
Member Author

Choose a reason for hiding this comment

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

See WebAssembly/design#595, which this already implements.


(* End section *)
let end_section () =
section "end" ignore () true
Copy link
Member

Choose a reason for hiding this comment

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

Currently there is no end section in BinaryEncoding.md. If we want an explicit terminator, perhaps instead we can propose a section_count : varuint32 that goes right after the version u32; but a dummy section seems like an abuse of what a section is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah right, forgot to remove that. Done.

@lukewagner
Copy link
Member

lukewagner commented Apr 20, 2016

I haven't read it all line-by-line, but fwiw, I think it's fine to merge this now and iterate towards precisely matching BinaryEncoding.md at either 0xb or 0xc. It's nice to have an alternate implementation that first builds the AST and only afterwards performs type validation. Ultimately, we'll shake out the discrepancies when we have ml-proto encode .wasms that SM/V8 consume and vice versa with sexpr-wasm-proto.

@rossberg
Copy link
Member Author

Okay, thanks. I'm taking that as an LGTM and go merge it.

@rossberg rossberg merged commit d992a1a into master Apr 20, 2016
@jfbastien jfbastien deleted the decode branch April 20, 2016 17:27
ngzhian added a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
* Update v128.const implementation status for v8

* Add note about Chrome version
rossberg pushed a commit that referenced this pull request Jul 23, 2024
Embedding:
* Adds an auxiliary syntactic class to represent exception throwing/propagation
* Allows `func_invoke` to propagate exceptions
* Adds `tag_type` and `exn_alloc` to the embedding interface

core/exec:
* Refactors exception allocation into `alloc-exception` in modules.rst to expose it to the embedder
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