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

Implement imported/exported modules/instances #2461

Conversation

alexcrichton
Copy link
Member

This commit implements the final piece of the module linking proposal
which is to flesh out the support for importing/exporting instances and
modules. This ended up having a few changes:

  • Two more PrimaryMap instances are now stored in an Instance. The value
    for instances is InstanceHandle (pretty easy) and for modules it's
    Box<dyn Any> (less easy).

  • The custom host state for InstanceHandle for wasmtime is now
    Arc<TypeTables to be able to fully reconstruct an instance's types
    just from its instance.

  • Type matching for imports now has been updated to take
    instances/modules into account.

One of the main downsides of this implementation is that type matching
of imports is duplicated between wasmparser and wasmtime, leading to
posssible bugs especially in the subtelties of module linking. I'm not
sure how best to unify these two pieces of validation, however, and it
may be more trouble than it's worth.

cc #2094

Note that this is built on #2451 so only the last commit is relevant.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm lightbeam Issues related to the Lightbeam compiler wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Dec 1, 2020
@github-actions
Copy link

github-actions bot commented Dec 1, 2020

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "lightbeam", "wasmtime:api", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton force-pushed the import-export-instances-modules branch from c271404 to b3160d7 Compare December 2, 2020 22:58
@github-actions github-actions bot added cranelift:area:peepmatic fuzzing Issues related to our fuzzing infrastructure labels Dec 2, 2020
@alexcrichton alexcrichton force-pushed the import-export-instances-modules branch from b3160d7 to 3dd17c4 Compare December 2, 2020 23:24
@github-actions
Copy link

github-actions bot commented Dec 2, 2020

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift:area:peepmatic", "fuzzing"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: cranelift:area:peepmatic, fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

Changes look great 👍. A few typo corrections and nits.

crates/environ/src/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/module.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/instance.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/types/matching.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/instance.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/instance.rs Outdated Show resolved Hide resolved
This commit implements the final piece of the module linking proposal
which is to flesh out the support for importing/exporting instances and
modules. This ended up having a few changes:

* Two more `PrimaryMap` instances are now stored in an `Instance`. The value
  for instances is `InstanceHandle` (pretty easy) and for modules it's
  `Box<dyn Any>` (less easy).

* The custom host state for `InstanceHandle` for `wasmtime` is now
  `Arc<TypeTables` to be able to fully reconstruct an instance's types
  just from its instance.

* Type matching for imports now has been updated to take
  instances/modules into account.

One of the main downsides of this implementation is that type matching
of imports is duplicated between wasmparser and wasmtime, leading to
posssible bugs especially in the subtelties of module linking. I'm not
sure how best to unify these two pieces of validation, however, and it
may be more trouble than it's worth.

cc bytecodealliance#2094
@alexcrichton alexcrichton force-pushed the import-export-instances-modules branch from 3dd17c4 to d11c1ee Compare December 3, 2020 15:17
Currently there's two witx binaries in our repository given the two wasi
spec submodules, so this updates the publication script to vendor the
right one.
@alexcrichton alexcrichton merged commit f003388 into bytecodealliance:main Dec 3, 2020
@alexcrichton alexcrichton deleted the import-export-instances-modules branch December 3, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:wasm cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure lightbeam Issues related to the Lightbeam compiler wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants