review feedback

Author: fitzgen

crates/cranelift/src/translate/code_translator.rs

@@ -2860,8 +2860,6 @@ pub fn translate_operator(
         Operator::BrOnCast {
             relative_depth,
             to_ref_type,
-            // TODO: we should take advantage of our knowledge of the type we
-            // are casting from when generating the test.
             from_ref_type: _,
         } => {
             let r = state.peek1();
@@ -2896,8 +2894,6 @@ pub fn translate_operator(
         Operator::BrOnCastFail {
             relative_depth,
             to_ref_type,
-            // TODO: we should take advantage of our knowledge of the type we
-            // are casting from when generating the test.
             from_ref_type: _,
         } => {
             let r = state.peek1();

crates/cranelift/src/translate/func_translator.rs

@@ -250,21 +250,8 @@ fn parse_function_body(
         builder.set_srcloc(cur_srcloc(&reader.get_binary_reader()));
 
         let op = reader.read()?;
-
-        // Get the operand types for this operator.
-        let arity = op.operator_arity(&*validator);
-        operand_types.clear();
-        let operand_types = arity.and_then(|(operand_arity, _result_arity)| {
-            for i in (0..operand_arity).rev() {
-                let i = usize::try_from(i).unwrap();
-                let ty = validator.get_operand_type(i)??;
-                let ty = environ.convert_valtype(ty).ok()?;
-                operand_types.push(ty);
-            }
-            Some(&operand_types[..])
-        });
-
-        validator.op(pos, &op)?;
+        let operand_types =
+            validate_op_and_get_operand_types(validator, environ, &mut operand_types, &op, pos)?;
 
         environ.before_translate_operator(&op, operand_types, builder, state)?;
         translate_operator(validator, &op, operand_types, builder, state, environ)?;
@@ -293,6 +280,40 @@ fn parse_function_body(
     Ok(())
 }
 
+fn validate_op_and_get_operand_types<'a>(
+    validator: &mut FuncValidator<impl WasmModuleResources>,
+    environ: &mut FuncEnvironment<'_>,
+    operand_types: &'a mut Vec<wasmtime_environ::WasmValType>,
+    op: &wasmparser::Operator<'_>,
+    pos: usize,
+) -> WasmResult<Option<&'a [wasmtime_environ::WasmValType]>> {
+    // Get the operand types for this operator.
+    //
+    // Note that we don't know if the `op` is valid yet, but only valid ops will
+    // definitely have arity. However, we also must check the arity before
+    // validating the op so that the validator has the right state to correctly
+    // report the arity. Furthermore, even if the op is valid, if it is in
+    // unreachable code, the op might want to pop more values from the stack
+    // than actually exist on the stack (which is allowed in unreachable code)
+    // so even if we can get arity, we are only guaranteed to have operand types
+    // for ops that are not only valid but also reachable.
+    let arity = op.operator_arity(&*validator);
+    operand_types.clear();
+    let operand_types = arity.and_then(|(operand_arity, _result_arity)| {
+        for i in (0..operand_arity).rev() {
+            let i = usize::try_from(i).unwrap();
+            let ty = validator.get_operand_type(i)??;
+            let ty = environ.convert_valtype(ty).ok()?;
+            operand_types.push(ty);
+        }
+        Some(&operand_types[..])
+    });
+
+    validator.op(pos, &op)?;
+
+    Ok(operand_types)
+}
+
 /// Get the current source location from a reader.
 fn cur_srcloc(reader: &BinaryReader) -> ir::SourceLoc {
     // We record source locations as byte code offsets relative to the beginning of the file.