Skip to content

Commit

Permalink
Require wasmtime options are first when running modules (#6737)
Browse files Browse the repository at this point in the history
* Require wasmtime options are first when running modules

Currently the way we've configured argument parsing it's valid to
execute a command such as:

    wasmtime run foo.wasm -O

which is the same as:

    wasmtime run -O foo.wasm

or otherwise all flags are attempted to be parsed as Wasmtime flags and
an error is generated when they're not wasmtime flags. I've personally
found this a bit confusing in the past and I find myself frequently
executing:

    wasmtime run -- foo.wasm -other -arguments

While this works my general impression is that many other "wrapper
commands" don't behave this way and typically don't require `--` to pass
flags to the target executable. This commit reconfigures argument
parsing to consider any argument after the WebAssembly module itself to
be an argument to the wasm program rather than an argument to Wasmtime.
This means that all Wasmtime options must come between the `run` command
and the `foo.wasm` WebAssembly argument.

* Update wasi testsuite runner

* Support `wasmtime -- run`

Additionally use more clap features to avoid manually checking against
subcommands.

* Remove stale comment

* Reorder wasi-nn arguments

* Reorder more flags

* Fix unused import on Windows

* Don't run a stdio test on Windows

* Update gdb/lldb tests

* Don't assert that the write succeeds

prtest:full
  • Loading branch information
alexcrichton authored Jul 20, 2023
1 parent 329a2ba commit 8091556
Show file tree
Hide file tree
Showing 10 changed files with 339 additions and 138 deletions.
2 changes: 1 addition & 1 deletion ci/run-wasi-crypto-example.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ pushd "$RUST_BINDINGS"
cargo build --release --target=wasm32-wasi
popd

cargo run --features wasi-crypto -- run "$RUST_BINDINGS/target/wasm32-wasi/release/wasi-crypto-guest.wasm" --wasi-modules=experimental-wasi-crypto
cargo run --features wasi-crypto -- run --wasi-modules=experimental-wasi-crypto "$RUST_BINDINGS/target/wasm32-wasi/release/wasi-crypto-guest.wasm"
2 changes: 1 addition & 1 deletion ci/run-wasi-nn-example.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ cp target/wasm32-wasi/release/wasi-nn-example.wasm $TMP_DIR
popd

# Run the example in Wasmtime (note that the example uses `fixture` as the expected location of the model/tensor files).
cargo run -- run --mapdir fixture::$TMP_DIR $TMP_DIR/wasi-nn-example.wasm --wasi-modules=experimental-wasi-nn
cargo run -- run --mapdir fixture::$TMP_DIR --wasi-modules=experimental-wasi-nn $TMP_DIR/wasi-nn-example.wasm

# Clean up the temporary directory only if it was not specified (users may want to keep the directory around).
if [[ $REMOVE_TMP_DIR -eq 1 ]]; then
Expand Down
46 changes: 27 additions & 19 deletions src/bin/wasmtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
//! See `wasmtime --help` for usage.
use anyhow::Result;
use clap::{error::ErrorKind, Parser};
use clap::Parser;
use wasmtime_cli::commands::{
CompileCommand, ConfigCommand, ExploreCommand, RunCommand, SettingsCommand, WastCommand,
};
Expand All @@ -27,10 +27,24 @@ use wasmtime_cli::commands::{
\n\
Invoking a specific function (e.g. `add`) in a WebAssembly module:\n\
\n \
wasmtime example.wasm --invoke add 1 2\n"
wasmtime example.wasm --invoke add 1 2\n",
// This option enables the pattern below where we ask clap to parse twice
// sorta: once where it's trying to find a subcommand and once assuming
// a subcommand doesn't get passed. Clap should then, apparently,
// fill in the `subcommand` if found and otherwise fill in the
// `RunCommand`.
args_conflicts_with_subcommands = true
)]
enum Wasmtime {
// !!! IMPORTANT: if subcommands are added or removed, update `parse_module` in `src/commands/run.rs`. !!!
struct Wasmtime {
#[clap(subcommand)]
subcommand: Option<Subcommand>,
#[clap(flatten)]
run: RunCommand,
}

#[derive(Parser)]
enum Subcommand {
/// Controls Wasmtime configuration settings
Config(ConfigCommand),
/// Compiles a WebAssembly module.
Expand All @@ -48,26 +62,20 @@ enum Wasmtime {
impl Wasmtime {
/// Executes the command.
pub fn execute(self) -> Result<()> {
match self {
Self::Config(c) => c.execute(),
Self::Compile(c) => c.execute(),
Self::Explore(c) => c.execute(),
Self::Run(c) => c.execute(),
Self::Settings(c) => c.execute(),
Self::Wast(c) => c.execute(),
let subcommand = self.subcommand.unwrap_or(Subcommand::Run(self.run));
match subcommand {
Subcommand::Config(c) => c.execute(),
Subcommand::Compile(c) => c.execute(),
Subcommand::Explore(c) => c.execute(),
Subcommand::Run(c) => c.execute(),
Subcommand::Settings(c) => c.execute(),
Subcommand::Wast(c) => c.execute(),
}
}
}

fn main() -> Result<()> {
Wasmtime::try_parse()
.unwrap_or_else(|e| match e.kind() {
ErrorKind::InvalidSubcommand | ErrorKind::UnknownArgument => {
Wasmtime::Run(RunCommand::parse())
}
_ => e.exit(),
})
.execute()
Wasmtime::parse().execute()
}

#[test]
Expand Down
106 changes: 50 additions & 56 deletions src/commands/run.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
//! The module that implements the `wasmtime run` command.
use anyhow::{anyhow, bail, Context as _, Result};
use clap::builder::{OsStringValueParser, TypedValueParser};
use clap::Parser;
use once_cell::sync::Lazy;
use std::ffi::OsStr;
use std::ffi::OsString;
use std::fs::File;
use std::io::Write;
use std::path::{Component, Path, PathBuf};
use std::path::{Path, PathBuf};
use std::thread;
use std::time::Duration;
use wasmtime::{
Expand Down Expand Up @@ -39,18 +36,6 @@ use wasmtime_wasi_threads::WasiThreadsCtx;
#[cfg(feature = "wasi-http")]
use wasmtime_wasi_http::WasiHttp;

fn parse_module(s: OsString) -> anyhow::Result<PathBuf> {
// Do not accept wasmtime subcommand names as the module name
match s.to_str() {
Some("help") | Some("config") | Some("run") | Some("wast") | Some("compile") => {
bail!("module name cannot be the same as a subcommand")
}
#[cfg(unix)]
Some("-") => Ok(PathBuf::from("/dev/stdin")),
_ => Ok(s.into()),
}
}

fn parse_env_var(s: &str) -> Result<(String, Option<String>)> {
let mut parts = s.splitn(2, '=');
Ok((
Expand Down Expand Up @@ -111,7 +96,7 @@ static AFTER_HELP: Lazy<String> = Lazy::new(|| crate::FLAG_EXPLANATIONS.to_strin

/// Runs a WebAssembly module
#[derive(Parser)]
#[structopt(name = "run", trailing_var_arg = true, after_help = AFTER_HELP.as_str())]
#[structopt(name = "run", after_help = AFTER_HELP.as_str())]
pub struct RunCommand {
#[clap(flatten)]
common: CommonOptions,
Expand Down Expand Up @@ -174,14 +159,6 @@ pub struct RunCommand {
#[clap(long = "mapdir", number_of_values = 1, value_name = "GUEST_DIR::HOST_DIR", value_parser = parse_map_dirs)]
map_dirs: Vec<(String, String)>,

/// The path of the WebAssembly module to run
#[clap(
required = true,
value_name = "MODULE",
value_parser = OsStringValueParser::new().try_map(parse_module),
)]
module: PathBuf,

/// Load the given WebAssembly module before the main module
#[clap(
long = "preload",
Expand Down Expand Up @@ -225,11 +202,6 @@ pub struct RunCommand {
#[clap(long = "coredump-on-trap", value_name = "PATH")]
coredump_on_trap: Option<String>,

// NOTE: this must come last for trailing varargs
/// The arguments to pass to the module
#[clap(value_name = "ARGS")]
module_args: Vec<String>,

/// Maximum size, in bytes, that a linear memory is allowed to reach.
///
/// Growth beyond this limit will cause `memory.grow` instructions in
Expand Down Expand Up @@ -261,6 +233,14 @@ pub struct RunCommand {
/// memory, for example.
#[clap(long)]
trap_on_grow_failure: bool,

/// The WebAssembly module to run and arguments to pass to it.
///
/// Arguments passed to the wasm module will be configured as WASI CLI
/// arguments unless the `--invoke` CLI argument is passed in which case
/// arguments will be interpreted as arguments to the function specified.
#[clap(value_name = "WASM", trailing_var_arg = true, required = true)]
module_and_args: Vec<PathBuf>,
}

#[derive(Clone)]
Expand Down Expand Up @@ -303,13 +283,13 @@ impl RunCommand {

// Make wasi available by default.
let preopen_dirs = self.compute_preopen_dirs()?;
let argv = self.compute_argv();
let argv = self.compute_argv()?;

let mut linker = Linker::new(&engine);
linker.allow_unknown_exports(self.allow_unknown_exports);

// Read the wasm module binary either as `*.wat` or a raw binary.
let module = self.load_module(linker.engine(), &self.module)?;
let module = self.load_module(linker.engine(), &self.module_and_args[0])?;
let mut modules = vec![(String::new(), module.clone())];

let host = Host::default();
Expand Down Expand Up @@ -370,8 +350,12 @@ impl RunCommand {
// Load the main wasm module.
match self
.load_main_module(&mut store, &mut linker, module, modules, &argv[0])
.with_context(|| format!("failed to run main module `{}`", self.module.display()))
{
.with_context(|| {
format!(
"failed to run main module `{}`",
self.module_and_args[0].display()
)
}) {
Ok(()) => (),
Err(e) => {
// Exit the process if Wasmtime understands the error;
Expand Down Expand Up @@ -420,27 +404,25 @@ impl RunCommand {
Ok(listeners)
}

fn compute_argv(&self) -> Vec<String> {
fn compute_argv(&self) -> Result<Vec<String>> {
let mut result = Vec::new();

// Add argv[0], which is the program name. Only include the base name of the
// main wasm module, to avoid leaking path information.
result.push(
self.module
.components()
.next_back()
.map(Component::as_os_str)
.and_then(OsStr::to_str)
.unwrap_or("")
.to_owned(),
);

// Add the remaining arguments.
for arg in self.module_args.iter() {
result.push(arg.clone());
for (i, arg) in self.module_and_args.iter().enumerate() {
// For argv[0], which is the program name. Only include the base
// name of the main wasm module, to avoid leaking path information.
let arg = if i == 0 {
arg.components().next_back().unwrap().as_os_str()
} else {
arg.as_ref()
};
result.push(
arg.to_str()
.ok_or_else(|| anyhow!("failed to convert {arg:?} to utf-8"))?
.to_string(),
);
}

result
Ok(result)
}

fn setup_epoch_handler(
Expand Down Expand Up @@ -541,9 +523,10 @@ impl RunCommand {
}

// Use "" as a default module name.
linker
.module(&mut *store, "", &module)
.context(format!("failed to instantiate {:?}", self.module))?;
linker.module(&mut *store, "", &module).context(format!(
"failed to instantiate {:?}",
self.module_and_args[0]
))?;

// If a function to invoke was given, invoke it.
let func = if let Some(name) = &self.invoke {
Expand Down Expand Up @@ -584,7 +567,7 @@ impl RunCommand {
is experimental and may break in the future"
);
}
let mut args = self.module_args.iter();
let mut args = self.module_and_args.iter().skip(1);
let mut values = Vec::new();
for ty in ty.params() {
let val = match args.next() {
Expand All @@ -597,6 +580,9 @@ impl RunCommand {
}
}
};
let val = val
.to_str()
.ok_or_else(|| anyhow!("argument is not valid utf-8: {val:?}"))?;
values.push(match ty {
// TODO: integer parsing here should handle hexadecimal notation
// like `0x0...`, but the Rust standard library currently only
Expand All @@ -623,7 +609,9 @@ impl RunCommand {
if let Err(err) = invoke_res {
let err = if err.is::<wasmtime::Trap>() {
if let Some(coredump_path) = self.coredump_on_trap.as_ref() {
let source_name = self.module.to_str().unwrap_or_else(|| "unknown");
let source_name = self.module_and_args[0]
.to_str()
.unwrap_or_else(|| "unknown");

if let Err(coredump_err) = generate_coredump(&err, &source_name, coredump_path)
{
Expand Down Expand Up @@ -664,6 +652,12 @@ impl RunCommand {
}

fn load_module(&self, engine: &Engine, path: &Path) -> Result<Module> {
let path = match path.to_str() {
#[cfg(unix)]
Some("-") => "/dev/stdin".as_ref(),
_ => path,
};

if self.allow_precompiled {
unsafe { Module::from_trusted_file(engine, path) }
} else {
Expand Down
Loading

0 comments on commit 8091556

Please sign in to comment.