Remove mem_byte/table_byte instruction payloads (#1584)

alexcrichton · web-flow · commit 024fb6776845 · 2024-05-29T19:00:22.000Z
This commit removes a workaround in the `wasmparser` opcode representation. The `mem_byte` field of `MemoryGrow` and `MemorySize` and the `table_byte` field of `CallIndirect` are no longer necessary after #1548 because features can be consulted directly when parsing binaries.
diff --git a/crates/wasm-mutate/src/mutators/peephole/dfg.rs b/crates/wasm-mutate/src/mutators/peephole/dfg.rs
@@ -640,11 +640,11 @@ impl<'a> DFGBuilder {
                         idx,
                     );
                 }
-                Operator::MemoryGrow { mem, mem_byte: _ } => {
+                Operator::MemoryGrow { mem } => {
                     let arg = self.pop_operand(idx, false);
                     self.push_node(Lang::MemoryGrow(*mem, Id::from(arg)), idx);
                 }
-                Operator::MemorySize { mem, mem_byte: _ } => {
+                Operator::MemorySize { mem } => {
                     self.push_node(Lang::MemorySize(*mem), idx);
                 }
                 Operator::TableGrow { table } => {
diff --git a/crates/wasm-mutate/src/mutators/translate.rs b/crates/wasm-mutate/src/mutators/translate.rs
@@ -370,8 +370,6 @@ pub fn op(t: &mut dyn Translator, op: &Operator<'_>) -> Result<Instruction<'stat
                 .into(),
             $arg.default(),
         ));
-        (map $arg:ident table_byte) => (());
-        (map $arg:ident mem_byte) => (());
         (map $arg:ident flags) => (());
         (map $arg:ident ty) => (t.translate_ty($arg)?);
         (map $arg:ident hty) => (t.translate_heapty($arg)?);
@@ -402,22 +400,14 @@ pub fn op(t: &mut dyn Translator, op: &Operator<'_>) -> Result<Instruction<'stat
         (build V128Const $arg:ident) => (I::V128Const($arg.i128()));
         (build TryTable $table:ident) => (unimplemented_try_table());
         (build $op:ident $arg:ident) => (I::$op($arg));
-        (build CallIndirect $ty:ident $table:ident $_:ident) => (I::CallIndirect {
+        (build CallIndirect $ty:ident $table:ident) => (I::CallIndirect {
             ty: $ty,
             table: $table,
         });
         (build ReturnCallIndirect $ty:ident $table:ident) => (I::ReturnCallIndirect {
             ty: $ty,
             table: $table,
         });
-        (build MemoryGrow $mem:ident $_:ident) => (I::MemoryGrow($mem));
-        (build MemorySize $mem:ident $_:ident) => (I::MemorySize($mem));
-        (build StructNew $type_index:ident) => (I::StructNew($type_index));
-        (build ArrayGet $type_index:ident) => (I::ArrayGet($type_index));
-        (build ArrayGetS $type_index:ident) => (I::ArrayGetS($type_index));
-        (build ArrayGetU $type_index:ident) => (I::ArrayGetU($type_index));
-        (build ArraySet $type_index:ident) => (I::ArraySet($type_index));
-        (build ArrayFill $type_index:ident) => (I::ArrayFill($type_index));
         (build $op:ident $($arg:ident)*) => (I::$op { $($arg),* });
     }
 
diff --git a/crates/wasmparser/src/binary_reader.rs b/crates/wasmparser/src/binary_reader.rs
@@ -281,12 +281,6 @@ impl<'a> BinaryReader<'a> {
         })
     }
 
-    fn read_first_byte_and_var_u32(&mut self) -> Result<(u8, u32)> {
-        let pos = self.position;
-        let val = self.read_var_u32()?;
-        Ok((self.buffer[pos], val))
-    }
-
     fn read_memarg(&mut self, max_align: u8) -> Result<MemArg> {
         let flags_pos = self.original_position();
         let mut flags = self.read_var_u32()?;
@@ -833,8 +827,8 @@ impl<'a> BinaryReader<'a> {
             0x10 => visitor.visit_call(self.read_var_u32()?),
             0x11 => {
                 let index = self.read_var_u32()?;
-                let (table_byte, table_index) = self.read_first_byte_and_var_u32()?;
-                visitor.visit_call_indirect(index, table_index, table_byte)
+                let table = self.read_table_index_or_zero_if_not_reference_types()?;
+                visitor.visit_call_indirect(index, table)
             }
             0x12 => visitor.visit_return_call(self.read_var_u32()?),
             0x13 => visitor.visit_return_call_indirect(self.read_var_u32()?, self.read_var_u32()?),
@@ -888,12 +882,12 @@ impl<'a> BinaryReader<'a> {
             0x3d => visitor.visit_i64_store16(self.read_memarg(1)?),
             0x3e => visitor.visit_i64_store32(self.read_memarg(2)?),
             0x3f => {
-                let (mem_byte, mem) = self.read_first_byte_and_var_u32()?;
-                visitor.visit_memory_size(mem, mem_byte)
+                let mem = self.read_memory_index_or_zero_if_not_multi_memory()?;
+                visitor.visit_memory_size(mem)
             }
             0x40 => {
-                let (mem_byte, mem) = self.read_first_byte_and_var_u32()?;
-                visitor.visit_memory_grow(mem, mem_byte)
+                let mem = self.read_memory_index_or_zero_if_not_multi_memory()?;
+                visitor.visit_memory_grow(mem)
             }
 
             0x41 => visitor.visit_i32_const(self.read_var_i32()?),
@@ -1754,6 +1748,32 @@ impl<'a> BinaryReader<'a> {
             }
         }
     }
+
+    fn read_memory_index_or_zero_if_not_multi_memory(&mut self) -> Result<u32> {
+        if self.features.multi_memory() {
+            self.read_var_u32()
+        } else {
+            // Before bulk memory this byte was required to be a single zero
+            // byte, not a LEB-encoded zero, so require a precise zero byte.
+            match self.read_u8()? {
+                0 => Ok(0),
+                _ => bail!(self.original_position() - 1, "zero byte expected"),
+            }
+        }
+    }
+
+    fn read_table_index_or_zero_if_not_reference_types(&mut self) -> Result<u32> {
+        if self.features.reference_types() {
+            self.read_var_u32()
+        } else {
+            // Before reference types this byte was required to be a single zero
+            // byte, not a LEB-encoded zero, so require a precise zero byte.
+            match self.read_u8()? {
+                0 => Ok(0),
+                _ => bail!(self.original_position() - 1, "zero byte expected"),
+            }
+        }
+    }
 }
 
 impl<'a> BrTable<'a> {
diff --git a/crates/wasmparser/src/lib.rs b/crates/wasmparser/src/lib.rs
@@ -169,7 +169,7 @@ macro_rules! for_each_operator {
             @mvp BrTable { targets: $crate::BrTable<'a> } => visit_br_table
             @mvp Return => visit_return
             @mvp Call { function_index: u32 } => visit_call
-            @mvp CallIndirect { type_index: u32, table_index: u32, table_byte: u8 } => visit_call_indirect
+            @mvp CallIndirect { type_index: u32, table_index: u32 } => visit_call_indirect
             @tail_call ReturnCall { function_index: u32 } => visit_return_call
             @tail_call ReturnCallIndirect { type_index: u32, table_index: u32 } => visit_return_call_indirect
             @mvp Drop => visit_drop
@@ -203,8 +203,8 @@ macro_rules! for_each_operator {
             @mvp I64Store8 { memarg: $crate::MemArg } => visit_i64_store8
             @mvp I64Store16 { memarg: $crate::MemArg } => visit_i64_store16
             @mvp I64Store32 { memarg: $crate::MemArg } => visit_i64_store32
-            @mvp MemorySize { mem: u32, mem_byte: u8 } => visit_memory_size
-            @mvp MemoryGrow { mem: u32, mem_byte: u8 } => visit_memory_grow
+            @mvp MemorySize { mem: u32 } => visit_memory_size
+            @mvp MemoryGrow { mem: u32 } => visit_memory_grow
             @mvp I32Const { value: i32 } => visit_i32_const
             @mvp I64Const { value: i64 } => visit_i64_const
             @mvp F32Const { value: $crate::Ieee32 } => visit_f32_const
diff --git a/crates/wasmparser/src/validator/operators.rs b/crates/wasmparser/src/validator/operators.rs
@@ -1584,18 +1584,7 @@ where
         self.check_func_type_index_same_results(type_index)?;
         Ok(())
     }
-    fn visit_call_indirect(
-        &mut self,
-        index: u32,
-        table_index: u32,
-        table_byte: u8,
-    ) -> Self::Output {
-        if table_byte != 0 && !self.features.reference_types() {
-            bail!(
-                self.offset,
-                "reference-types not enabled: zero byte expected"
-            );
-        }
+    fn visit_call_indirect(&mut self, index: u32, table_index: u32) -> Self::Output {
         self.check_call_indirect(index, table_index)?;
         Ok(())
     }
@@ -1928,18 +1917,12 @@ where
         self.pop_operand(Some(ty))?;
         Ok(())
     }
-    fn visit_memory_size(&mut self, mem: u32, mem_byte: u8) -> Self::Output {
-        if mem_byte != 0 && !self.features.multi_memory() {
-            bail!(self.offset, "multi-memory not enabled: zero byte expected");
-        }
+    fn visit_memory_size(&mut self, mem: u32) -> Self::Output {
         let index_ty = self.check_memory_index(mem)?;
         self.push_operand(index_ty)?;
         Ok(())
     }
-    fn visit_memory_grow(&mut self, mem: u32, mem_byte: u8) -> Self::Output {
-        if mem_byte != 0 && !self.features.multi_memory() {
-            bail!(self.offset, "multi-memory not enabled: zero byte expected");
-        }
+    fn visit_memory_grow(&mut self, mem: u32) -> Self::Output {
         let index_ty = self.check_memory_index(mem)?;
         self.pop_operand(Some(index_ty))?;
         self.push_operand(index_ty)?;
diff --git a/crates/wasmprinter/src/operator.rs b/crates/wasmprinter/src/operator.rs
@@ -421,13 +421,12 @@ macro_rules! define_visit {
     // when an index is 0 or similar. The final case in this list is the
     // catch-all which prints each payload individually based on the name of the
     // payload field.
-    (payload $self:ident CallIndirect $ty:ident $table:ident $byte:ident) => (
+    (payload $self:ident CallIndirect $ty:ident $table:ident) => (
         if $table != 0 {
             $self.push_str(" ");
             $self.table_index($table)?;
         }
         $self.type_index($ty)?;
-        let _ = $byte;
     );
     (payload $self:ident ReturnCallIndirect $ty:ident $table:ident) => (
         if $table != 0 {
@@ -469,7 +468,13 @@ macro_rules! define_visit {
             $self.table_index($src)?;
         }
     );
-    (payload $self:ident $mem_op:ident $mem:ident mem_byte) => (
+    (payload $self:ident MemoryGrow $mem:ident) => (
+        if $mem != 0 {
+            $self.push_str(" ");
+            $self.memory_index($mem)?;
+        }
+    );
+    (payload $self:ident MemorySize $mem:ident) => (
         if $mem != 0 {
             $self.push_str(" ");
             $self.memory_index($mem)?;
diff --git a/crates/wit-component/src/gc.rs b/crates/wit-component/src/gc.rs
@@ -1033,8 +1033,6 @@ macro_rules! define_visit {
     (mark_live $self:ident $arg:ident lanes) => {};
     (mark_live $self:ident $arg:ident flags) => {};
     (mark_live $self:ident $arg:ident value) => {};
-    (mark_live $self:ident $arg:ident mem_byte) => {};
-    (mark_live $self:ident $arg:ident table_byte) => {};
     (mark_live $self:ident $arg:ident local_index) => {};
     (mark_live $self:ident $arg:ident relative_depth) => {};
     (mark_live $self:ident $arg:ident tag_index) => {};
@@ -1186,21 +1184,12 @@ macro_rules! define_encode {
     (mk BrTable $arg:ident) => ({
         BrTable($arg.0, $arg.1)
     });
-    (mk CallIndirect $ty:ident $table:ident $table_byte:ident) => ({
-        let _ = $table_byte;
+    (mk CallIndirect $ty:ident $table:ident) => ({
         CallIndirect { ty: $ty, table: $table }
     });
     (mk ReturnCallIndirect $ty:ident $table:ident) => (
         ReturnCallIndirect { ty: $ty, table: $table }
     );
-    (mk MemorySize $mem:ident $mem_byte:ident) => ({
-        let _ = $mem_byte;
-        MemorySize($mem)
-    });
-    (mk MemoryGrow $mem:ident $mem_byte:ident) => ({
-        let _ = $mem_byte;
-        MemoryGrow($mem)
-    });
     (mk TryTable $try_table:ident) => ({
         let _ = $try_table;
         unimplemented_try_table()
diff --git a/tests/cli/dump-llvm-object.wat.stdout b/tests/cli/dump-llvm-object.wat.stdout
@@ -102,7 +102,7 @@
   0xf2 | 20 03       | local_get local_index:3
   0xf4 | 24 00       | global_set global_index:0
   0xf6 | 20 00       | local_get local_index:0
-  0xf8 | 11 05 00    | call_indirect type_index:5 table_index:0 table_byte:0
+  0xf8 | 11 05 00    | call_indirect type_index:5 table_index:0
   0xfb | 41 10       | i32_const value:16
   0xfd | 21 04       | local_set local_index:4
   0xff | 20 03       | local_get local_index:3

Original file line number	Diff line number	Diff line change
`@@ -640,11 +640,11 @@ impl<'a> DFGBuilder {`
`640`	`640`	`idx,`
`641`	`641`	`);`
`642`	`642`	`}`
`643`		`- Operator::MemoryGrow { mem, mem_byte: _ } => {`
	`643`	`+ Operator::MemoryGrow { mem } => {`
`644`	`644`	`let arg = self.pop_operand(idx, false);`
`645`	`645`	`self.push_node(Lang::MemoryGrow(*mem, Id::from(arg)), idx);`
`646`	`646`	`}`
`647`		`- Operator::MemorySize { mem, mem_byte: _ } => {`
	`647`	`+ Operator::MemorySize { mem } => {`
`648`	`648`	`self.push_node(Lang::MemorySize(*mem), idx);`
`649`	`649`	`}`
`650`	`650`	`Operator::TableGrow { table } => {`