From f673cde370b1b16e8fbf83cc410bee30e3766545 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 14 Aug 2024 12:31:07 -0500 Subject: [PATCH] Refactor use of `CodeBuilder` on the CLI (#9125) * Refactor use of `CodeBuilder` on the CLI This commit refactors `wasmtime run` and `wasmtime compile` to unconditionally use `CodeBuilder` internally. This will in theory help out in the future if more debug-related options are added to `CodeBuilder` for example. This refactoring required some changes to `CodeBuilder` to be able to support a query about whether the internal bytes were a component or a module. The text format is now converted to binary immediately when supplied rather than during the compilation phase. This in turn required some API changes to make the selection of supporting the text format a compile-time choice of method rather than a runtime value. * Fix compile * Fix no-cranelift build of CLI --- crates/wasmtime/src/compile.rs | 2 +- crates/wasmtime/src/compile/code_builder.rs | 187 +++++++++++------- crates/wasmtime/src/compile/runtime.rs | 4 +- crates/wasmtime/src/engine.rs | 4 +- crates/wasmtime/src/lib.rs | 2 +- .../src/runtime/component/component.rs | 7 +- crates/wasmtime/src/runtime/module.rs | 9 +- src/commands/compile.rs | 22 +-- src/common.rs | 39 ++-- tests/all/debug/obj.rs | 2 +- 10 files changed, 152 insertions(+), 126 deletions(-) diff --git a/crates/wasmtime/src/compile.rs b/crates/wasmtime/src/compile.rs index a5fcfc15abf3..1e4ef3d6d5d0 100644 --- a/crates/wasmtime/src/compile.rs +++ b/crates/wasmtime/src/compile.rs @@ -41,7 +41,7 @@ use wasmtime_environ::{ }; mod code_builder; -pub use self::code_builder::{CodeBuilder, HashedEngineCompileEnv}; +pub use self::code_builder::{CodeBuilder, CodeHint, HashedEngineCompileEnv}; #[cfg(feature = "runtime")] mod runtime; diff --git a/crates/wasmtime/src/compile/code_builder.rs b/crates/wasmtime/src/compile/code_builder.rs index 3ea10c766c59..0d35886c9056 100644 --- a/crates/wasmtime/src/compile/code_builder.rs +++ b/crates/wasmtime/src/compile/code_builder.rs @@ -18,31 +18,36 @@ use std::path::Path; /// [`Module::deserialize`](crate::Module::deserialize) instead. /// /// A [`CodeBuilder`] requires a source of WebAssembly bytes to be configured -/// before calling [`compile_module_serialized`] or [`compile_module`]. This can be -/// provided with either the [`wasm`] or [`wasm_file`] method. Note that only -/// a single source of bytes can be provided. +/// before calling [`compile_module_serialized`] or [`compile_module`]. This can +/// be provided with either the [`wasm_binary`] or [`wasm_binary_file`] method. +/// Note that only a single source of bytes can be provided. /// /// # WebAssembly Text Format /// -/// This builder supports the WebAssembly Text Format (`*.wat` files). -/// WebAssembly text files are automatically converted to a WebAssembly binary -/// and then the binary is compiled. This requires the `wat` feature of the -/// `wasmtime` crate to be enabled, and the feature is enabled by default. -/// -/// If the text format is not desired then the [`CodeBuilder::wat`] method -/// can be used to disable this conversion. +/// This builder supports the WebAssembly Text Format (`*.wat` files) through +/// the [`CodeBuilder::wasm_binary_or_text`] and +/// [`CodeBuilder::wasm_binary_or_text_file`] methods. These methods +/// automatically convert WebAssembly text files to binary. Note though that +/// this behavior is disabled if the `wat` crate feature is not enabled. /// /// [`compile_module_serialized`]: CodeBuilder::compile_module_serialized /// [`compile_module`]: CodeBuilder::compile_module -/// [`wasm`]: CodeBuilder::wasm -/// [`wasm_file`]: CodeBuilder::wasm_file +/// [`wasm_binary`]: CodeBuilder::wasm_binary +/// [`wasm_binary_file`]: CodeBuilder::wasm_binary_file pub struct CodeBuilder<'a> { pub(super) engine: &'a Engine, wasm: Option>, wasm_path: Option>, dwarf_package: Option>, dwarf_package_path: Option>, - wat: bool, +} + +/// Return value of [`CodeBuilder::hint`] +pub enum CodeHint { + /// Hint that the code being compiled is a module. + Module, + /// Hint that the code being compiled is a component. + Component, } impl<'a> CodeBuilder<'a> { @@ -55,15 +60,14 @@ impl<'a> CodeBuilder<'a> { wasm_path: None, dwarf_package: None, dwarf_package_path: None, - wat: cfg!(feature = "wat"), } } - /// Configures the WebAssembly binary or text that is being compiled. + /// Configures the WebAssembly binary that is being compiled. /// - /// The `wasm_bytes` parameter is either a binary WebAssembly file or a - /// WebAssembly module in its text format. This will be stored within the - /// [`CodeBuilder`] for processing later when compilation is finalized. + /// The `wasm_bytes` parameter must be a binary WebAssembly file. + /// This will be stored within the [`CodeBuilder`] for processing later when + /// compilation is finalized. /// /// The optional `wasm_path` parameter is the path to the `wasm_bytes` on /// disk, if any. This may be used for diagnostics and other @@ -72,11 +76,15 @@ impl<'a> CodeBuilder<'a> { /// /// # Errors /// - /// If wasm bytes have already been configured via a call to this method or - /// [`CodeBuilder::wasm_file`] then an error will be returned. - pub fn wasm(&mut self, wasm_bytes: &'a [u8], wasm_path: Option<&'a Path>) -> Result<&mut Self> { + /// This method will return an error if WebAssembly bytes have already been + /// configured. + pub fn wasm_binary( + &mut self, + wasm_bytes: impl Into>, + wasm_path: Option<&'a Path>, + ) -> Result<&mut Self> { if self.wasm.is_some() { - bail!("cannot call `wasm` or `wasm_file` twice"); + bail!("cannot configure wasm bytes twice"); } self.wasm = Some(wasm_bytes.into()); self.wasm_path = wasm_path.map(|p| p.into()); @@ -88,73 +96,89 @@ impl<'a> CodeBuilder<'a> { Ok(self) } - /// Configures whether the WebAssembly text format is supported in this - /// builder. + /// Equivalent of [`CodeBuilder::wasm_binary`] that also accepts the + /// WebAssembly text format. /// - /// This support is enabled by default if the `wat` crate feature is also - /// enabled. + /// This method will configure the WebAssembly binary to be compiled. The + /// input `wasm_bytes` may either be the wasm text format or the binary + /// format. If the `wat` crate feature is enabled, which is enabled by + /// default, then the text format will automatically be converted to the + /// binary format. /// /// # Errors /// - /// If this feature is explicitly enabled here via this method and the - /// `wat` crate feature is disabled then an error will be returned. - pub fn wat(&mut self, enable: bool) -> Result<&mut Self> { - if !cfg!(feature = "wat") && enable { - bail!("support for `wat` was disabled at compile time"); - } - self.wat = enable; - Ok(self) + /// This method will return an error if WebAssembly bytes have already been + /// configured. This method will also return an error if `wasm_bytes` is the + /// wasm text format and the text syntax is not valid. + pub fn wasm_binary_or_text( + &mut self, + wasm_bytes: &'a [u8], + wasm_path: Option<&'a Path>, + ) -> Result<&mut Self> { + #[cfg(feature = "wat")] + let wasm_bytes = wat::parse_bytes(wasm_bytes).map_err(|mut e| { + if let Some(path) = wasm_path { + e.set_path(path); + } + e + })?; + self.wasm_binary(wasm_bytes, wasm_path) } /// Reads the `file` specified for the WebAssembly bytes that are going to /// be compiled. /// /// This method will read `file` from the filesystem and interpret it - /// either as a WebAssembly binary or as a WebAssembly text file. The - /// contents are inspected to do this, the file extension is not consulted. + /// as a WebAssembly binary. /// /// A DWARF package file will be probed using the root of `file` and with a - /// `.dwp` extension. If found, it will be loaded and DWARF fusion + /// `.dwp` extension. If found, it will be loaded and DWARF fusion /// performed. /// /// # Errors /// - /// If wasm bytes have already been configured via a call to this method or - /// [`CodeBuilder::wasm`] then an error will be returned. + /// This method will return an error if WebAssembly bytes have already been + /// configured. /// /// If `file` can't be read or an error happens reading it then that will /// also be returned. /// /// If DWARF fusion is performed and the DWARF packaged file cannot be read /// then an error will be returned. - pub fn wasm_file(&mut self, file: &'a Path) -> Result<&mut Self> { - if self.wasm.is_some() { - bail!("cannot call `wasm` or `wasm_file` twice"); - } + pub fn wasm_binary_file(&mut self, file: &'a Path) -> Result<&mut Self> { let wasm = std::fs::read(file) .with_context(|| format!("failed to read input file: {}", file.display()))?; - self.wasm = Some(wasm.into()); - self.wasm_path = Some(file.into()); - self.dwarf_package_from_wasm_path()?; - - Ok(self) + self.wasm_binary(wasm, Some(file)) } - pub(super) fn wasm_binary(&self) -> Result> { - let wasm = self - .wasm - .as_ref() - .ok_or_else(|| anyhow!("no wasm bytes have been configured"))?; - if self.wat { - #[cfg(feature = "wat")] - return wat::parse_bytes(wasm).map_err(|mut e| { - if let Some(path) = &self.wasm_path { - e.set_path(path); - } - e.into() - }); + /// Equivalent of [`CodeBuilder::wasm_binary_file`] that also accepts the + /// WebAssembly text format. + /// + /// This method is will read the file at `path` and interpret the contents + /// to determine if it's the wasm text format or binary format. The file + /// extension of `file` is not consulted. The text format is automatically + /// converted to the binary format if the crate feature `wat` is active. + /// + /// # Errors + /// + /// In addition to the errors returned by [`CodeBuilder::wasm_binary_file`] + /// this may also fail if the text format is read and the syntax is invalid. + pub fn wasm_binary_or_text_file(&mut self, file: &'a Path) -> Result<&mut Self> { + #[cfg(feature = "wat")] + { + let wasm = wat::parse_file(file)?; + self.wasm_binary(wasm, Some(file)) + } + #[cfg(not(feature = "wat"))] + { + self.wasm_binary_file(file) } - Ok((&wasm[..]).into()) + } + + pub(super) fn get_wasm(&self) -> Result<&[u8]> { + self.wasm + .as_deref() + .ok_or_else(|| anyhow!("no wasm bytes have been configured")) } /// Explicitly specify DWARF `.dwp` path. @@ -163,7 +187,7 @@ impl<'a> CodeBuilder<'a> { /// /// This method will return an error if the `.dwp` file has already been set /// through [`CodeBuilder::dwarf_package`] or auto-detection in - /// [`CodeBuilder::wasm_file`]. + /// [`CodeBuilder::wasm_binary_file`]. /// /// This method will also return an error if `file` cannot be read. pub fn dwarf_package_file(&mut self, file: &Path) -> Result<&mut Self> { @@ -189,8 +213,8 @@ impl<'a> CodeBuilder<'a> { } /// Gets the DWARF package. - pub(super) fn dwarf_package_binary(&self) -> Option<&[u8]> { - return self.dwarf_package.as_deref(); + pub(super) fn get_dwarf_package(&self) -> Option<&[u8]> { + self.dwarf_package.as_deref() } /// Set the DWARF package binary. @@ -202,7 +226,7 @@ impl<'a> CodeBuilder<'a> { /// # Errors /// /// Returns an error if the `*.dwp` file is already set via auto-probing in - /// [`CodeBuilder::wasm_file`] or explicitly via + /// [`CodeBuilder::wasm_binary_file`] or explicitly via /// [`CodeBuilder::dwarf_package_file`]. pub fn dwarf_package(&mut self, dwp_bytes: &'a [u8]) -> Result<&mut Self> { if self.dwarf_package.is_some() { @@ -212,11 +236,30 @@ impl<'a> CodeBuilder<'a> { Ok(self) } + /// Returns a hint, if possible, of what the provided bytes are. + /// + /// This method can be use to detect what the previously supplied bytes to + /// methods such as [`CodeBuilder::wasm_binary_or_text`] are. This will + /// return whether a module or a component was found in the provided bytes. + /// + /// This method will return `None` if wasm bytes have not been configured + /// or if the provided bytes don't look like either a component or a + /// module. + pub fn hint(&self) -> Option { + let wasm = self.wasm.as_ref()?; + if wasmparser::Parser::is_component(wasm) { + Some(CodeHint::Component) + } else if wasmparser::Parser::is_core_wasm(wasm) { + Some(CodeHint::Module) + } else { + None + } + } + /// Finishes this compilation and produces a serialized list of bytes. /// - /// This method requires that either [`CodeBuilder::wasm`] or - /// [`CodeBuilder::wasm_file`] was invoked prior to indicate what is - /// being compiled. + /// This method requires that either [`CodeBuilder::wasm_binary`] or + /// related methods were invoked prior to indicate what is being compiled. /// /// This method will block the current thread until compilation has /// finished, and when done the serialized artifact will be returned. @@ -229,8 +272,8 @@ impl<'a> CodeBuilder<'a> { /// This can fail if the input wasm module was not valid or if another /// compilation-related error is encountered. pub fn compile_module_serialized(&self) -> Result> { - let wasm = self.wasm_binary()?; - let dwarf_package = self.dwarf_package_binary(); + let wasm = self.get_wasm()?; + let dwarf_package = self.get_dwarf_package(); let (v, _) = super::build_artifacts(self.engine, &wasm, dwarf_package.as_deref())?; Ok(v) } @@ -240,7 +283,7 @@ impl<'a> CodeBuilder<'a> { /// instead of a module. #[cfg(feature = "component-model")] pub fn compile_component_serialized(&self) -> Result> { - let bytes = self.wasm_binary()?; + let bytes = self.get_wasm()?; let (v, _) = super::build_component_artifacts(self.engine, &bytes, None)?; Ok(v) } diff --git a/crates/wasmtime/src/compile/runtime.rs b/crates/wasmtime/src/compile/runtime.rs index 3d880c61c293..57ae9c7960e9 100644 --- a/crates/wasmtime/src/compile/runtime.rs +++ b/crates/wasmtime/src/compile/runtime.rs @@ -13,8 +13,8 @@ impl<'a> CodeBuilder<'a> { &self, build_artifacts: fn(&Engine, &[u8], Option<&[u8]>) -> Result<(MmapVecWrapper, Option)>, ) -> Result<(Arc, Option)> { - let wasm = self.wasm_binary()?; - let dwarf_package = self.dwarf_package_binary(); + let wasm = self.get_wasm()?; + let dwarf_package = self.get_dwarf_package(); self.engine .check_compatible_with_native_host() diff --git a/crates/wasmtime/src/engine.rs b/crates/wasmtime/src/engine.rs index 2d5a3a7de915..36a90cf86ad7 100644 --- a/crates/wasmtime/src/engine.rs +++ b/crates/wasmtime/src/engine.rs @@ -494,7 +494,7 @@ impl Engine { /// [text]: https://webassembly.github.io/spec/core/text/index.html pub fn precompile_module(&self, bytes: &[u8]) -> Result> { crate::CodeBuilder::new(self) - .wasm(bytes, None)? + .wasm_binary_or_text(bytes, None)? .compile_module_serialized() } @@ -503,7 +503,7 @@ impl Engine { #[cfg(feature = "component-model")] pub fn precompile_component(&self, bytes: &[u8]) -> Result> { crate::CodeBuilder::new(self) - .wasm(bytes, None)? + .wasm_binary_or_text(bytes, None)? .compile_component_serialized() } diff --git a/crates/wasmtime/src/lib.rs b/crates/wasmtime/src/lib.rs index 69f2a8a3ceb3..9a8139fb82f4 100644 --- a/crates/wasmtime/src/lib.rs +++ b/crates/wasmtime/src/lib.rs @@ -363,7 +363,7 @@ pub use runtime::*; #[cfg(any(feature = "cranelift", feature = "winch"))] mod compile; #[cfg(any(feature = "cranelift", feature = "winch"))] -pub use compile::CodeBuilder; +pub use compile::{CodeBuilder, CodeHint}; mod config; mod engine; diff --git a/crates/wasmtime/src/runtime/component/component.rs b/crates/wasmtime/src/runtime/component/component.rs index 7dd4eca08b56..e85c277e1068 100644 --- a/crates/wasmtime/src/runtime/component/component.rs +++ b/crates/wasmtime/src/runtime/component/component.rs @@ -161,7 +161,7 @@ impl Component { #[cfg(any(feature = "cranelift", feature = "winch"))] pub fn new(engine: &Engine, bytes: impl AsRef<[u8]>) -> Result { crate::CodeBuilder::new(engine) - .wasm(bytes.as_ref(), None)? + .wasm_binary_or_text(bytes.as_ref(), None)? .compile_component() } @@ -173,7 +173,7 @@ impl Component { #[cfg(all(feature = "std", any(feature = "cranelift", feature = "winch")))] pub fn from_file(engine: &Engine, file: impl AsRef) -> Result { crate::CodeBuilder::new(engine) - .wasm_file(file.as_ref())? + .wasm_binary_or_text_file(file.as_ref())? .compile_component() } @@ -189,8 +189,7 @@ impl Component { #[cfg(any(feature = "cranelift", feature = "winch"))] pub fn from_binary(engine: &Engine, binary: &[u8]) -> Result { crate::CodeBuilder::new(engine) - .wasm(binary, None)? - .wat(false)? + .wasm_binary(binary, None)? .compile_component() } diff --git a/crates/wasmtime/src/runtime/module.rs b/crates/wasmtime/src/runtime/module.rs index 128355ad0678..abb96227e066 100644 --- a/crates/wasmtime/src/runtime/module.rs +++ b/crates/wasmtime/src/runtime/module.rs @@ -243,7 +243,7 @@ impl Module { #[cfg(any(feature = "cranelift", feature = "winch"))] pub fn new(engine: &Engine, bytes: impl AsRef<[u8]>) -> Result { crate::CodeBuilder::new(engine) - .wasm(bytes.as_ref(), None)? + .wasm_binary_or_text(bytes.as_ref(), None)? .compile_module() } @@ -278,7 +278,7 @@ impl Module { #[cfg(all(feature = "std", any(feature = "cranelift", feature = "winch")))] pub fn from_file(engine: &Engine, file: impl AsRef) -> Result { crate::CodeBuilder::new(engine) - .wasm_file(file.as_ref())? + .wasm_binary_or_text_file(file.as_ref())? .compile_module() } @@ -316,8 +316,7 @@ impl Module { #[cfg(any(feature = "cranelift", feature = "winch"))] pub fn from_binary(engine: &Engine, binary: &[u8]) -> Result { crate::CodeBuilder::new(engine) - .wasm(binary, None)? - .wat(false)? + .wasm_binary(binary, None)? .compile_module() } @@ -351,7 +350,7 @@ impl Module { } crate::CodeBuilder::new(engine) - .wasm(&mmap, Some(file.as_ref()))? + .wasm_binary_or_text(&mmap[..], Some(file.as_ref()))? .compile_module() } diff --git a/src/commands/compile.rs b/src/commands/compile.rs index 556850977ce6..ab536ff371e6 100644 --- a/src/commands/compile.rs +++ b/src/commands/compile.rs @@ -5,7 +5,7 @@ use clap::Parser; use once_cell::sync::Lazy; use std::fs; use std::path::PathBuf; -use wasmtime::Engine; +use wasmtime::{CodeBuilder, CodeHint, Engine}; use wasmtime_cli_flags::CommonOptions; static AFTER_HELP: Lazy = Lazy::new(|| { @@ -87,11 +87,8 @@ impl CompileCommand { ); } - #[cfg(feature = "wat")] - let input = wat::parse_file(&self.module).with_context(|| "failed to read input file")?; - #[cfg(not(feature = "wat"))] - let input = std::fs::read(&self.module) - .with_context(|| format!("failed to read input file: {:?}", self.module))?; + let mut code = CodeBuilder::new(&engine); + code.wasm_binary_or_text_file(&self.module)?; let output = self.output.take().unwrap_or_else(|| { let mut output: PathBuf = self.module.file_name().unwrap().into(); @@ -99,19 +96,14 @@ impl CompileCommand { output }); - let output_bytes = if wasmparser::Parser::is_component(&input) { + let output_bytes = match code.hint() { #[cfg(feature = "component-model")] - { - engine.precompile_component(&input)? - } + Some(CodeHint::Component) => code.compile_component_serialized()?, #[cfg(not(feature = "component-model"))] - { + Some(CodeHint::Component) => { bail!("component model support was disabled at compile time") } - } else { - wasmtime::CodeBuilder::new(&engine) - .wasm(&input, Some(&self.module))? - .compile_module_serialized()? + Some(CodeHint::Module) | None => code.compile_module_serialized()?, }; fs::write(&output, output_bytes) .with_context(|| format!("failed to write output: {}", output.display()))?; diff --git a/src/common.rs b/src/common.rs index 47ebef7834ac..79837d79071c 100644 --- a/src/common.rs +++ b/src/common.rs @@ -227,33 +227,26 @@ impl RunCommon { } #[cfg(any(feature = "cranelift", feature = "winch"))] None => { - // Parse the text format here specifically to add the `path` to - // the error message if there's a syntax error. - #[cfg(feature = "wat")] - let bytes = wat::parse_bytes(bytes).map_err(|mut e| { - e.set_path(path); - e - })?; - if wasmparser::Parser::is_component(&bytes) { - #[cfg(feature = "component-model")] - { - self.ensure_allow_components()?; - let component = wasmtime::CodeBuilder::new(engine) - .wasm(&bytes, Some(path))? - .compile_component()?; - RunTarget::Component(component) + let mut code = wasmtime::CodeBuilder::new(engine); + code.wasm_binary_or_text(bytes, Some(path))?; + match code.hint() { + Some(wasmtime::CodeHint::Component) => { + #[cfg(feature = "component-model")] + { + self.ensure_allow_components()?; + RunTarget::Component(code.compile_component()?) + } + #[cfg(not(feature = "component-model"))] + { + bail!("support for components was not enabled at compile time"); + } } - #[cfg(not(feature = "component-model"))] - { - bail!("support for components was not enabled at compile time"); + Some(wasmtime::CodeHint::Module) | None => { + RunTarget::Core(code.compile_module()?) } - } else { - let module = wasmtime::CodeBuilder::new(engine) - .wasm(&bytes, Some(path))? - .compile_module()?; - RunTarget::Core(module) } } + #[cfg(not(any(feature = "cranelift", feature = "winch")))] None => { let _ = path; diff --git a/tests/all/debug/obj.rs b/tests/all/debug/obj.rs index e44677c834bf..4395da771b9d 100644 --- a/tests/all/debug/obj.rs +++ b/tests/all/debug/obj.rs @@ -18,7 +18,7 @@ pub fn compile_cranelift( } let engine = Engine::new(&config)?; let module = CodeBuilder::new(&engine) - .wasm(wasm, path)? + .wasm_binary_or_text(wasm, path)? .compile_module()?; let bytes = module.serialize()?;