Repository: bytecodealliance/wasm-tools
Pull request #1720 (2024-08-13 20:47:12)

Test `wasm_encoder::reencode` in roundtrip tests
Run all binaries in the spec test suite and our own through the `wasm_encoder::reencode` process to make sure the final binary is byte-for-byte identical in applicable situations. This fixes a few discrepancies along the way with value type encodings (wasm-encoder was producing some slightly longer forms) and additionally improves support for constant expressions by being a bit more general in the reencoding process.


Patch:
diff --git a/crates/wasm-encoder/src/core/types.rs b/crates/wasm-encoder/src/core/types.rs
index 23859d06..8b05ac4f 100644
--- a/crates/wasm-encoder/src/core/types.rs
+++ b/crates/wasm-encoder/src/core/types.rs
@@ -328,29 +328,34 @@ impl RefType {
 
 impl Encode for RefType {
     fn encode(&self, sink: &mut Vec<u8>) {
-        if self.nullable {
-            // Favor the original encodings of `funcref` and `externref` where
-            // possible.
-            use AbstractHeapType::*;
-            match self.heap_type {
-                HeapType::Abstract {
-                    shared: false,
-                    ty: Func,
-                } => return sink.push(0x70),
-                HeapType::Abstract {
-                    shared: false,
-                    ty: Extern,
-                } => return sink.push(0x6f),
-                _ => {}
+        match self {
+            // Binary abbreviations (i.e., short form), for when the ref is
+            // nullable.
+            RefType {
+                nullable: true,
+                heap_type: heap @ HeapType::Abstract { .. },
+            } => {
+                heap.encode(sink);
             }
-        }
 
-        if self.nullable {
-            sink.push(0x63);
-        } else {
-            sink.push(0x64);
+            // Generic 'ref null <heaptype>' encoding (i.e., long form).
+            RefType {
+                nullable: true,
+                heap_type,
+            } => {
+                sink.push(0x63);
+                heap_type.encode(sink);
+            }
+
+            // Generic 'ref <heaptype>' encoding.
+            RefType {
+                nullable: false,
+                heap_type,
+            } => {
+                sink.push(0x64);
+                heap_type.encode(sink);
+            }
         }
-        self.heap_type.encode(sink);
     }
 }
 
diff --git a/crates/wasm-encoder/src/reencode.rs b/crates/wasm-encoder/src/reencode.rs
index 8c1111d4..b98d9ce7 100644
--- a/crates/wasm-encoder/src/reencode.rs
+++ b/crates/wasm-encoder/src/reencode.rs
@@ -286,12 +286,11 @@ pub trait Reencode {
     }
 
     /// Parses a single instruction from `reader` and adds it to `function`.
-    fn parse_instruction(
+    fn parse_instruction<'a>(
         &mut self,
-        function: &mut crate::Function,
-        reader: &mut wasmparser::OperatorsReader<'_>,
-    ) -> Result<(), Error<Self::Error>> {
-        utils::parse_instruction(self, function, reader)
+        reader: &mut wasmparser::OperatorsReader<'a>,
+    ) -> Result<crate::Instruction<'a>, Error<Self::Error>> {
+        utils::parse_instruction(self, reader)
     }
 
     /// Parses the input `section` given from the `wasmparser` crate and adds
@@ -570,6 +569,7 @@ impl Reencode for RoundtripReencoder {
 #[allow(missing_docs)] // FIXME
 pub mod utils {
     use super::{Error, Reencode};
+    use crate::Encode;
 
     pub fn parse_core_module<T: ?Sized + Reencode>(
         reencoder: &mut T,
@@ -1574,45 +1574,15 @@ pub mod utils {
         reencoder: &mut T,
         const_expr: wasmparser::ConstExpr,
     ) -> Result<crate::ConstExpr, Error<T::Error>> {
-        let mut ops = const_expr.get_operators_reader().into_iter();
-
-        let result = match ops.next() {
-            Some(Ok(wasmparser::Operator::I32Const { value })) => {
-                crate::ConstExpr::i32_const(value)
-            }
-            Some(Ok(wasmparser::Operator::I64Const { value })) => {
-                crate::ConstExpr::i64_const(value)
-            }
-            Some(Ok(wasmparser::Operator::F32Const { value })) => {
-                crate::ConstExpr::f32_const(f32::from_bits(value.bits()))
-            }
-            Some(Ok(wasmparser::Operator::F64Const { value })) => {
-                crate::ConstExpr::f64_const(f64::from_bits(value.bits()))
-            }
-            Some(Ok(wasmparser::Operator::V128Const { value })) => {
-                crate::ConstExpr::v128_const(i128::from_le_bytes(*value.bytes()))
-            }
-            Some(Ok(wasmparser::Operator::RefNull { hty })) => {
-                crate::ConstExpr::ref_null(reencoder.heap_type(hty)?)
-            }
-            Some(Ok(wasmparser::Operator::RefFunc { function_index })) => {
-                crate::ConstExpr::ref_func(reencoder.function_index(function_index))
-            }
-            Some(Ok(wasmparser::Operator::GlobalGet { global_index })) => {
-                crate::ConstExpr::global_get(reencoder.global_index(global_index))
-            }
-
-            // TODO: support the extended-const proposal.
-            Some(Ok(_op)) => return Err(Error::InvalidConstExpr),
-
-            Some(Err(e)) => return Err(Error::ParseError(e)),
-            None => return Err(Error::InvalidConstExpr),
-        };
+        let mut ops = const_expr.get_operators_reader();
+        let mut bytes = Vec::new();
 
-        match (ops.next(), ops.next()) {
-            (Some(Ok(wasmparser::Operator::End)), None) => Ok(result),
-            _ => Err(Error::InvalidConstExpr),
+        while !ops.is_end_then_eof() {
+            let insn = reencoder.parse_instruction(&mut ops)?;
+            insn.encode(&mut bytes);
         }
+
+        Ok(crate::ConstExpr::raw(bytes))
     }
 
     pub fn block_type<T: ?Sized + Reencode>(
@@ -1736,7 +1706,7 @@ pub mod utils {
         let mut f = reencoder.new_function_with_parsed_locals(&func)?;
         let mut reader = func.get_operators_reader()?;
         while !reader.eof() {
-            reencoder.parse_instruction(&mut f, &mut reader)?;
+            f.instruction(&reencoder.parse_instruction(&mut reader)?);
         }
         code.function(&f);
         Ok(())
@@ -1757,13 +1727,12 @@ pub mod utils {
     }
 
     /// Parses a single instruction from `reader` and adds it to `function`.
-    pub fn parse_instruction<T: ?Sized + Reencode>(
+    pub fn parse_instruction<'a, T: ?Sized + Reencode>(
         reencoder: &mut T,
-        function: &mut crate::Function,
-        reader: &mut wasmparser::OperatorsReader<'_>,
-    ) -> Result<(), Error<T::Error>> {
-        function.instruction(&reencoder.instruction(reader.read()?)?);
-        Ok(())
+        reader: &mut wasmparser::OperatorsReader<'a>,
+    ) -> Result<crate::Instruction<'a>, Error<T::Error>> {
+        let instruction = reencoder.instruction(reader.read()?)?;
+        Ok(instruction)
     }
 
     pub fn parse_unknown_section<T: ?Sized + Reencode>(
diff --git a/crates/wast/src/core/binary.rs b/crates/wast/src/core/binary.rs
index 5007c425..b64e559a 100644
--- a/crates/wast/src/core/binary.rs
+++ b/crates/wast/src/core/binary.rs
@@ -458,12 +458,9 @@ impl<'a> Encode for RefType<'a> {
             // nullable.
             RefType {
                 nullable: true,
-                heap: HeapType::Abstract { shared, ty },
+                heap: heap @ HeapType::Abstract { .. },
             } => {
-                if *shared {
-                    e.push(0x65);
-                }
-                ty.encode(e);
+                heap.encode(e);
             }
 
             // Generic 'ref null <heaptype>' encoding (i.e., long form).
diff --git a/crates/wit-component/src/gc.rs b/crates/wit-component/src/gc.rs
index 8f8904c0..05b011d9 100644
--- a/crates/wit-component/src/gc.rs
+++ b/crates/wit-component/src/gc.rs
@@ -746,7 +746,7 @@ impl<'a> Module<'a> {
                     func.instruction(&Instruction::Call(lazy_stack_init_index));
                     let mut reader = body.get_operators_reader()?;
                     while !reader.eof() {
-                        map.parse_instruction(&mut func, &mut reader)?;
+                        func.instruction(&map.parse_instruction(&mut reader)?);
                     }
                     code.function(&func);
                 }


Test patch:
diff --git a/tests/roundtrip.rs b/tests/roundtrip.rs
index 62cc04a1..cf1e67e0 100644
--- a/tests/roundtrip.rs
+++ b/tests/roundtrip.rs
@@ -30,6 +30,7 @@ use std::process::{Command, Stdio};
 use std::str;
 use std::sync::atomic::{AtomicUsize, Ordering::SeqCst};
 use std::sync::Arc;
+use wasm_encoder::reencode::{Reencode, RoundtripReencoder};
 use wasmparser::*;
 use wast::core::{Module, ModuleKind};
 use wast::lexer::Lexer;
@@ -178,6 +179,16 @@ impl TestState {
             self.bump_ntests();
             self.binary_compare(&binary2, contents)
                 .context("failed to compare original `wat` with roundtrip `wat`")?;
+
+            if !wasmparser::Parser::is_component(contents) {
+                let mut reencode = Default::default();
+                RoundtripReencoder
+                    .parse_core_module(&mut reencode, wasmparser::Parser::new(0), contents)
+                    .context("failed to reencode module")?;
+
+                self.binary_compare(&reencode.finish(), contents)
+                    .context("failed to compare reencoded module with original encoding")?;
+            }
         }
 
         // Test that the `wasmprinter`-printed bytes have "pretty" whitespace
@@ -526,8 +537,9 @@ impl TestState {
         dump.stdin.take().unwrap().write_all(bytes)?;
         let mut stdout = String::new();
         dump.stdout.take().unwrap().read_to_string(&mut stdout)?;
-        if dump.wait()?.success() {
-            bail!("dump subcommand failed");
+        let status = dump.wait()?;
+        if !status.success() {
+            bail!("dump subcommand failed: {status}");
         }
         Ok(stdout)
     }