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

Add a compile command to Wasmtime. #2791

Merged
merged 12 commits into from
Apr 2, 2021

Conversation

peterhuene
Copy link
Member

This PR adds a compile command to the Wasmtime CLI.

The command can be used to Ahead-Of-Time (AOT) compile WebAssembly modules.

With the all-arch feature enabled, AOT compilation can be performed for
non-native architectures (i.e. cross-compilation).

The Module::compile method has been added to perform AOT compilation.

A few of the CLI flags relating to "on by default" Wasm features have been
changed to be "--disable-XYZ" flags.

A simple example of using the wasmtime compile command:

$ wasmtime compile input.wasm
$ wasmtime input.cwasm

@peterhuene
Copy link
Member Author

peterhuene commented Mar 30, 2021

A follow-up PR will allow for disabling JIT in Wasmtime via a feature flag (ideally with the removal of dependencies, but that might be a substantial undertaking).

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:docs Issues related to Wasmtime's documentation labels Mar 30, 2021
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "wasmtime:api", "wasmtime:docs"

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

  • peterhuene: wasmtime:api

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

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Very nice! I'm liking how this is shaping up!

crates/wasmtime/src/config.rs Show resolved Hide resolved
crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/arm32/mod.rs Show resolved Hide resolved
crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved
src/commands/wasm2obj.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/config.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/commands/compile.rs Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Show resolved Hide resolved
crates/wasmtime/src/module.rs Outdated Show resolved Hide resolved
@peterhuene peterhuene force-pushed the compile-command branch 4 times, most recently from b840adb to 585169e Compare April 1, 2021 07:09
@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Apr 1, 2021
@github-actions
Copy link

github-actions bot commented Apr 1, 2021

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

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

  • peterhuene: wasmtime:c-api

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

Learn more.

crates/wasmtime/src/engine.rs Show resolved Hide resolved
crates/wasmtime/src/module.rs Show resolved Hide resolved
crates/wasmtime/src/module/serialization.rs Show resolved Hide resolved
src/commands/compile.rs Outdated Show resolved Hide resolved
src/commands/compile.rs Outdated Show resolved Hide resolved
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks again!

I think this is good to go whenever, if there's anything minor remaining seems fine to leave a FIXME

peterhuene and others added 6 commits April 1, 2021 19:38
This commit adds a `compile` command to the Wasmtime CLI.

The command can be used to Ahead-Of-Time (AOT) compile WebAssembly modules.

With the `all-arch` feature enabled, AOT compilation can be performed for
non-native architectures (i.e. cross-compilation).

The `Module::compile` method has been added to perform AOT compilation.

A few of the CLI flags relating to "on by default" Wasm features have been
changed to be "--disable-XYZ" flags.

A simple example of using the `wasmtime compile` command:

```text
$ wasmtime compile input.wasm
$ wasmtime input.cwasm
```
Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
* Remove `Config::for_target` in favor of setter `Config::target`.
* Remove explicit setting of Cranelift flags in `Config::new` in favor of
  calling the `Config` methods that do the same thing.
* Serialize the package version independently of the data when serializing a
  module.
* Use struct deconstructing in module serialization to ensure tunables and
  features aren't missed.
* Move common log initialization in the CLI into `CommonOptions`.
This commit hides the existing WebAssembly feature CLI options (e.g.
`--enable-simd`) and adds a `--wasm-features` flag that enables multiple
(or all) WebAssembly features.

Features can be disabled by prefixing the value with `-`, e.g.
`--wasm-features=-simd`.
* Removed `Config::cranelift_clear_cpu_flags`.
* Renamed `Config::cranelift_other_flag` to `Config::cranelift::flag_set`.
* Renamed `--cranelift-flag` to `--cranelift-set`.
* Renamed `--cranelift-preset` to `--cranelift-enable`.
This commit adds the `wasmtime settings` command to print out available
Cranelift settings for a target (defaults to the host).

The compile command has been updated to remove the Cranelift ISA options in
favor of encouraging users to use `wasmtime settings` to discover what settings
are available.  This will reduce the maintenance cost for syncing the compile
command with Cranelift ISA flags.
* Move `Module::compile` to `Engine::precompile_module`.
* Remove `Module::deserialize` method.
* Make `Module::serialize` the same format as `Engine::precompile_module`.
* Make `Engine::precompile_module` return a `Vec<u8>`.
* Move the remaining serialization-related code to `serialization.rs`.
This commit sorts the settings output by the `wasmtime settings` command.
* Expand doc comment on `Engine::precompile_module`.
* Add FIXME comment regarding a future ISA flag compatibility check before
  doing a JIT from `Module::from_binary`.
* Remove no-longer-needed CLI groups from the `compile` command.
Update the `wat` crate to latest version and use `Error::set_path` in
`Module::from_file` to properly record the path associated with errors.
@github-actions github-actions bot added cranelift:wasm fuzzing Issues related to our fuzzing infrastructure labels Apr 2, 2021
@github-actions
Copy link

github-actions bot commented Apr 2, 2021

Subscribe to Label Action

cc @fitzgen

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

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

  • fitzgen: fuzzing

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

Learn more.

This commit changes how both the shared flags and ISA flags are stored in the
serialized module to detect incompatibilities when a serialized module is
instantiated.

It improves the error reporting when a compiled module has mismatched shared
flags.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

👍

crates/wasmtime/src/module/serialization.rs Show resolved Hide resolved
fmt.indent(|fmt| {
fmtln!(fmt, "let values = match &d.detail {");
fmt.indent(|fmt| {
fmtln!(fmt, "detail::Detail::Preset => return None,");
Copy link
Member

Choose a reason for hiding this comment

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

One possible backwards-compatibility consideration: it looks like this will result in no encoding of default options in the saved compiled module, but if we later change a default in an incompatible way, would we want to catch that?

(It's entirely possible that this is made irrelevant by some other versioning -- do you encode e.g. a git hash or wasmtime version in the .cwasm file too?)

Copy link
Member Author

@peterhuene peterhuene Apr 2, 2021

Choose a reason for hiding this comment

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

Agreed that there are way too many ways incompatibilities can be introduced, so to combat that we do indeed record the crate version in the cwasm and must be an identical match for now (similar to the preexisting requirement for the code cache).

@cfallin
Copy link
Member

cfallin commented Apr 2, 2021

(LGTM otherwise with respect to Cranelift settings changes.)

@peterhuene
Copy link
Member Author

Thanks! Proceeding to merge.

@peterhuene peterhuene merged commit b7b47e3 into bytecodealliance:main Apr 2, 2021
@peterhuene peterhuene deleted the compile-command branch April 2, 2021 18:18
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Apr 26, 2021
I thought I was being clever suggesting that `Module::deserialize` was
removed from bytecodealliance#2791 by funneling all module constructors into
`Module::new`. As our studious fuzzers have found, though, this means
that `Module::new` is not safe currently to pass arbitrary user-defined
input into. Now one might pretty reasonable expect to be able to do
that, however, being a WebAssembly engine and all. This PR as a result
separates the `deserialize` part of `Module::new` back into
`Module::deserialize`.

This means that binary blobs created with `Module::serialize` and
`Engine::precompile_module` will need to be passed to
`Module::deserialize` to "rehydrate" them back into a `Module`. This
restores the property that it should be safe to pass arbitrary input to
`Module::new` since it's always expected to be a wasm module. This also
means that fuzzing will no longer attempt to fuzz `Module::deserialize`
which isn't something we want to do anyway.
alexcrichton added a commit that referenced this pull request Apr 27, 2021
* Bring back `Module::deserialize`

I thought I was being clever suggesting that `Module::deserialize` was
removed from #2791 by funneling all module constructors into
`Module::new`. As our studious fuzzers have found, though, this means
that `Module::new` is not safe currently to pass arbitrary user-defined
input into. Now one might pretty reasonable expect to be able to do
that, however, being a WebAssembly engine and all. This PR as a result
separates the `deserialize` part of `Module::new` back into
`Module::deserialize`.

This means that binary blobs created with `Module::serialize` and
`Engine::precompile_module` will need to be passed to
`Module::deserialize` to "rehydrate" them back into a `Module`. This
restores the property that it should be safe to pass arbitrary input to
`Module::new` since it's always expected to be a wasm module. This also
means that fuzzing will no longer attempt to fuzz `Module::deserialize`
which isn't something we want to do anyway.

* Fix an example

* Mark `Module::deserialize` as `unsafe`
mchesser pushed a commit to mchesser/wasmtime that referenced this pull request May 24, 2021
* Bring back `Module::deserialize`

I thought I was being clever suggesting that `Module::deserialize` was
removed from bytecodealliance#2791 by funneling all module constructors into
`Module::new`. As our studious fuzzers have found, though, this means
that `Module::new` is not safe currently to pass arbitrary user-defined
input into. Now one might pretty reasonable expect to be able to do
that, however, being a WebAssembly engine and all. This PR as a result
separates the `deserialize` part of `Module::new` back into
`Module::deserialize`.

This means that binary blobs created with `Module::serialize` and
`Engine::precompile_module` will need to be passed to
`Module::deserialize` to "rehydrate" them back into a `Module`. This
restores the property that it should be safe to pass arbitrary input to
`Module::new` since it's always expected to be a wasm module. This also
means that fuzzing will no longer attempt to fuzz `Module::deserialize`
which isn't something we want to do anyway.

* Fix an example

* Mark `Module::deserialize` as `unsafe`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. cranelift:wasm cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. wasmtime:docs Issues related to Wasmtime's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants