From a301202b7d91e73b2ba393f3817c4ab1d1392cbb Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 29 Mar 2021 17:26:02 -0500 Subject: [PATCH] Remove the type-driven ability for duplicates in a `Linker` (#2789) When `Linker` was first created it was attempted to be created with the ability to instantiate any wasm modules, including those with duplicate import strings of different types. In an effort to support this a `Linker` supports defining the same names twice so long as they're defined with differently-typed values. This ended up causing wast testsuite failures module linking is enabled, however, because the wrong error message is returned. While it would be possible to fix this there's already the possibility for confusing error messages today due to the `Linker` trying to take on this type-level complexity. In a way this is yet-another type checker for wasm imports, but sort of a bad one because it only supports things like globals/functions, and otherwise you can only define one `Memory`, for example, with a particular name. This commit completely removes this feature from `Linker` to simplify the implementation and make error messages more straightforward. This means that any error message coming from a `Linker` is purely "this thing wasn't defined" rather than a hybrid of "maybe the types didn't match?". I think this also better aligns with the direction that we see conventional wasm modules going which is that duplicate imports are not ever present. --- crates/wasmtime/src/linker.rs | 81 +++++------------------------------ tests/all/linker.rs | 31 +++++++------- 2 files changed, 26 insertions(+), 86 deletions(-) diff --git a/crates/wasmtime/src/linker.rs b/crates/wasmtime/src/linker.rs index 153ecd75dfd9..cd2be6a32690 100644 --- a/crates/wasmtime/src/linker.rs +++ b/crates/wasmtime/src/linker.rs @@ -1,7 +1,6 @@ use crate::instance::InstanceBuilder; use crate::{ - Extern, ExternType, Func, FuncType, GlobalType, ImportType, Instance, IntoFunc, Module, Store, - Trap, + Extern, ExternType, Func, FuncType, ImportType, Instance, IntoFunc, Module, Store, Trap, }; use anyhow::{anyhow, bail, Context, Error, Result}; use log::warn; @@ -29,15 +28,9 @@ use std::rc::Rc; /// module and then has its own name. This basically follows the wasm standard /// for modularization. /// -/// Names in a `Linker` can be defined twice, but only for different signatures -/// of items. This means that every item defined in a `Linker` has a unique -/// name/type pair. For example you can define two functions with the module -/// name `foo` and item name `bar`, so long as they have different function -/// signatures. Currently duplicate memories and tables are not allowed, only -/// one-per-name is allowed. -/// -/// Note that allowing duplicates by shadowing the previous definition can be -/// controlled with the [`Linker::allow_shadowing`] method as well. +/// Names in a `Linker` cannot be defined twice, but allowing duplicates by +/// shadowing the previous definition can be controlled with the +/// [`Linker::allow_shadowing`] method. pub struct Linker { store: Store, string2idx: HashMap, usize>, @@ -50,17 +43,6 @@ pub struct Linker { struct ImportKey { name: usize, module: usize, - kind: ImportKind, -} - -#[derive(Hash, PartialEq, Eq, Debug)] -enum ImportKind { - Func(FuncType), - Global(GlobalType), - Memory, - Table, - Module, - Instance, } impl Linker { @@ -517,17 +499,15 @@ impl Linker { } fn insert(&mut self, module: &str, name: Option<&str>, item: Extern) -> Result<()> { - let key = self.import_key(module, name, item.ty()); + let key = self.import_key(module, name); let desc = || match name { Some(name) => format!("{}::{}", module, name), None => module.to_string(), }; match self.map.entry(key) { - Entry::Occupied(o) if !self.allow_shadowing => bail!( - "import of `{}` with kind {:?} defined twice", - desc(), - o.key().kind, - ), + Entry::Occupied(_) if !self.allow_shadowing => { + bail!("import of `{}` defined twice", desc(),) + } Entry::Occupied(mut o) => { o.insert(item); } @@ -537,11 +517,7 @@ impl Linker { if let Extern::Func(_) = &item { if let Some(name) = name { if self.store.get_host_func(module, name).is_some() { - bail!( - "import of `{}` with kind {:?} defined twice", - desc(), - v.key().kind, - ) + bail!("import of `{}` defined twice", desc(),) } } } @@ -552,24 +528,12 @@ impl Linker { Ok(()) } - fn import_key(&mut self, module: &str, name: Option<&str>, ty: ExternType) -> ImportKey { + fn import_key(&mut self, module: &str, name: Option<&str>) -> ImportKey { ImportKey { module: self.intern_str(module), name: name .map(|name| self.intern_str(name)) .unwrap_or(usize::max_value()), - kind: self.import_kind(ty), - } - } - - fn import_kind(&self, ty: ExternType) -> ImportKind { - match ty { - ExternType::Func(f) => ImportKind::Func(f), - ExternType::Global(f) => ImportKind::Global(f), - ExternType::Memory(_) => ImportKind::Memory, - ExternType::Table(_) => ImportKind::Table, - ExternType::Module(_) => ImportKind::Module, - ExternType::Instance(_) => ImportKind::Instance, } } @@ -649,33 +613,11 @@ impl Linker { } fn link_error(&self, import: &ImportType) -> Error { - let mut options = Vec::new(); - for i in self.map.keys() { - if &*self.strings[i.module] != import.module() - || self.strings.get(i.name).map(|s| &**s) != import.name() - { - continue; - } - options.push(format!(" * {:?}\n", i.kind)); - } let desc = match import.name() { Some(name) => format!("{}::{}", import.module(), name), None => import.module().to_string(), }; - if options.is_empty() { - return anyhow!("unknown import: `{}` has not been defined", desc); - } - - options.sort(); - - anyhow!( - "incompatible import type for `{}` specified\n\ - desired signature was: {:?}\n\ - signatures available:\n\n{}", - desc, - import.ty(), - options.concat(), - ) + anyhow!("unknown import: `{}` has not been defined", desc) } /// Returns the [`Store`] that this linker is connected to. @@ -829,7 +771,6 @@ impl Linker { Some(name) => *self.string2idx.get(name)?, None => usize::max_value(), }, - kind: self.import_kind(import.ty()), }; self.map.get(&key).cloned() } diff --git a/tests/all/linker.rs b/tests/all/linker.rs index 078509774d56..e3d21e96bb2d 100644 --- a/tests/all/linker.rs +++ b/tests/all/linker.rs @@ -27,46 +27,45 @@ fn link_twice_bad() -> Result<()> { let mut linker = Linker::new(&store); // functions - linker.func("", "", || {})?; - assert!(linker.func("", "", || {}).is_err()); + linker.func("f", "", || {})?; + assert!(linker.func("f", "", || {}).is_err()); assert!(linker - .func("", "", || -> Result<(), Trap> { loop {} }) + .func("f", "", || -> Result<(), Trap> { loop {} }) .is_err()); - linker.func("", "", |_: i32| {})?; // globals let ty = GlobalType::new(ValType::I32, Mutability::Const); let global = Global::new(&store, ty, Val::I32(0))?; - linker.define("", "", global.clone())?; - assert!(linker.define("", "", global.clone()).is_err()); + linker.define("g", "1", global.clone())?; + assert!(linker.define("g", "1", global.clone()).is_err()); let ty = GlobalType::new(ValType::I32, Mutability::Var); let global = Global::new(&store, ty, Val::I32(0))?; - linker.define("", "", global.clone())?; - assert!(linker.define("", "", global.clone()).is_err()); + linker.define("g", "2", global.clone())?; + assert!(linker.define("g", "2", global.clone()).is_err()); let ty = GlobalType::new(ValType::I64, Mutability::Const); let global = Global::new(&store, ty, Val::I64(0))?; - linker.define("", "", global.clone())?; - assert!(linker.define("", "", global.clone()).is_err()); + linker.define("g", "3", global.clone())?; + assert!(linker.define("g", "3", global.clone()).is_err()); // memories let ty = MemoryType::new(Limits::new(1, None)); let memory = Memory::new(&store, ty); - linker.define("", "", memory.clone())?; - assert!(linker.define("", "", memory.clone()).is_err()); + linker.define("m", "", memory.clone())?; + assert!(linker.define("m", "", memory.clone()).is_err()); let ty = MemoryType::new(Limits::new(2, None)); let memory = Memory::new(&store, ty); - assert!(linker.define("", "", memory.clone()).is_err()); + assert!(linker.define("m", "", memory.clone()).is_err()); // tables let ty = TableType::new(ValType::FuncRef, Limits::new(1, None)); let table = Table::new(&store, ty, Val::FuncRef(None))?; - linker.define("", "", table.clone())?; - assert!(linker.define("", "", table.clone()).is_err()); + linker.define("t", "", table.clone())?; + assert!(linker.define("t", "", table.clone()).is_err()); let ty = TableType::new(ValType::FuncRef, Limits::new(2, None)); let table = Table::new(&store, ty, Val::FuncRef(None))?; - assert!(linker.define("", "", table.clone()).is_err()); + assert!(linker.define("t", "", table.clone()).is_err()); Ok(()) }