Linker refactoring (#1773)

* Minor code tidying.

* Document that `Linker::iter`'s iteration order is arbitrary.

* Add a few more tests for `wasmtime::Linker`.

* Refactor `Linker::compute_imports`.

 - Extract the error message generation into a separate function.
 - In the error message, sort the candidates.

* Fix a typo in a comment.

* Add `__rtti_base` to the list of allowed but deprecated exports.

* Don't print an Error message when a program exits normally.

* Update comments to reflect the current code.

* Also allow "table" as an exported table, which is used by AssemblyScript.

Cargo.lock

@@ -2170,6 +2170,7 @@ dependencies = [
  "cfg-if",
  "lazy_static",
  "libc",
+ "log",
  "region",
  "rustc-demangle",
  "target-lexicon",

crates/wasmtime/Cargo.toml

@@ -23,6 +23,7 @@ cfg-if = "0.1.9"
 backtrace = "0.3.42"
 rustc-demangle = "0.1.16"
 lazy_static = "1.4"
+log = "0.4.8"
 wat = { version = "1.0.18", optional = true }
 
 [target.'cfg(target_os = "windows")'.dependencies]

crates/wasmtime/src/linker.rs

@@ -2,7 +2,8 @@ use crate::{
     Extern, ExternType, Func, FuncType, GlobalType, ImportType, Instance, IntoFunc, Module, Store,
     Trap,
 };
-use anyhow::{anyhow, bail, Context, Result};
+use anyhow::{anyhow, bail, Context, Error, Result};
+use log::warn;
 use std::collections::hash_map::{Entry, HashMap};
 use std::rc::Rc;
 
@@ -403,11 +404,10 @@ impl Linker {
                 let export_name = export.name().to_owned();
                 let func = Func::new(&self.store, func_ty.clone(), move |_, params, results| {
                     // Create a new instance for this command execution.
-                    let instance = Instance::new(&module, &imports).map_err(|error| match error
+                    let instance = Instance::new(&module, &imports).map_err(|error| {
-                        .downcast::<Trap>()
+                        error
-                    {
+                            .downcast::<Trap>()
-                        Ok(trap) => trap,
+                            .unwrap_or_else(|error| Trap::new(format!("{:?}", error)))
-                        Err(error) => Trap::new(format!("{:?}", error)),
                     })?;
 
                     // `unwrap()` everything here because we know the instance contains a
@@ -436,14 +436,22 @@ impl Linker {
             } else if export.name() == "__indirect_function_table" && export.ty().table().is_some()
             {
                 // Allow an exported "__indirect_function_table" table for now.
+            } else if export.name() == "table" && export.ty().table().is_some() {
+                // Allow an exported "table" table for now.
             } else if export.name() == "__data_end" && export.ty().global().is_some() {
                 // Allow an exported "__data_end" memory for compatibility with toolchains
                 // which use --export-dynamic, which unfortunately doesn't work the way
                 // we want it to.
+                warn!("command module exporting '__data_end' is deprecated");
             } else if export.name() == "__heap_base" && export.ty().global().is_some() {
                 // Allow an exported "__data_end" memory for compatibility with toolchains
                 // which use --export-dynamic, which unfortunately doesn't work the way
                 // we want it to.
+                warn!("command module exporting '__heap_base' is deprecated");
+            } else if export.name() == "__rtti_base" && export.ty().global().is_some() {
+                // Allow an exported "__rtti_base" memory for compatibility with
+                // AssemblyScript.
+                warn!("command module exporting '__rtti_base' is deprecated; pass `--runtime half` to the AssemblyScript compiler");
             } else {
                 bail!("command export '{}' is not a function", export.name());
             }
@@ -529,7 +537,7 @@ impl Linker {
     ///
     /// Each import of `module` will be looked up in this [`Linker`] and must
     /// have previously been defined. If it was previously defined with an
-    /// incorrect signature or if it was not prevoiusly defined then an error
+    /// incorrect signature or if it was not previously defined then an error
     /// will be returned because the import can not be satisfied.
     ///
     /// Per the WebAssembly spec, instantiation includes running the module's
@@ -568,45 +576,41 @@ impl Linker {
     }
 
     fn compute_imports(&self, module: &Module) -> Result<Vec<Extern>> {
-        let mut imports = Vec::new();
+        module
+            .imports()
+            .map(|import| self.get(&import).ok_or_else(|| self.link_error(&import)))
+            .collect()
+    }
 
-        for import in module.imports() {
+    fn link_error(&self, import: &ImportType) -> Error {
-            if let Some(item) = self.get(&import) {
+        let mut options = Vec::new();
-                imports.push(item);
+        for i in self.map.keys() {
+            if &*self.strings[i.module] != import.module()
+                || &*self.strings[i.name] != import.name()
+            {
                 continue;
             }
-
+            options.push(format!("  * {:?}\n", i.kind));
-            let mut options = String::new();
+        }
-            for i in self.map.keys() {
+        if options.is_empty() {
-                if &*self.strings[i.module] != import.module()
+            return anyhow!(
-                    || &*self.strings[i.name] != import.name()
+                "unknown import: `{}::{}` has not been defined",
-                {
-                    continue;
-                }
-                options.push_str("  * ");
-                options.push_str(&format!("{:?}", i.kind));
-                options.push_str("\n");
-            }
-            if options.len() == 0 {
-                bail!(
-                    "unknown import: `{}::{}` has not been defined",
-                    import.module(),
-                    import.name()
-                )
-            }
-
-            bail!(
-                "incompatible import type for `{}::{}` specified\n\
-                 desired signature was: {:?}\n\
-                 signatures available:\n\n{}",
                 import.module(),
-                import.name(),
+                import.name()
-                import.ty(),
+            );
-                options,
-            )
         }
 
-        Ok(imports)
+        options.sort();
+
+        anyhow!(
+            "incompatible import type for `{}::{}` specified\n\
+                 desired signature was: {:?}\n\
+                 signatures available:\n\n{}",
+            import.module(),
+            import.name(),
+            import.ty(),
+            options.concat(),
+        )
     }
 
     /// Returns the [`Store`] that this linker is connected to.
@@ -614,7 +618,7 @@ impl Linker {
         &self.store
     }
 
-    /// Returns an iterator over all items defined in this `Linker`.
+    /// Returns an iterator over all items defined in this `Linker`, in arbitrary order.
     ///
     /// The iterator returned will yield 3-tuples where the first two elements
     /// are the module name and item name for the external item, and the third
@@ -716,15 +720,14 @@ impl Linker {
     }
 }
 
-/// Modules can be interpreted either as Commands (instance lifetime ends
+/// Modules can be interpreted either as Commands or Reactors.
-/// when the start function returns) or Reactor (instance persists).
 enum ModuleKind {
-    /// The instance is a Command, and this is its start function. The
+    /// The instance is a Command, meaning an instance is created for each
-    /// instance should be consumed.
+    /// exported function and lives for the duration of the function call.
     Command,
 
-    /// The instance is a Reactor, and this is its initialization function,
+    /// The instance is a Reactor, meaning one instance is created which
-    /// along with the instance itself, which should persist.
+    /// may live across multiple calls.
     Reactor,
 }
 

src/commands/run.rs

@@ -162,8 +162,6 @@ impl RunCommand {
                 // a message and exit.
                 if let Some(trap) = e.downcast_ref::<Trap>() {
                     // Print the error message in the usual way.
-                    eprintln!("Error: {:?}", e);
-
                     if let Some(status) = trap.i32_exit_status() {
                         // On Windows, exit status 3 indicates an abort (see below),
                         // so return 1 indicating a non-zero status to avoid ambiguity.
@@ -173,6 +171,8 @@ impl RunCommand {
                         process::exit(status);
                     }
 
+                    eprintln!("Error: {:?}", e);
+
                     // If the program exited because of a trap, return an error code
                     // to the outside environment indicating a more severe problem
                     // than a simple failure.

tests/all/linker.rs

@@ -66,7 +66,40 @@ fn link_twice_bad() -> Result<()> {
 }
 
 #[test]
-fn interposition() -> Result<()> {
+fn function_interposition() -> Result<()> {
+    let store = Store::default();
+    let mut linker = Linker::new(&store);
+    linker.allow_shadowing(true);
+    let mut module = Module::new(
+        &store,
+        r#"(module (func (export "green") (result i32) (i32.const 7)))"#,
+    )?;
+    for _ in 0..4 {
+        let instance = linker.instantiate(&module)?;
+        linker.define(
+            "red",
+            "green",
+            instance.get_export("green").unwrap().clone(),
+        )?;
+        module = Module::new(
+            &store,
+            r#"(module
+                (import "red" "green" (func (result i32)))
+                (func (export "green") (result i32) (i32.mul (call 0) (i32.const 2)))
+            )"#,
+        )?;
+    }
+    let instance = linker.instantiate(&module)?;
+    let func = instance.get_export("green").unwrap().into_func().unwrap();
+    let func = func.get0::<i32>()?;
+    assert_eq!(func()?, 112);
+    Ok(())
+}
+
+// Same as `function_interposition`, but the linker's name for the function
+// differs from the module's name.
+#[test]
+fn function_interposition_renamed() -> Result<()> {
     let store = Store::default();
     let mut linker = Linker::new(&store);
     linker.allow_shadowing(true);
@@ -95,3 +128,32 @@ fn interposition() -> Result<()> {
     assert_eq!(func()?, 112);
     Ok(())
 }
+
+// Similar to `function_interposition`, but use `Linker::instance` instead of
+// `Linker::define`.
+#[test]
+fn module_interposition() -> Result<()> {
+    let store = Store::default();
+    let mut linker = Linker::new(&store);
+    linker.allow_shadowing(true);
+    let mut module = Module::new(
+        &store,
+        r#"(module (func (export "export") (result i32) (i32.const 7)))"#,
+    )?;
+    for _ in 0..4 {
+        let instance = linker.instantiate(&module)?;
+        linker.instance("instance", &instance)?;
+        module = Module::new(
+            &store,
+            r#"(module
+                (import "instance" "export" (func (result i32)))
+                (func (export "export") (result i32) (i32.mul (call 0) (i32.const 2)))
+            )"#,
+        )?;
+    }
+    let instance = linker.instantiate(&module)?;
+    let func = instance.get_export("export").unwrap().into_func().unwrap();
+    let func = func.get0::<i32>()?;
+    assert_eq!(func()?, 112);
+    Ok(())
+}