threads: improve tests (#1719)

* threads: rename tests

This change simply renames the shared-everything-threads tests to use
more conventional names within the repository.

* threads: add more Binaryen tests

@tlively has been working on Binaryen support for
shared-everything-threads and has upstreamed several [tests] that are
useful here. This change incorporates some "bottom type" checks for
arrays and structs as a `shared-ref_eq.wast` test checking `ref.eq`
behavior for shared types (renamed to `ref-equivalence.wast` here).

To make these added tests pass, validation for `ref.eq` now allows it to
check both shared and ushared `eqref` types the the `ref.i31_shared`
returns the correct type.

[tests]: https://github.com/WebAssembly/binaryen/blob/main/test/spec
[shared-ref_eq.wast]: https://github.com/WebAssembly/binaryen/blob/main/test/spec/shared-ref_eq.wast

* threads: add polymorphism over `shared`

Certain instructions accept both shared and unshared references:
`ref.eq`, `i31.get_s`, `i31.get_u`, `array.len`, `any.convert_extern`,
and `extern.convert_any`. Validation needs to handle these special cases
to pass Binaryen's `polymorphism.wast` test. This change refactors the
`pop_operand` family of functions with several helpers to add
`pop_maybe_shared_ref`; `pop_maybe_shared_ref` lets the validator pass
an unshared `RefType` expectation but adapts it to being shared if that
is what is on the stack.

* Undo previous refactoring; reuse `OperatorValidatorTemp::pop_ref`

This change undoes previous work to rationalize the duplication in
`_pop_operand`, `pop_ref`, and now `pop_maybe_shared_ref`. As suggested
by @alexcrichton, this attempt keeps the status quo by reusing `pop_ref`
and doing a bit more work after the fact (re-checking the subtype
relationship).

* Handle `ref.eq` types from unreachable code

This validator's existing logic assumed that the bottom and heap bottom
types matched any reference type (see `_pop_operand`). This change
surfaces that logic to `visit_ref_eq` as well: if `pop_maybe_shared_ref`
returns `None` (a bottom or heap bottom) for either value to `ref.eq`,
we skip checking the shared-ness match, assuming like we do in
`_pop_operand` that a bottom or heap bottom matches any reference type
(we've already checked that the types are subtypes of `eqref`).

* Add TODO for unshared propagation in unreachable code

* Return shared `bool` from `pop_maybe_shared_ref`

* Rename `is_shared_ref_type` to `is_shared`

Author: abrown

crates/wasmparser/src/resources.rs

@@ -67,6 +67,13 @@ pub trait WasmModuleResources {
     /// Is `a` a subtype of `b`?
     fn is_subtype(&self, a: ValType, b: ValType) -> bool;
 
+    /// Is the given reference type `shared`?
+    ///
+    /// While abstract heap types do carry along a `shared` flag, concrete heap
+    /// types do not. This function resolves those concrete heap types to
+    /// determine `shared`-ness.
+    fn is_shared(&self, ty: RefType) -> bool;
+
     /// Check and canonicalize a value type.
     ///
     /// This will validate that `t` is valid under the `features` provided and
@@ -161,6 +168,9 @@ where
     fn is_subtype(&self, a: ValType, b: ValType) -> bool {
         T::is_subtype(self, a, b)
     }
+    fn is_shared(&self, ty: RefType) -> bool {
+        T::is_shared(self, ty)
+    }
     fn element_count(&self) -> u32 {
         T::element_count(self)
     }
@@ -220,6 +230,10 @@ where
         T::is_subtype(self, a, b)
     }
 
+    fn is_shared(&self, ty: RefType) -> bool {
+        T::is_shared(self, ty)
+    }
+
     fn element_count(&self) -> u32 {
         T::element_count(self)
     }

crates/wasmparser/src/validator/core.rs

@@ -1322,6 +1322,10 @@ impl WasmModuleResources for OperatorValidatorResources<'_> {
         self.types.valtype_is_subtype(a, b)
     }
 
+    fn is_shared(&self, ty: RefType) -> bool {
+        self.types.reftype_is_shared(ty)
+    }
+
     fn element_count(&self) -> u32 {
         self.module.element_types.len() as u32
     }
@@ -1394,6 +1398,10 @@ impl WasmModuleResources for ValidatorResources {
         self.0.snapshot.as_ref().unwrap().valtype_is_subtype(a, b)
     }
 
+    fn is_shared(&self, ty: RefType) -> bool {
+        self.0.snapshot.as_ref().unwrap().reftype_is_shared(ty)
+    }
+
     fn element_count(&self) -> u32 {
         self.0.element_types.len() as u32
     }

crates/wasmparser/src/validator/func.rs

@@ -237,7 +237,7 @@ impl<T: WasmModuleResources> FuncValidator<T> {
 mod tests {
     use super::*;
     use crate::types::CoreTypeId;
-    use crate::HeapType;
+    use crate::{HeapType, RefType};
 
     struct EmptyResources(crate::SubType);
 
@@ -288,6 +288,9 @@ mod tests {
         fn is_subtype(&self, _t1: ValType, _t2: ValType) -> bool {
             todo!()
         }
+        fn is_shared(&self, _ty: RefType) -> bool {
+            todo!()
+        }
         fn element_count(&self) -> u32 {
             todo!()
         }

crates/wasmparser/src/validator/operators.rs

@@ -199,15 +199,6 @@ impl From<RefType> for MaybeType {
     }
 }
 
-impl MaybeType {
-    fn as_type(&self) -> Option<ValType> {
-        match *self {
-            Self::Type(ty) => Some(ty),
-            Self::Bot | Self::HeapBot => None,
-        }
-    }
-}
-
 impl OperatorValidator {
     fn new(features: &WasmFeatures, allocs: OperatorValidatorAllocations) -> Self {
         let OperatorValidatorAllocations {
@@ -613,6 +604,7 @@ where
         Ok(actual)
     }
 
+    /// Pop a reference type from the operand stack.
     fn pop_ref(&mut self) -> Result<Option<RefType>> {
         match self.pop_operand(None)? {
             MaybeType::Bot | MaybeType::HeapBot => Ok(None),
@@ -625,6 +617,40 @@ where
         }
     }
 
+    /// Pop a reference type from the operand stack, checking if it is a subtype
+    /// of `expected` or the shared version of `expected`. This function returns
+    /// the popped reference type and its `shared`-ness, saving extra lookups
+    /// for concrete types. This function will panic if `expected` is shared or
+    /// a concrete type.
+    fn pop_maybe_shared_ref(&mut self, expected: RefType) -> Result<Option<(RefType, bool)>> {
+        debug_assert!(!expected.is_concrete_type_ref());
+        assert!(!self.resources.is_shared(expected));
+        let actual = match self.pop_ref()? {
+            Some(rt) => rt,
+            None => return Ok(None),
+        };
+        // Change our expectation based on whether we're dealing with an actual
+        // shared or unshared type.
+        let is_actual_shared = self.resources.is_shared(actual);
+        let expected = if is_actual_shared {
+            expected
+                .shared()
+                .expect("this only expects abstract heap types")
+        } else {
+            expected
+        };
+        // Check (again) that the actual type is a subtype of the expected type.
+        // Note that `_pop_operand` already does this kind of thing but we leave
+        // that for a future refactoring (TODO).
+        if !self.resources.is_subtype(actual.into(), expected.into()) {
+            bail!(
+                self.offset,
+                "type mismatch: expected subtype of {expected}, found {actual}",
+            )
+        }
+        Ok(Some((actual, is_actual_shared)))
+    }
+
     /// Fetches the type for the local at `idx`, returning an error if it's out
     /// of bounds.
     fn local(&self, idx: u32) -> Result<ValType> {
@@ -2723,8 +2749,22 @@ where
         Ok(())
     }
     fn visit_ref_eq(&mut self) -> Self::Output {
-        self.pop_operand(Some(RefType::EQ.nullable().into()))?;
-        self.pop_operand(Some(RefType::EQ.nullable().into()))?;
+        let a = self.pop_maybe_shared_ref(RefType::EQ.nullable())?;
+        let b = self.pop_maybe_shared_ref(RefType::EQ.nullable())?;
+        match (a, b) {
+            (Some((_, is_a_shared)), Some((_, is_b_shared))) => {
+                if is_a_shared != is_b_shared {
+                    bail!(
+                        self.offset,
+                        "type mismatch: expected `ref.eq` types to match `shared`-ness"
+                    );
+                }
+            }
+            _ => {
+                // One or both of the types are from unreachable code; assume
+                // the shared-ness matches.
+            }
+        }
         self.push_operand(ValType::I32)
     }
     fn visit_v128_load(&mut self, memarg: MemArg) -> Self::Output {
@@ -4224,7 +4264,7 @@ where
         Ok(())
     }
     fn visit_array_len(&mut self) -> Self::Output {
-        self.pop_operand(Some(RefType::ARRAY.nullable().into()))?;
+        self.pop_maybe_shared_ref(RefType::ARRAY.nullable())?;
         self.push_operand(ValType::I32)
     }
     fn visit_array_fill(&mut self, array_type_index: u32) -> Self::Output {
@@ -4406,24 +4446,32 @@ where
         Ok(())
     }
     fn visit_any_convert_extern(&mut self) -> Self::Output {
-        let extern_ref = self.pop_operand(Some(RefType::EXTERNREF.into()))?;
-        let is_nullable = extern_ref
-            .as_type()
-            .map_or(false, |ty| ty.as_reference_type().unwrap().is_nullable());
+        let extern_ref = self.pop_maybe_shared_ref(RefType::EXTERNREF)?;
+        let (is_nullable, shared) = if let Some((extern_ref, shared)) = extern_ref {
+            (extern_ref.is_nullable(), shared)
+        } else {
+            // TODO: propagating unshared may be incorrect here
+            // (https://github.com/WebAssembly/shared-everything-threads/issues/80)
+            (false, false)
+        };
         let heap_type = HeapType::Abstract {
-            shared: false, // TODO: handle shared--see https://github.com/WebAssembly/shared-everything-threads/issues/65.
+            shared,
             ty: AbstractHeapType::Any,
         };
         let any_ref = RefType::new(is_nullable, heap_type).unwrap();
         self.push_operand(any_ref)
     }
     fn visit_extern_convert_any(&mut self) -> Self::Output {
-        let any_ref = self.pop_operand(Some(RefType::ANY.nullable().into()))?;
-        let is_nullable = any_ref
-            .as_type()
-            .map_or(false, |ty| ty.as_reference_type().unwrap().is_nullable());
+        let any_ref = self.pop_maybe_shared_ref(RefType::ANY.nullable())?;
+        let (is_nullable, shared) = if let Some((any_ref, shared)) = any_ref {
+            (any_ref.is_nullable(), shared)
+        } else {
+            // TODO: propagating unshared may be incorrect here
+            // (https://github.com/WebAssembly/shared-everything-threads/issues/80)
+            (false, false)
+        };
         let heap_type = HeapType::Abstract {
-            shared: false, // TODO: handle shared--see https://github.com/WebAssembly/shared-everything-threads/issues/65.
+            shared,
             ty: AbstractHeapType::Extern,
         };
         let extern_ref = RefType::new(is_nullable, heap_type).unwrap();
@@ -4539,11 +4587,11 @@ where
         ))
     }
     fn visit_i31_get_s(&mut self) -> Self::Output {
-        self.pop_operand(Some(ValType::Ref(RefType::I31REF)))?;
+        self.pop_maybe_shared_ref(RefType::I31REF)?;
         self.push_operand(ValType::I32)
     }
     fn visit_i31_get_u(&mut self) -> Self::Output {
-        self.pop_operand(Some(ValType::Ref(RefType::I31REF)))?;
+        self.pop_maybe_shared_ref(RefType::I31REF)?;
         self.push_operand(ValType::I32)
     }
     fn visit_try(&mut self, mut ty: BlockType) -> Self::Output {

…cal/shared-everything-threads/array.wast …al/shared-everything-threads/arrays.wasttests/local/shared-everything-threads/array.wast renamed to tests/local/shared-everything-threads/arrays.wast tests/local/shared-everything-threads/array.wast renamed to tests/local/shared-everything-threads/arrays.wast

@@ -114,7 +114,31 @@
     (array.init_data $i8 0 (local.get 0) (i32.const 0) (i32.const 0) (i32.const 0)))
 )
 
-;; Check `array.atomic.rmw.*` instructions.
+;; Bottom types can be used as shared arrays.
+(module
+  (type $i8 (shared (array (mut i8))))
+  (type $i32 (shared (array (mut i32))))
+  (type $funcs (shared (array (mut (ref null (shared func))))))
+
+  (data)
+  ;; See https://github.com/bytecodealliance/wasm-tools/issues/1717.
+  (elem (ref null (shared func)))
+
+  (func (array.get_s $i8 (ref.null (shared none)) (i32.const 0)) (drop))
+  (func (array.get_u $i8 (ref.null (shared none)) (i32.const 0)) (drop))
+  (func (array.get $i32 (ref.null (shared none)) (i32.const 0)) (drop))
+  (func (array.set $i8 (ref.null (shared none)) (i32.const 0) (i32.const 0)))
+  (func (param (ref null $i8))
+    (array.copy $i8 $i8 (ref.null (shared none)) (i32.const 0) (local.get 0) (i32.const 0) (i32.const 0)))
+  (func (param (ref null $i8))
+    (array.copy $i8 $i8 (local.get 0) (i32.const 0) (ref.null (shared none)) (i32.const 0) (i32.const 0)))
+  (func (array.copy $i8 $i8 (ref.null (shared none)) (i32.const 0) (ref.null (shared none)) (i32.const 0) (i32.const 0)))
+  (func (array.fill $i8 (ref.null (shared none)) (i32.const 0) (i32.const 0) (i32.const 0)))
+  (func (array.init_data $i8 0 (ref.null (shared none)) (i32.const 0) (i32.const 0) (i32.const 0)))
+  (func (array.init_elem $funcs 0 (ref.null (shared none)) (i32.const 0) (i32.const 0) (i32.const 0)))
+)
+
+;; Exhaustively check `array.atomic.rmw.*` instructions.
 (module (; get, i32, seq_cst ;)
   (type $a (shared (array (mut i32))))
   (func (export "array-atomic-get-i32-seq_cst") (param $x (ref null $a)) (param $y i32) (result i32)

…al/shared-everything-threads/global.wast …l/shared-everything-threads/globals.wasttests/local/shared-everything-threads/global.wast renamed to tests/local/shared-everything-threads/globals.wast tests/local/shared-everything-threads/global.wast renamed to tests/local/shared-everything-threads/globals.wast

@@ -0,0 +1,23 @@
+;; Some instructions are shared-polymorphic and work with shared or unshared
+;; references.
+(module
+  (func (drop (ref.eq (ref.null (shared none)) (ref.null (shared none)))))
+
+  (func (param (ref null (shared i31))) (drop (i31.get_s (local.get 0))))
+  (func (param (ref null (shared i31))) (drop (i31.get_u (local.get 0))))
+
+  (func (param (ref null (shared array))) (drop (array.len (local.get 0))))
+
+  (func (param (ref null (shared extern))) (result (ref null (shared any)))
+    (any.convert_extern (local.get 0))
+  )
+  (func (param (ref (shared extern))) (result (ref (shared any)))
+    (any.convert_extern (local.get 0))
+  )
+  (func (param (ref null (shared any))) (result (ref null (shared extern)))
+    (extern.convert_any (local.get 0))
+  )
+  (func (param (ref (shared any))) (result (ref (shared extern)))
+    (extern.convert_any (local.get 0))
+  )
+)