Update handling and parsing of element segments (#1126)

* Update handling and parsing of element segments

This commit started off by updating the spec testsuite submodule to the
latest version and I ended up fixing various bits of parsing element
segments. The changes here are a little subtle, but they are:

* The text format for element segments was refactored and audited to
  attempt to match what's currently written in the spec. The spec
  testsuite doesn't look like it follows the spec perfectly so a few
  adjustments are made. The main change here functionally is that inline
  element segments on `table` definitions might now automatically switch
  a list-of-index text format into a list-of-expression binary format.
  This is what #952 and its discussion was about. Other than this though
  some parsing has been tightened up as previously some invalid element
  segments were allowed and now require some more specifiers. For
  example `(elem)` was previously considered valid but that is no longer
  valid, instead it must be `(elem func)`.

* The `wasmparser` and `wasm-encoder` crates have both removed the type
  field from their element segment representations, instead only having
  this type information on the list-of-expressions payload. This
  reflects how in the binary format the list-of-indexes form of element
  segments does not have type information. This was primarily done to
  adjust validation to pass a new test where a list-of-indexes element
  segment seems to be expected to fit in a table of `(ref func)`. This
  means that the type of the element segment previously inferred as
  `funcref` was no longer valid and instead it should be `(ref func)`.
  To better represent this these pieces were refactored internally.

* Validation of the `table_init` instruction was updated to perform a
  subtyping check between the element segment and the destination table
  to allow initializing a nullable table with a non-nullable element
  segment, which is a valid operation where the types are not exactly
  equal.

* One extra error message had to be allowed for some new spec tests
  about invalid modules.

Overall this isn't expected to be a major update of any form. Just
another in the long line of updates and adjustments to how element
segments work and validate.

* Fix benchmark

crates/wasm-encoder/src/core/elements.rs

@@ -22,11 +22,10 @@ use crate::{encode_section, ConstExpr, Encode, RefType, Section, SectionId};
 /// let mut elements = ElementSection::new();
 /// let table_index = 0;
 /// let offset = ConstExpr::i32_const(42);
-/// let element_type = RefType::FUNCREF;
 /// let functions = Elements::Functions(&[
 ///     // Function indices...
 /// ]);
-/// elements.active(Some(table_index), &offset, element_type, functions);
+/// elements.active(Some(table_index), &offset, functions);
 ///
 /// let mut module = Module::new();
 /// module
@@ -47,7 +46,7 @@ pub enum Elements<'a> {
     /// A sequences of references to functions by their indices.
     Functions(&'a [u32]),
     /// A sequence of reference expressions.
-    Expressions(&'a [ConstExpr]),
+    Expressions(RefType, &'a [ConstExpr]),
 }
 
 /// An element segment's mode.
@@ -79,8 +78,6 @@ pub enum ElementMode<'a> {
 pub struct ElementSegment<'a> {
     /// The element segment's mode.
     pub mode: ElementMode<'a>,
-    /// The element segment's type.
-    pub element_type: RefType,
     /// This segment's elements.
     pub elements: Elements<'a>,
 }
@@ -104,50 +101,53 @@ impl ElementSection {
     /// Define an element segment.
     pub fn segment<'a>(&mut self, segment: ElementSegment<'a>) -> &mut Self {
         let expr_bit = match segment.elements {
-            Elements::Expressions(_) => 0b100u32,
+            Elements::Expressions(..) => 0b100u32,
             Elements::Functions(_) => 0b000u32,
         };
+        let mut encode_type = false;
         match &segment.mode {
-            ElementMode::Active {
-                table: None,
-                offset,
-            } if segment.element_type == RefType::FUNCREF => {
-                (/* 0x00 | */expr_bit).encode(&mut self.bytes);
-                offset.encode(&mut self.bytes);
-            }
             ElementMode::Passive => {
                 (0x01 | expr_bit).encode(&mut self.bytes);
-                if expr_bit == 0 {
-                    self.bytes.push(0x00); // elemkind == funcref
-                } else {
-                    segment.element_type.encode(&mut self.bytes);
-                }
+                encode_type = true;
             }
             ElementMode::Active { table, offset } => {
-                (0x02 | expr_bit).encode(&mut self.bytes);
-                table.unwrap_or(0).encode(&mut self.bytes);
-                offset.encode(&mut self.bytes);
-                if expr_bit == 0 {
-                    self.bytes.push(0x00); // elemkind == funcref
-                } else {
-                    segment.element_type.encode(&mut self.bytes);
+                match (table, &segment.elements) {
+                    // If the `table` is not specified then the 0x00 encoding
+                    // can be used with either function indices or expressions
+                    // that have a `funcref` type.
+                    (None, Elements::Functions(_) | Elements::Expressions(RefType::FUNCREF, _)) => {
+                        (/* 0x00 | */expr_bit).encode(&mut self.bytes);
+                    }
+
+                    // ... otherwise fall through for all other expressions here
+                    // with table 0 or an explicitly specified table to the 0x02
+                    // encoding.
+                    (None, Elements::Expressions(..)) | (Some(_), _) => {
+                        (0x02 | expr_bit).encode(&mut self.bytes);
+                        table.unwrap_or(0).encode(&mut self.bytes);
+                        encode_type = true;
+                    }
                 }
+                offset.encode(&mut self.bytes);
             }
             ElementMode::Declared => {
                 (0x03 | expr_bit).encode(&mut self.bytes);
-                if expr_bit == 0 {
-                    self.bytes.push(0x00); // elemkind == funcref
-                } else {
-                    segment.element_type.encode(&mut self.bytes);
-                }
+                encode_type = true;
             }
         }
 
         match segment.elements {
             Elements::Functions(fs) => {
+                if encode_type {
+                    // elemkind == funcref
+                    self.bytes.push(0x00);
+                }
                 fs.encode(&mut self.bytes);
             }
-            Elements::Expressions(e) => {
+            Elements::Expressions(ty, e) => {
+                if encode_type {
+                    ty.encode(&mut self.bytes);
+                }
                 e.len().encode(&mut self.bytes);
                 for expr in e {
                     expr.encode(&mut self.bytes);
@@ -168,37 +168,33 @@ impl ElementSection {
         &mut self,
         table_index: Option<u32>,
         offset: &ConstExpr,
-        element_type: RefType,
         elements: Elements<'_>,
     ) -> &mut Self {
         self.segment(ElementSegment {
             mode: ElementMode::Active {
                 table: table_index,
                 offset,
             },
-            element_type,
             elements,
         })
     }
 
     /// Encode a passive element segment.
     ///
     /// Passive segments are part of the bulk memory proposal.
-    pub fn passive<'a>(&mut self, element_type: RefType, elements: Elements<'a>) -> &mut Self {
+    pub fn passive<'a>(&mut self, elements: Elements<'a>) -> &mut Self {
         self.segment(ElementSegment {
             mode: ElementMode::Passive,
-            element_type,
             elements,
         })
     }
 
     /// Encode a declared element segment.
     ///
     /// Declared segments are part of the bulk memory proposal.
-    pub fn declared<'a>(&mut self, element_type: RefType, elements: Elements<'a>) -> &mut Self {
+    pub fn declared<'a>(&mut self, elements: Elements<'a>) -> &mut Self {
         self.segment(ElementSegment {
             mode: ElementMode::Declared,
-            element_type,
             elements,
         })
     }

crates/wasm-mutate/src/mutators/modify_const_exprs.rs

@@ -195,7 +195,7 @@ impl Mutator for ConstExpressionMutator {
                             // to skip a specific number of activations of the Translator methods.
                             let item_count = match &element.items {
                                 wasmparser::ElementItems::Functions(r) => r.count(),
-                                wasmparser::ElementItems::Expressions(r) => r.count(),
+                                wasmparser::ElementItems::Expressions(_, r) => r.count(),
                             };
                             if item_count > 0 {
                                 let skip = translator.config.rng().gen_range(0..item_count);
@@ -287,9 +287,9 @@ mod tests {
     #[test]
     fn reduce_elem_funcref() {
         match_reduction(
-            r#"(module (table 0 funcref) (elem $a $b) (func $a) (func $b))"#,
+            r#"(module (table 0 funcref) (elem func $a $b) (func $a) (func $b))"#,
             super::ConstExpressionMutator::ElementFunc,
-            r#"(module (table 0 funcref) (elem $a $a) (func $a) (func $b))"#,
+            r#"(module (table 0 funcref) (elem func $a $a) (func $a) (func $b))"#,
         );
     }
 

crates/wasm-mutate/src/mutators/remove_item.rs

@@ -546,7 +546,7 @@ mod tests {
     #[test]
     fn remove_elem() {
         crate::mutators::match_mutation(
-            r#"(module (elem))"#,
+            r#"(module (elem funcref))"#,
             RemoveItemMutator(Item::Element),
             r#"(module)"#,
         );

crates/wasm-mutate/src/mutators/translate.rs

@@ -282,7 +282,6 @@ pub fn element(
         ElementKind::Passive => ElementMode::Passive,
         ElementKind::Declared => ElementMode::Declared,
     };
-    let element_type = t.translate_refty(&element.ty)?;
     let functions;
     let exprs;
     let elements = match element.items {
@@ -293,25 +292,21 @@ pub fn element(
                 .collect::<Result<Vec<_>, _>>()?;
             Elements::Functions(&functions)
         }
-        wasmparser::ElementItems::Expressions(reader) => {
+        wasmparser::ElementItems::Expressions(ty, reader) => {
             exprs = reader
                 .into_iter()
                 .map(|f| {
                     t.translate_const_expr(
                         &f?,
-                        &wasmparser::ValType::Ref(element.ty),
+                        &wasmparser::ValType::Ref(ty),
                         ConstExprKind::ElementFunction,
                     )
                 })
                 .collect::<Result<Vec<_>, _>>()?;
-            Elements::Expressions(&exprs)
+            Elements::Expressions(t.translate_refty(&ty)?, &exprs)
         }
     };
-    s.segment(ElementSegment {
-        mode,
-        element_type,
-        elements,
-    });
+    s.segment(ElementSegment { mode, elements });
     Ok(())
 }
 

crates/wasm-smith/src/core/encode.rs

@@ -158,7 +158,7 @@ impl Module {
                             None => wasm_encoder::ConstExpr::ref_null(el.ty.heap_type),
                         }
                     }));
-                    wasm_encoder::Elements::Expressions(&exps)
+                    wasm_encoder::Elements::Expressions(el.ty, &exps)
                 }
                 Elements::Functions(fs) => wasm_encoder::Elements::Functions(fs),
             };
@@ -169,13 +169,13 @@ impl Module {
                         Offset::Const64(n) => ConstExpr::i64_const(n),
                         Offset::Global(g) => ConstExpr::global_get(g),
                     };
-                    elems.active(*table, &offset, el.ty, elements);
+                    elems.active(*table, &offset, elements);
                 }
                 ElementKind::Passive => {
-                    elems.passive(el.ty, elements);
+                    elems.passive(elements);
                 }
                 ElementKind::Declared => {
-                    elems.declared(el.ty, elements);
+                    elems.declared(elements);
                 }
             }
         }

crates/wasmparser/benches/benchmark.rs

@@ -136,7 +136,7 @@ fn read_all_wasm(wasm: &[u8]) -> Result<()> {
                                 op?;
                             }
                         }
-                        wasmparser::ElementItems::Expressions(r) => {
+                        wasmparser::ElementItems::Expressions(_, r) => {
                             for op in r {
                                 op?;
                             }

crates/wasmparser/src/readers/core/elements.rs

@@ -26,8 +26,6 @@ pub struct Element<'a> {
     pub kind: ElementKind<'a>,
     /// The initial elements of the element segment.
     pub items: ElementItems<'a>,
-    /// The type of the elements.
-    pub ty: RefType,
     /// The range of the the element segment.
     pub range: Range<usize>,
 }
@@ -54,7 +52,7 @@ pub enum ElementItems<'a> {
     /// This element contains function indices.
     Functions(SectionLimited<'a, u32>),
     /// This element contains constant expressions used to initialize the table.
-    Expressions(SectionLimited<'a, ConstExpr<'a>>),
+    Expressions(RefType, SectionLimited<'a, ConstExpr<'a>>),
 }
 
 /// A reader for the element section of a WebAssembly module.
@@ -104,10 +102,10 @@ impl<'a> FromReader<'a> for Element<'a> {
         let exprs = flags & 0b100 != 0;
         let ty = if flags & 0b011 != 0 {
             if exprs {
-                reader.read()?
+                Some(reader.read()?)
             } else {
                 match reader.read()? {
-                    ExternalKind::Func => RefType::FUNCREF,
+                    ExternalKind::Func => None,
                     _ => {
                         return Err(BinaryReaderError::new(
                             "only the function external type is supported in elem segment",
@@ -117,7 +115,7 @@ impl<'a> FromReader<'a> for Element<'a> {
                 }
             }
         } else {
-            RefType::FUNCREF
+            None
         };
         // FIXME(#188) ideally wouldn't have to do skips here
         let data = reader.skip(|reader| {
@@ -134,11 +132,12 @@ impl<'a> FromReader<'a> for Element<'a> {
             Ok(())
         })?;
         let items = if exprs {
-            ElementItems::Expressions(SectionLimited::new(
-                data.remaining_buffer(),
-                data.original_position(),
-            )?)
+            ElementItems::Expressions(
+                ty.unwrap_or(RefType::FUNCREF),
+                SectionLimited::new(data.remaining_buffer(), data.original_position())?,
+            )
         } else {
+            assert!(ty.is_none());
             ElementItems::Functions(SectionLimited::new(
                 data.remaining_buffer(),
                 data.original_position(),
@@ -148,11 +147,6 @@ impl<'a> FromReader<'a> for Element<'a> {
         let elem_end = reader.original_position();
         let range = elem_start..elem_end;
 
-        Ok(Element {
-            kind,
-            items,
-            ty,
-            range,
-        })
+        Ok(Element { kind, items, range })
     }
 }

crates/wasmparser/src/validator/core.rs

@@ -199,24 +199,30 @@ impl ModuleState {
     ) -> Result<()> {
         // the `funcref` value type is allowed all the way back to the MVP, so
         // don't check it here
-        if e.ty != RefType::FUNCREF {
-            self.module
-                .check_value_type(ValType::Ref(e.ty), features, offset)?;
-        }
+        let element_ty = match &e.items {
+            crate::ElementItems::Functions(_) => RefType::FUNC,
+            crate::ElementItems::Expressions(ty, _) => {
+                self.module
+                    .check_value_type(ValType::Ref(*ty), features, offset)?;
+                *ty
+            }
+        };
+
         match e.kind {
             ElementKind::Active {
                 table_index,
                 offset_expr,
             } => {
                 let table = self.module.table_at(table_index.unwrap_or(0), offset)?;
-                if !self
-                    .module
-                    .matches(ValType::Ref(e.ty), ValType::Ref(table.element_type), types)
-                {
+                if !self.module.matches(
+                    ValType::Ref(element_ty),
+                    ValType::Ref(table.element_type),
+                    types,
+                ) {
                     return Err(BinaryReaderError::new(
                         format!(
                             "type mismatch: invalid element type `{}` for table type `{}`",
-                            ty_to_str(e.ty.into()),
+                            ty_to_str(element_ty.into()),
                             ty_to_str(table.element_type.into()),
                         ),
                         offset,
@@ -248,27 +254,21 @@ impl ModuleState {
         match e.items {
             crate::ElementItems::Functions(reader) => {
                 let count = reader.count();
-                if !e.ty.is_nullable() && count == 0 {
-                    return Err(BinaryReaderError::new(
-                        "a non-nullable element must come with an initialization expression",
-                        offset,
-                    ));
-                }
                 validate_count(count)?;
                 for f in reader.into_iter_with_offsets() {
                     let (offset, f) = f?;
                     self.module.get_func_type(f, types, offset)?;
                     self.module.assert_mut().function_references.insert(f);
                 }
             }
-            crate::ElementItems::Expressions(reader) => {
+            crate::ElementItems::Expressions(ty, reader) => {
                 validate_count(reader.count())?;
                 for expr in reader {
-                    self.check_const_expr(&expr?, ValType::Ref(e.ty), features, types)?;
+                    self.check_const_expr(&expr?, ValType::Ref(ty), features, types)?;
                 }
             }
         }
-        self.module.assert_mut().element_types.push(e.ty);
+        self.module.assert_mut().element_types.push(element_ty);
         Ok(())
     }