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

Initial implementation of module linking #26

Merged
merged 7 commits into from
Jun 18, 2020

Conversation

alexcrichton
Copy link
Member

This commit is the initial implementation of the module linking proposal
in the three tooling crates of this repository. Unfortunately this is
just one massive commit which isn't really able to be reviewed in a
nuanced way. I wanted to bundle everything up though because
implementing each of the features of the module linking proposal ended
up having a pretty major impact on at least the text parser.

The main focus of this commit is getting the text parser to a "pretty
complete" point. I suspect there's still various odds and ends
remaining, but I believe it's almost entirely up to date with the
current state of the proposal. The wasmprinter crate should be in good
shape but I'm pretty certian that wasmparser is still missing at least
some pieces of validation.

This is hoped to be a good unit of work to start from after which I can
focus more on a complete implementation in wasmparser in terms of
validation. From there I think we'll have a good suite of tests and can
start moving on towards an implementation in an engine and/or
implementation in toolchains.

The largest changes here are to the text parser, and it's important to
note that the text parser at least is ideally pretty low-impact in terms
of risky changes. We've got a pretty good regression test suite, and the
interface of the text parser is "given this string give me the binary
wasm". Coupled with the fact that the text parser is 100% safe code I'm
pretty certain that the only bad pieces which can arise are panics,
which fuzzers in theory should find relatively quickly.

@alexcrichton
Copy link
Member Author

FWIW I've also been keeping a running log of various planned changes at https://gist.github.com/alexcrichton/506ac6d2f7d505d556d68fb969489183. That should be a high-level summary of where the binary format and everything is currently at. It also has some of the trickiness around the text parser.

@yurydelendik @fitzgen I'm curious, do y'all want to take a look at this? I completely understand if you'd prefer not to, it's sort of just me dumping thounsands of lines of code into this repo. To make matters worse I'm the primary owner of most of the changed code and I can't review my own work :(.

I do think that all is not lost though. Given the context in which the wast crate is used I think it's pretty low risk (as mentioned in the PR description). I hope to have much smaller follow-ups for all future bugfixes and features for module-linking.

@fitzgen
Copy link
Member

fitzgen commented Jun 12, 2020

Would it make sense to do a video chat where you screen share and walk us through the top-level changes?

FWIW I've also been keeping a running log of various planned changes at https://gist.github.com/alexcrichton/506ac6d2f7d505d556d68fb969489183. That should be a high-level summary of where the binary format and everything is currently at. It also has some of the trickiness around the text parser.

So this has both module linking and interface types edits in it?

@tschneidereit
Copy link
Member

First off, 🎉🎉🎉‼

@yurydelendik @fitzgen I'm curious, do y'all want to take a look at this? I completely understand if you'd prefer not to, it's sort of just me dumping thounsands of lines of code into this repo. To make matters worse I'm the primary owner of most of the changed code and I can't review my own work :(.

I think it'd be good if we could define clearly what we're comfortable landing without review, but then do a proper review of the remaining parts.

Based on a conversation with @alexcrichton, here's what I'd propose as a first approximation of that:
We can skip review of the changes to the text parsers because

  • they don't introduce additional security-relevant functionality since
    • they don't change or introduce any unsafe code
    • the output of the parser isn't treated as trusted: we emit a binary .wasm file that gets fed into the binary parser as any other untrusted content
  • we're not at this point worried about perfect functional correctness: that's not possible in any well-defined sense anyway, given that the spec this implements isn't finished itself

Additionally, the tests can be skipped / rubber-stamped.

The changes to the binary parser, and any other bits not captured by this should be reviewed.

@alexcrichton do you think that makes sense? And if so, can you list the files that should be reviewed?

@alexcrichton
Copy link
Member Author

I'd be more than happy to jump on a call, but I agree with @tschneidereit's assessment. One thing I might tweak though is getting a close-ish review of the tests, and if others have ideas for new tests that'd be pretty useful. So far that's the best way for exposing issues, and given the context of the text parser it's generally fine to gloss over its internal details especially while the spec is still settling.

@fitzgen also for interface types, yeah that gist contains both, and I'll be making further PRs to support interface types. That part should be a lot simpler though.

@fitzgen
Copy link
Member

fitzgen commented Jun 12, 2020

Sounds like a plan to me!

Copy link
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

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

Overall looks good. I could not figure out how do we perform inlined modules validation.

crates/wasmparser/src/binary_reader.rs Show resolved Hide resolved
crates/wasmparser/src/primitives.rs Outdated Show resolved Hide resolved
crates/wasmparser/src/validator.rs Show resolved Hide resolved
@alexcrichton
Copy link
Member Author

Ok I'm gonna go ahead and merge this and will continue to follow-up on validation in following PRs!

This commit is the initial implementation of the module linking proposal
in the three tooling crates of this repository. Unfortunately this is
just one massive commit which isn't really able to be reviewed in a
nuanced way. I wanted to bundle everything up though because
implementing each of the features of the module linking proposal ended
up having a pretty major impact on at least the text parser.

The main focus of this commit is getting the text parser to a "pretty
complete" point. I suspect there's still various odds and ends
remaining, but I believe it's almost entirely up to date with the
current state of the proposal. The `wasmprinter` crate should be in good
shape but I'm pretty certian that `wasmparser` is still missing at least
some pieces of validation.

This is hoped to be a good unit of work to start from after which I can
focus more on a complete implementation in `wasmparser` in terms of
validation. From there I think we'll have a good suite of tests and can
start moving on towards an implementation in an engine and/or
implementation in toolchains.

The largest changes here are to the text parser, and it's important to
note that the text parser at least is ideally pretty low-impact in terms
of risky changes. We've got a pretty good regression test suite, and the
interface of the text parser is "given this string give me the binary
wasm". Coupled with the fact that the text parser is 100% safe code I'm
pretty certain that the only bad pieces which can arise are panics,
which fuzzers in theory should find relatively quickly.
Also replace a "wut" string with an actual namespace description.
@alexcrichton alexcrichton merged commit c1aa81e into bytecodealliance:master Jun 18, 2020
@alexcrichton alexcrichton deleted the interface-types branch June 18, 2020 15:50
fitzgen pushed a commit to fitzgen/wasm-tools that referenced this pull request Oct 21, 2020
This commit adds functionality to parse comments in the source to
`Parser`, allowing access to `Comment` tokens from a `Cursor`. This in
turn should allow scraping comments on items, perhaps for documentation
and other purposes.

All existing methods on `Cursor` will implicitly skip
whitespace/comments (as they do today). Additionally `Parser::is_empty`
will only take `Token` tokens into account. Finally `cur_span` also will
only return the span for `Token` tokens, rather than comment tokens.
These latter pieces may want to get tweaked over time...

Closes bytecodealliance#25
dhil added a commit to dhil/wasm-tools that referenced this pull request Oct 10, 2022
dhil added a commit to dhil/wasm-tools that referenced this pull request Nov 1, 2023
This patch allows the use of abstract continuation types without
requiring the GC proposal to be enabled first.
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.

4 participants