Repository: bytecodealliance/wasm-tools
Pull request #1258 (2023-10-19 23:47:24)

wit-component: use library order to determine duplicate symbol priority
This addresses a couple of todo items in the shared-everything linking code in `wit-component`.  Now, instead of erroring out on any duplicate symbols, we pick the first library in the order they were specified.


Patch:
diff --git a/crates/wit-component/src/linking.rs b/crates/wit-component/src/linking.rs
index 3af76c6a..5265dd11 100644
--- a/crates/wit-component/src/linking.rs
+++ b/crates/wit-component/src/linking.rs
@@ -836,7 +836,8 @@ fn resolve_symbols<'a>(
                     // symbol, in which case we may redundantly insert the same value.
                     resolved.insert(*key, *exporter);
                 }
-                _ => {
+                [exporter, ..] => {
+                    resolved.insert(*key, *exporter);
                     duplicates.push((metadata.name, *key, value.as_slice()));
                 }
             }
@@ -879,14 +880,15 @@ fn resolve_symbols<'a>(
     for metadata in metadata {
         for name in &metadata.table_address_imports {
             if let Some((key, value)) = function_exporters.get(name) {
+                // Note that we do not use `insert_unique` here since multiple libraries may import the same
+                // symbol, in which case we may redundantly insert the same value.
                 match value.as_slice() {
                     [] => unreachable!(),
                     [exporter] => {
-                        // Note that we do not use `insert_unique` here since multiple libraries may import the
-                        // same symbol, in which case we may redundantly insert the same value.
                         resolved.insert(key, *exporter);
                     }
-                    _ => {
+                    [exporter, ..] => {
+                        resolved.insert(key, *exporter);
                         duplicates.push((metadata.name, *key, value.as_slice()));
                     }
                 }
@@ -1158,6 +1160,8 @@ fn find_reachable<'a>(
 #[derive(Default)]
 pub struct Linker {
     /// The `(name, module, dl_openable)` triple representing the libraries to be composed
+    ///
+    /// The order of this list determines priority in cases where more than one library exports the same symbol.
     libraries: Vec<(String, Vec<u8>, bool)>,
 
     /// The set of adapters to use when generating the component
@@ -1286,14 +1290,12 @@ impl Linker {
                 }),
             })
             .map(|exporters| {
-                // TODO: When there are multiple exporters, we should choose the one that came first in the order
-                // they were specified.
                 let first = *exporters.first().unwrap();
                 *exporters = vec![first];
                 first.0
             });
 
-        let (exporters, missing, duplicates) = resolve_symbols(&metadata, &exporters);
+        let (exporters, missing, _) = resolve_symbols(&metadata, &exporters);
 
         if !missing.is_empty() {
             if missing
@@ -1324,28 +1326,6 @@ impl Linker {
             }
         }
 
-        if !duplicates.is_empty() {
-            // TODO: When there are multiple exporters for a given symbol, we should choose the one that came first
-            // in the order they were specified.
-            bail!(
-                "duplicate symbol(s):\n{}",
-                duplicates
-                    .iter()
-                    .map(|(importer, key, exporters)| {
-                        format!(
-                            "\t{importer} needs {key}, provided by {}",
-                            exporters
-                                .iter()
-                                .map(|(exporter, _)| *exporter)
-                                .collect::<Vec<_>>()
-                                .join(", ")
-                        )
-                    })
-                    .collect::<Vec<_>>()
-                    .join("\n")
-            );
-        }
-
         let dependencies = find_dependencies(&metadata, &exporters)?;
 
         {
diff --git a/crates/wit-component/tests/components/error-link-duplicate-symbols/lib-bar.wat b/crates/wit-component/tests/components/link-duplicate-symbols/lib-bar.wat
similarity index 100%
rename from crates/wit-component/tests/components/error-link-duplicate-symbols/lib-bar.wat
rename to crates/wit-component/tests/components/link-duplicate-symbols/lib-bar.wat
diff --git a/crates/wit-component/tests/components/error-link-duplicate-symbols/lib-bar.wit b/crates/wit-component/tests/components/link-duplicate-symbols/lib-bar.wit
similarity index 100%
rename from crates/wit-component/tests/components/error-link-duplicate-symbols/lib-bar.wit
rename to crates/wit-component/tests/components/link-duplicate-symbols/lib-bar.wit
diff --git a/crates/wit-component/tests/components/error-link-duplicate-symbols/lib-foo.wat b/crates/wit-component/tests/components/link-duplicate-symbols/lib-foo.wat
similarity index 100%
rename from crates/wit-component/tests/components/error-link-duplicate-symbols/lib-foo.wat
rename to crates/wit-component/tests/components/link-duplicate-symbols/lib-foo.wat
diff --git a/crates/wit-component/tests/components/error-link-duplicate-symbols/lib-foo.wit b/crates/wit-component/tests/components/link-duplicate-symbols/lib-foo.wit
similarity index 100%
rename from crates/wit-component/tests/components/error-link-duplicate-symbols/lib-foo.wit
rename to crates/wit-component/tests/components/link-duplicate-symbols/lib-foo.wit
diff --git a/src/bin/wasm-tools/component.rs b/src/bin/wasm-tools/component.rs
index f680fb35..85c0edee 100644
--- a/src/bin/wasm-tools/component.rs
+++ b/src/bin/wasm-tools/component.rs
@@ -271,6 +271,9 @@ fn parse_library(s: &str) -> Result<(String, Vec<u8>)> {
 /// The resulting component's type will be the union of the types found in any `component-type*` custom sections in
 /// the input modules.
 ///
+/// Note that the order in which input libraries are listed determines priority in cases where more than one
+/// library exports the same symbol.
+///
 /// See
 /// https://github.com/WebAssembly/component-model/blob/main/design/mvp/examples/SharedEverythingDynamicLinking.md
 /// for further details.


Test patch:
diff --git a/crates/wit-component/tests/components.rs b/crates/wit-component/tests/components.rs
index 4bf1f7ad..84cdfa41 100644
--- a/crates/wit-component/tests/components.rs
+++ b/crates/wit-component/tests/components.rs
@@ -75,11 +75,15 @@ fn component_encoding_via_flags() -> Result<()> {
                 .encode()
         } else {
             let mut libs = glob::glob(path.join("lib-*.wat").to_str().unwrap())?
-                .map(|path| ("lib-", path, false))
+                .map(|path| Ok(("lib-", path?, false)))
                 .chain(
                     glob::glob(path.join("dlopen-lib-*.wat").to_str().unwrap())?
-                        .map(|path| ("dlopen-lib-", path, true)),
-                );
+                        .map(|path| Ok(("dlopen-lib-", path?, true))),
+                )
+                .collect::<Result<Vec<_>>>()?;
+
+            // Sort list to ensure deterministic order, which determines priority in cases of duplicate symbols:
+            libs.sort_by(|(_, a, _), (_, b, _)| a.cmp(b));
 
             let mut linker = Linker::default().validate(true);
 
@@ -87,10 +91,12 @@ fn component_encoding_via_flags() -> Result<()> {
                 linker = linker.stub_missing_functions(true);
             }
 
-            let linker = libs.try_fold(linker, |linker, (prefix, path, dl_openable)| {
-                let (name, wasm) = read_name_and_module(prefix, &path?, &resolve, pkg)?;
-                Ok::<_, Error>(linker.library(&name, &wasm, dl_openable)?)
-            })?;
+            let linker =
+                libs.into_iter()
+                    .try_fold(linker, |linker, (prefix, path, dl_openable)| {
+                        let (name, wasm) = read_name_and_module(prefix, &path, &resolve, pkg)?;
+                        Ok::<_, Error>(linker.library(&name, &wasm, dl_openable)?)
+                    })?;
 
             adapters
                 .try_fold(linker, |linker, path| {
diff --git a/crates/wit-component/tests/components/error-link-duplicate-symbols/error.txt b/crates/wit-component/tests/components/error-link-duplicate-symbols/error.txt
deleted file mode 100644
index 1765e8e4..00000000
--- a/crates/wit-component/tests/components/error-link-duplicate-symbols/error.txt
+++ /dev/null
@@ -1,3 +0,0 @@
-duplicate symbol(s):
-	bar needs foo (function [I32] -> [I32]), provided by bar, foo
-	foo needs foo (function [I32] -> [I32]), provided by bar, foo
\ No newline at end of file
diff --git a/crates/wit-component/tests/components/link-duplicate-symbols/component.wat b/crates/wit-component/tests/components/link-duplicate-symbols/component.wat
new file mode 100644
index 00000000..e182b2f6
--- /dev/null
+++ b/crates/wit-component/tests/components/link-duplicate-symbols/component.wat
@@ -0,0 +1,170 @@
+(component
+  (type (;0;)
+    (instance
+      (type (;0;) (func (param "v" s32) (result s32)))
+      (export (;0;) "foo" (func (type 0)))
+    )
+  )
+  (import (interface "test:test/test") (instance (;0;) (type 0)))
+  (core module (;0;)
+    (type (;0;) (func (param i32) (result i32)))
+    (func (;0;) (type 0) (param i32) (result i32)
+      local.get 0
+      i32.const 0
+      call_indirect (type 0)
+    )
+    (table (;0;) 1 funcref)
+    (memory (;0;) 17)
+    (global (;0;) (mut i32) i32.const 1048576)
+    (global (;1;) i32 i32.const 1048592)
+    (global (;2;) i32 i32.const 0)
+    (global (;3;) i32 i32.const 1048592)
+    (global (;4;) i32 i32.const 0)
+    (global (;5;) (mut i32) i32.const 1048592)
+    (global (;6;) (mut i32) i32.const 1114112)
+    (export "__stack_pointer" (global 0))
+    (export "bar:memory_base" (global 1))
+    (export "bar:table_base" (global 2))
+    (export "foo:memory_base" (global 3))
+    (export "foo:table_base" (global 4))
+    (export "__heap_base" (global 5))
+    (export "__heap_end" (global 6))
+    (export "foo" (func 0))
+    (export "__indirect_function_table" (table 0))
+    (export "memory" (memory 0))
+    (@producers
+      (processed-by "wit-component" "$CARGO_PKG_VERSION")
+    )
+  )
+  (core module (;1;)
+    (@dylink.0
+      (mem-info (memory 0 4))
+      (needed "foo")
+    )
+    (type (;0;) (func (param i32) (result i32)))
+    (import "env" "foo" (func $import_foo (;0;) (type 0)))
+    (func $bar (;1;) (type 0) (param i32) (result i32)
+      unreachable
+    )
+    (export "bar" (func $bar))
+    (export "foo" (func $bar))
+  )
+  (core module (;2;)
+    (@dylink.0
+      (mem-info (memory 0 4))
+      (needed "bar")
+    )
+    (type (;0;) (func (param i32) (result i32)))
+    (import "test:test/test" "foo" (func $import_foo (;0;) (type 0)))
+    (import "env" "foo" (func $import_foo2 (;1;) (type 0)))
+    (import "env" "bar" (func $import_bar (;2;) (type 0)))
+    (func $foo (;3;) (type 0) (param i32) (result i32)
+      unreachable
+    )
+    (export "test:test/test#foo" (func $foo))
+    (export "foo" (func $foo))
+  )
+  (core module (;3;)
+    (type (;0;) (func))
+    (type (;1;) (func (param i32)))
+    (type (;2;) (func (param i32) (result i32)))
+    (import "env" "memory" (memory (;0;) 0))
+    (import "env" "__indirect_function_table" (table (;0;) 0 funcref))
+    (import "bar" "foo" (func (;0;) (type 2)))
+    (func (;1;) (type 0))
+    (start 1)
+    (elem (;0;) (i32.const 0) func)
+    (elem (;1;) (i32.const 0) func 0)
+    (data (;0;) (i32.const 1048576) "\00\00\00\00\00\00\10\00")
+    (@producers
+      (processed-by "wit-component" "$CARGO_PKG_VERSION")
+    )
+  )
+  (core instance (;0;) (instantiate 0))
+  (alias core export 0 "memory" (core memory (;0;)))
+  (alias core export 0 "__heap_base" (core global (;0;)))
+  (alias core export 0 "__heap_end" (core global (;1;)))
+  (core instance (;1;)
+    (export "__heap_base" (global 0))
+    (export "__heap_end" (global 1))
+  )
+  (core instance (;2;))
+  (alias core export 0 "memory" (core memory (;1;)))
+  (alias core export 0 "__indirect_function_table" (core table (;0;)))
+  (alias core export 0 "__stack_pointer" (core global (;2;)))
+  (alias core export 0 "bar:memory_base" (core global (;3;)))
+  (alias core export 0 "bar:table_base" (core global (;4;)))
+  (alias core export 0 "foo" (core func (;0;)))
+  (core instance (;3;)
+    (export "memory" (memory 1))
+    (export "__indirect_function_table" (table 0))
+    (export "__stack_pointer" (global 2))
+    (export "__memory_base" (global 3))
+    (export "__table_base" (global 4))
+    (export "foo" (func 0))
+  )
+  (core instance (;4;) (instantiate 1
+      (with "GOT.mem" (instance 1))
+      (with "GOT.func" (instance 2))
+      (with "env" (instance 3))
+    )
+  )
+  (alias core export 0 "__heap_base" (core global (;5;)))
+  (alias core export 0 "__heap_end" (core global (;6;)))
+  (core instance (;5;)
+    (export "__heap_base" (global 5))
+    (export "__heap_end" (global 6))
+  )
+  (core instance (;6;))
+  (alias core export 0 "memory" (core memory (;2;)))
+  (alias core export 0 "__indirect_function_table" (core table (;1;)))
+  (alias core export 0 "__stack_pointer" (core global (;7;)))
+  (alias core export 0 "foo:memory_base" (core global (;8;)))
+  (alias core export 0 "foo:table_base" (core global (;9;)))
+  (alias core export 4 "bar" (core func (;1;)))
+  (alias core export 4 "foo" (core func (;2;)))
+  (core instance (;7;)
+    (export "memory" (memory 2))
+    (export "__indirect_function_table" (table 1))
+    (export "__stack_pointer" (global 7))
+    (export "__memory_base" (global 8))
+    (export "__table_base" (global 9))
+    (export "bar" (func 1))
+    (export "foo" (func 2))
+  )
+  (alias export 0 "foo" (func (;0;)))
+  (core func (;3;) (canon lower (func 0)))
+  (core instance (;8;)
+    (export "foo" (func 3))
+  )
+  (core instance (;9;) (instantiate 2
+      (with "GOT.mem" (instance 5))
+      (with "GOT.func" (instance 6))
+      (with "env" (instance 7))
+      (with "test:test/test" (instance 8))
+    )
+  )
+  (core instance (;10;) (instantiate 3
+      (with "env" (instance 0))
+      (with "bar" (instance 4))
+      (with "foo" (instance 9))
+    )
+  )
+  (type (;1;) (func (param "v" s32) (result s32)))
+  (alias core export 9 "test:test/test#foo" (core func (;4;)))
+  (func (;1;) (type 1) (canon lift (core func 4)))
+  (component (;0;)
+    (type (;0;) (func (param "v" s32) (result s32)))
+    (import "import-func-foo" (func (;0;) (type 0)))
+    (type (;1;) (func (param "v" s32) (result s32)))
+    (export (;1;) "foo" (func 0) (func (type 1)))
+  )
+  (instance (;1;) (instantiate 0
+      (with "import-func-foo" (func 1))
+    )
+  )
+  (export (;2;) (interface "test:test/test") (instance 1))
+  (@producers
+    (processed-by "wit-component" "$CARGO_PKG_VERSION")
+  )
+)
\ No newline at end of file
diff --git a/crates/wit-component/tests/components/link-duplicate-symbols/component.wit.print b/crates/wit-component/tests/components/link-duplicate-symbols/component.wit.print
new file mode 100644
index 00000000..d02a685b
--- /dev/null
+++ b/crates/wit-component/tests/components/link-duplicate-symbols/component.wit.print
@@ -0,0 +1,7 @@
+package root:component
+
+world root {
+  import test:test/test
+
+  export test:test/test
+}