relax intra-component stream/future read/write rules (#12181)

Per WebAssembly/component-model#580, we now allow
intra-component reads and writes for all numeric payloads.

While I was implementing this, Alex and I spotted a couple of bugs, which I've
fixed here:

- We can't treat `bool` and `char` payloads as "flat" (i.e. trivially copy-able) since not all bit patterns are valid for those types.
- We weren't enforcing the rules (old or new) for streams with non-"flat" payloads (i.e. the ones that most needed them enforced 🤦)

Also, now that WebAssembly/component-model#578 has been
merged, I've updated the `tests/component-model` submodule and reduced the list
of "expect fail" tests for that directory.

Finally, note that the `p3_http_middleware_host_to_host` test is still expected
to trap, but now for a different reason.  Previously, it trapped because it
tried to do an intra-component read/write on a `stream<u8>`.  Now that that's
allowed, it instead traps when doing an intra-component read/write on a
`future<result<option<trailers>, error-code>>`, which is still prohibited.

Signed-off-by: Joel Dice <[email protected]>

Author: dicej

crates/cranelift/src/compiler/component.rs

@@ -856,17 +856,19 @@ impl<'a> TrampolineCompiler<'a> {
     /// Determine whether the specified type can be optimized as a stream
     /// payload by lifting and lowering with a simple `memcpy`.
     ///
-    /// Any type containing only "flat", primitive data (i.e. no pointers or
-    /// handles) should qualify for this optimization, but it's also okay to
-    /// conservatively return `None` here; the fallback slow path will always
-    /// work -- it just won't be as efficient.
+    /// Any type containing only "flat", primitive data for which all bit
+    /// patterns are valid (i.e. no pointers, handles, bools, or chars) should
+    /// qualify for this optimization, but it's also okay to conservatively
+    /// return `None` here; the fallback slow path will always work -- it just
+    /// won't be as efficient.
     fn flat_stream_element_info(&self, ty: TypeStreamTableIndex) -> Option<&CanonicalAbiInfo> {
         let payload = self.types[self.types[ty].ty].payload;
         match payload {
             None => Some(&CanonicalAbiInfo::ZERO),
             Some(
-                payload @ (InterfaceType::Bool
-                | InterfaceType::S8
+                // Note that we exclude `Bool` and `Char` from this list because
+                // not all bit patterns are valid for those types.
+                payload @ (InterfaceType::S8
                 | InterfaceType::U8
                 | InterfaceType::S16
                 | InterfaceType::U16
@@ -875,8 +877,7 @@ impl<'a> TrampolineCompiler<'a> {
                 | InterfaceType::S64
                 | InterfaceType::U64
                 | InterfaceType::Float32
-                | InterfaceType::Float64
-                | InterfaceType::Char),
+                | InterfaceType::Float64),
             ) => Some(self.types.canonical_abi(&payload)),
             // TODO: Recursively check for other "flat" types (i.e. those without pointers or handles),
             // e.g. `record`s, `variant`s, etc. which contain only flat types.

crates/test-util/src/wast.rs

@@ -186,7 +186,9 @@ fn component_test_config(test: &Path) -> TestConfig {
     if let Some(parent) = test.parent() {
         if parent.ends_with("async") {
             ret.component_model_async = Some(true);
+            ret.component_model_async_stackful = Some(true);
             ret.component_model_async_builtins = Some(true);
+            ret.component_model_threading = Some(true);
         }
         if parent.ends_with("wasm-tools") {
             ret.memory64 = Some(true);
@@ -699,26 +701,12 @@ impl WastTest {
             "component-model/test/values/trap-in-post-return.wast",
             "component-model/test/wasmtime/resources.wast",
             "component-model/test/wasm-tools/naming.wast",
-            // TODO: remove these once
-            // https://github.com/WebAssembly/component-model/pull/578 has been
-            // merged:
-            "component-model/test/async/async-calls-sync.wast",
-            "component-model/test/async/backpressure-deadlock.wast",
-            "component-model/test/async/cancel-stream.wast",
-            "component-model/test/async/cancel-subtask.wast",
-            "component-model/test/async/deadlock.wast",
-            "component-model/test/async/drop-subtask.wast",
-            "component-model/test/async/drop-waitable-set.wast",
-            "component-model/test/async/empty-wait.wast",
-            "component-model/test/async/fused.wast",
-            "component-model/test/async/future-read.wast",
-            "component-model/test/async/partial-stream-copies.wast",
-            "component-model/test/async/passing-resources.wast",
-            "component-model/test/async/stackful.wast",
+            // FIXME(#12129)
             "component-model/test/async/trap-if-block-and-sync.wast",
-            "component-model/test/async/trap-if-done.wast",
-            "component-model/test/async/wait-during-callback.wast",
-            "component-model/test/async/zero-length.wast",
+            // TODO: Remove this once
+            // https://github.com/bytecodealliance/wasm-tools/pull/2406 is
+            // merged and released, and Wasmtime has been updated to use it:
+            "component-model/test/async/same-component-stream-future.wast",
         ];
         if failing_component_model_tests
             .iter()

crates/wasi-http/tests/all/p3/mod.rs

@@ -333,7 +333,7 @@ async fn p3_http_middleware() -> Result<()> {
 async fn p3_http_middleware_host_to_host() {
     let error = format!("{:?}", test_http_middleware(true).await.unwrap_err());
 
-    let expected = "cannot read from and write to intra-component stream with non-unit payload";
+    let expected = "cannot read from and write to intra-component future with non-numeric payload";
 
     assert!(
         error.contains(expected),

crates/wasmtime/src/runtime/component/concurrent/futures_and_streams.rs

@@ -3042,9 +3042,9 @@ impl Instance {
 
                 let payload = types[types[write_ty].ty].payload;
 
-                if write_caller == read_caller && payload.is_some() {
+                if write_caller == read_caller && !allow_intra_component_read_write(payload) {
                     bail!(
-                        "cannot read from and write to intra-component future with non-unit payload"
+                        "cannot read from and write to intra-component future with non-numeric payload"
                     )
                 }
 
@@ -3084,13 +3084,15 @@ impl Instance {
                 }
             }
             (TransmitIndex::Stream(write_ty), TransmitIndex::Stream(read_ty)) => {
-                if let Some(flat_abi) = flat_abi {
-                    if write_caller == read_caller && types[types[write_ty].ty].payload.is_some() {
-                        bail!(
-                            "cannot read from and write to intra-component stream with non-unit payload"
-                        )
-                    }
+                if write_caller == read_caller
+                    && !allow_intra_component_read_write(types[types[write_ty].ty].payload)
+                {
+                    bail!(
+                        "cannot read from and write to intra-component stream with non-numeric payload"
+                    )
+                }
 
+                if let Some(flat_abi) = flat_abi {
                     // Fast path memcpy for "flat" (i.e. no pointers or handles) payloads:
                     let length_in_bytes = usize::try_from(flat_abi.size).unwrap() * count;
                     if length_in_bytes > 0 {
@@ -3122,7 +3124,20 @@ impl Instance {
                                 .as_mut_ptr();
                             // SAFETY: Both `src` and `dst` have been validated
                             // above.
-                            unsafe { src.copy_to(dst, length_in_bytes) };
+                            unsafe {
+                                if write_caller == read_caller {
+                                    // If the same instance owns both ends of
+                                    // the stream, the source and destination
+                                    // buffers might overlap.
+                                    src.copy_to(dst, length_in_bytes)
+                                } else {
+                                    // Since the read and write ends of the
+                                    // stream are owned by distinct instances,
+                                    // the buffers cannot possibly belong to the
+                                    // same memory and thus cannot overlap.
+                                    src.copy_to_nonoverlapping(dst, length_in_bytes)
+                                }
+                            }
                         }
                     }
                 } else {
@@ -4533,6 +4548,27 @@ impl Waitable {
     }
 }
 
+/// Determine whether an intra-component read/write is allowed for the specified
+/// `stream` or `future` payload type according to the component model
+/// specification.
+fn allow_intra_component_read_write(ty: Option<InterfaceType>) -> bool {
+    matches!(
+        ty,
+        None | Some(
+            InterfaceType::S8
+                | InterfaceType::U8
+                | InterfaceType::S16
+                | InterfaceType::U16
+                | InterfaceType::S32
+                | InterfaceType::U32
+                | InterfaceType::S64
+                | InterfaceType::U64
+                | InterfaceType::Float32
+                | InterfaceType::Float64
+        )
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

tests/component-model

@@ -2,15 +2,21 @@
 ;;! component_model_async_builtins = true
 
 (component
-  (core module $libc (memory (export "m") 1))
+  (core module $libc
+     (func (export "realloc") (param i32 i32 i32 i32) (result i32)
+        unreachable
+     )
+     (memory (export "m") 1)
+  )
   (core instance $libc (instantiate $libc))
 
-  (type $s (future u32))
+  (type $s (future string))
   (core func $future.new (canon future.new $s))
-  (core func $future.read (canon future.read $s async (memory $libc "m")))
+  (core func $future.read (canon future.read $s async (memory $libc "m") (realloc (func $libc "realloc"))))
   (core func $future.write (canon future.write $s async (memory $libc "m")))
 
   (core module $m
+    (import "" "m" (memory 1))
     (import "" "future.new" (func $future.new (result i64)))
     (import "" "future.read" (func $future.read (param i32 i32) (result i32)))
     (import "" "future.write" (func $future.write (param i32 i32) (result i32)))
@@ -36,6 +42,7 @@
 
   (core instance $i (instantiate $m
     (with "" (instance
+      (export "m" (memory $libc "m"))
       (export "future.new" (func $future.new))
       (export "future.read" (func $future.read))
       (export "future.write" (func $future.write))
@@ -45,4 +52,4 @@
   (func (export "run") (canon lift (core func $i "run")))
 )
 
-(assert_trap (invoke "run") "cannot read from and write to intra-component future with non-unit payload")
+(assert_trap (invoke "run") "cannot read from and write to intra-component future with non-numeric payload")

tests/misc_testsuite/component-model/async/intra-futures.wast

@@ -2,15 +2,21 @@
 ;;! component_model_async_builtins = true
 
 (component
-  (core module $libc (memory (export "m") 1))
+  (core module $libc
+     (func (export "realloc") (param i32 i32 i32 i32) (result i32)
+       (i32.const 8)
+     )
+     (memory (export "m") 1)
+  )
   (core instance $libc (instantiate $libc))
 
-  (type $s (stream u32))
+  (type $s (stream string))
   (core func $stream.new (canon stream.new $s))
-  (core func $stream.read (canon stream.read $s async (memory $libc "m")))
+  (core func $stream.read (canon stream.read $s async (memory $libc "m") (realloc (func $libc "realloc"))))
   (core func $stream.write (canon stream.write $s async (memory $libc "m")))
 
   (core module $m
+    (import "" "m" (memory 1))
     (import "" "stream.new" (func $stream.new (result i64)))
     (import "" "stream.read" (func $stream.read (param i32 i32 i32) (result i32)))
     (import "" "stream.write" (func $stream.write (param i32 i32 i32) (result i32)))
@@ -24,18 +30,19 @@
       (local.set $r (i32.wrap_i64 (local.get $tmp)))
       (local.set $w (i32.wrap_i64 (i64.shr_u (local.get $tmp) (i64.const 32))))
 
-      (call $stream.read (local.get $r) (i32.const 0) (i32.const 4))
+      (call $stream.read (local.get $r) (i32.const 0) (i32.const 1))
       i32.const -1 ;; BLOCKED
       i32.ne
       if unreachable end
 
-      (call $stream.write (local.get $w) (i32.const 0) (i32.const 4))
+      (call $stream.write (local.get $w) (i32.const 0) (i32.const 1))
       drop
     )
   )
 
   (core instance $i (instantiate $m
     (with "" (instance
+      (export "m" (memory $libc "m"))
       (export "stream.new" (func $stream.new))
       (export "stream.read" (func $stream.read))
       (export "stream.write" (func $stream.write))
@@ -45,4 +52,4 @@
   (func (export "run") (canon lift (core func $i "run")))
 )
 
-(assert_trap (invoke "run") "cannot read from and write to intra-component stream with non-unit payload")
+(assert_trap (invoke "run") "cannot read from and write to intra-component stream with non-numeric payload")