From cb9051729f4c96c9797a756f85c114d40c2e3508 Mon Sep 17 00:00:00 2001 From: nerodesu017 Date: Mon, 2 Dec 2024 13:35:13 +0200 Subject: [PATCH] wip: feedback --- src/core/error.rs | 4 +- src/core/reader/types/element.rs | 4 +- src/core/reader/types/mod.rs | 15 +---- src/execution/const_interpreter_loop.rs | 19 ++---- src/execution/interpreter_loop.rs | 13 ++-- src/execution/mod.rs | 16 ++--- src/execution/value.rs | 78 ++++++---------------- src/validation/read_constant_expression.rs | 2 +- tests/table.rs | 10 +-- tests/table_fill.rs | 2 +- tests/table_grow.rs | 1 + 11 files changed, 53 insertions(+), 111 deletions(-) diff --git a/src/core/error.rs b/src/core/error.rs index 79e4b603..f74e88dc 100644 --- a/src/core/error.rs +++ b/src/core/error.rs @@ -173,7 +173,7 @@ impl Display for Error { elem_idx )), Error::DifferentRefTypes(rref1, rref2) => f.write_fmt(format_args!( - "RefType {} is NOT equal to RefType {}", + "RefType {:?} is NOT equal to RefType {:?}", rref1, rref2 )), Error::ExpectedARefType(found_valtype) => f.write_fmt(format_args!( @@ -181,7 +181,7 @@ impl Display for Error { found_valtype )), Error::WrongRefTypeForInteropValue(ref_given, ref_wanted) => f.write_fmt(format_args!( - "Wrong RefType for InteropValue: Given {} - Needed {}", + "Wrong RefType for InteropValue: Given {:?} - Needed {:?}", ref_given, ref_wanted )), Error::FunctionIsNotDefined(func_idx) => f.write_fmt(format_args!( diff --git a/src/core/reader/types/element.rs b/src/core/reader/types/element.rs index 811141b0..6b19945c 100644 --- a/src/core/reader/types/element.rs +++ b/src/core/reader/types/element.rs @@ -18,7 +18,7 @@ impl Debug for ElemType { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { write!( f, - "ElemType {{\n\tinit: {:?},\n\tmode: {:?},\n\t#ty: {}\n}}", + "ElemType {{\n\tinit: {:?},\n\tmode: {:?},\n\t#ty: {:?}\n}}", self.init, self.mode, self.init.ty() @@ -105,7 +105,7 @@ impl ElemType { }; match reftype_or_elemkind { - Some(rty) => trace!("REFTYPE: {}", rty), + Some(rty) => trace!("REFTYPE: {:?}", rty), None => { trace!("REFTYPE NONE!") } diff --git a/src/core/reader/types/mod.rs b/src/core/reader/types/mod.rs index 2c241810..f61856b3 100644 --- a/src/core/reader/types/mod.rs +++ b/src/core/reader/types/mod.rs @@ -3,7 +3,7 @@ //! See: use alloc::vec::Vec; -use core::fmt::{Debug, Display, Formatter}; +use core::fmt::{Debug, Formatter}; use crate::core::reader::{WasmReadable, WasmReader}; use crate::execution::assert_validated::UnwrapValidatedExt; @@ -87,19 +87,6 @@ pub enum RefType { ExternRef, } -impl Display for RefType { - fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { - write!( - f, - "{}", - match self { - Self::ExternRef => "ExternRef", - Self::FuncRef => "FuncRef", - } - ) - } -} - impl RefType { /// TODO: we have to make sure they are NOT null Refs, but still, they are not valid ones as we cast them from RefTypes which don't hold addresses per-se pub fn to_null_ref(&self) -> Ref { diff --git a/src/execution/const_interpreter_loop.rs b/src/execution/const_interpreter_loop.rs index 5f98d0e0..45b15eb8 100644 --- a/src/execution/const_interpreter_loop.rs +++ b/src/execution/const_interpreter_loop.rs @@ -6,8 +6,6 @@ use crate::{ NumType, RefType, ValType, Value, }; -use super::store::FuncInst; - /// Execute a previosly-validated constant expression. These type of expressions are used for initializing global /// variables. /// @@ -33,7 +31,6 @@ pub(crate) fn run_const( mut wasm: WasmReader, stack: &mut Stack, _imported_globals: (), /*todo!*/ - funcs: &[FuncInst], ) { use crate::core::reader::types::opcode::*; loop { @@ -105,17 +102,11 @@ pub(crate) fn run_const( let reftype = RefType::read_unvalidated(&mut wasm); stack.push_value(Value::Ref(reftype.to_null_ref())); - trace!("Instruction: ref.null '{}' -> [{}]", reftype, reftype); + trace!("Instruction: ref.null '{:?}' -> [{:?}]", reftype, reftype); } REF_FUNC => { - // not unwrap validated because, guess what? not validated - let func_idx = wasm.read_var_u32().unwrap() as usize; - if func_idx >= funcs.len() { - panic!( - "Out of bounds ref.func ({func_idx}) access (max: {})", - funcs.len() - ); - } + // we already checked for the func_idx to be in bounds during validation + let func_idx = wasm.read_var_u32().unwrap_validated() as usize; stack.push_value(Value::Ref(Ref::Func(FuncAddr::new(Some(func_idx))))); } other => { @@ -129,14 +120,14 @@ pub(crate) fn run_const_span( wasm: &[u8], span: &Span, imported_globals: (), - funcs: &[FuncInst], + // funcs: &[FuncInst], ) -> Option { let mut wasm = WasmReader::new(wasm); wasm.move_start_to(*span).unwrap_validated(); let mut stack = Stack::new(); - run_const(wasm, &mut stack, imported_globals, funcs); + run_const(wasm, &mut stack, imported_globals); stack.peek_unknown_value() } diff --git a/src/execution/interpreter_loop.rs b/src/execution/interpreter_loop.rs index 1b148e99..ef376387 100644 --- a/src/execution/interpreter_loop.rs +++ b/src/execution/interpreter_loop.rs @@ -143,7 +143,10 @@ pub(super) fn run( Ref::Extern(_) => unreachable!(), }; - let func_to_call_inst = store.funcs.get(func_addr).unwrap_validated(); + let func_to_call_inst = store + .funcs + .get(func_addr.unwrap_validated()) + .unwrap_validated(); let func_ty_actual_index = func_to_call_inst.ty; @@ -156,7 +159,7 @@ pub(super) fn run( trace!("Instruction: call_indirect [{func_addr:?}]"); let locals = Locals::new(params, remaining_locals); - stack.push_stackframe(func_addr, func_ty, locals, wasm.pc); + stack.push_stackframe(func_addr.unwrap_validated(), func_ty, locals, wasm.pc); wasm.move_start_to(func_to_call_inst.code_expr) .unwrap_validated(); @@ -1982,13 +1985,13 @@ pub(super) fn run( let reftype = RefType::read_unvalidated(&mut wasm); stack.push_value(Value::Ref(reftype.to_null_ref())); - trace!("Instruction: ref.null '{}' -> [{}]", reftype, reftype); + trace!("Instruction: ref.null '{:?}' -> [{:?}]", reftype, reftype); } REF_IS_NULL => { let rref = stack.pop_unknown_ref(); let is_null = match rref { - Ref::Extern(rref) => rref.is_null, - Ref::Func(rref) => rref.is_null, + Ref::Extern(rref) => rref.addr.is_none(), + Ref::Func(rref) => rref.addr.is_none(), }; let res = if is_null { 1 } else { 0 }; diff --git a/src/execution/mod.rs b/src/execution/mod.rs index 38ca9338..a9c8e876 100644 --- a/src/execution/mod.rs +++ b/src/execution/mod.rs @@ -366,8 +366,7 @@ where .iter() .map(|expr| { get_address_offset( - run_const_span(validation_info.wasm, expr, (), &function_instances) - .unwrap_validated(), + run_const_span(validation_info.wasm, expr, ()).unwrap_validated(), ) }) .collect(), @@ -410,13 +409,8 @@ where assert!(tables.len() > table_idx); let offset = get_address_offset( - run_const_span( - validation_info.wasm, - &active_elem.offset, - (), - &function_instances, - ) - .unwrap_validated(), + run_const_span(validation_info.wasm, &active_elem.offset, ()) + .unwrap_validated(), ) as usize; let table = &mut tables[table_idx]; @@ -457,7 +451,7 @@ where let mut wasm = WasmReader::new(validation_info.wasm); wasm.move_start_to(active_data.offset).unwrap_validated(); let mut stack = Stack::new(); - run_const(wasm, &mut stack, (), &[]); + run_const(wasm, &mut stack, ()); let value = stack.peek_unknown_value(); if value.is_none() { panic!("No value on the stack for data segment offset"); @@ -508,7 +502,7 @@ where wasm.move_start_to(global.init_expr).unwrap_validated(); // We shouldn't need to clear the stack. If validation is correct, it will remain empty after execution. - run_const(wasm, &mut stack, (), &[]); + run_const(wasm, &mut stack, ()); let value = stack.pop_value(global.ty.ty); GlobalInst { diff --git a/src/execution/value.rs b/src/execution/value.rs index 17044578..1de46ac3 100644 --- a/src/execution/value.rs +++ b/src/execution/value.rs @@ -280,14 +280,14 @@ impl Ref { pub fn is_null(&self) -> bool { match self { - Self::Extern(extern_addr) => extern_addr.is_null, - Self::Func(func_addr) => func_addr.is_null, + Self::Extern(extern_addr) => extern_addr.addr.is_none(), + Self::Func(func_addr) => func_addr.addr.is_none(), } } pub fn is_specific_func(&self, func_id: u32) -> bool { match self { - Self::Func(func_addr) => !func_addr.is_null && func_addr.addr == func_id as usize, + Self::Func(func_addr) => func_addr.addr == Some(func_id as usize), _ => unreachable!(), } } @@ -296,23 +296,21 @@ impl Ref { impl Display for Ref { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { - Ref::Func(func_addr) => write!(f, "FuncRef({})", func_addr), - Ref::Extern(extern_addr) => write!(f, "ExternRef({})", extern_addr), + Ref::Func(func_addr) => write!(f, "FuncRef({:?})", func_addr), + Ref::Extern(extern_addr) => write!(f, "ExternRef({:?})", extern_addr), } } } #[derive(Clone, Copy, PartialEq)] pub struct FuncAddr { - pub is_null: bool, - // it is the idx of the function in the current module - pub addr: usize, + pub addr: Option, } impl Debug for FuncAddr { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - match self.is_null { - false => write!(f, "FuncAddr {{\n\taddr: {}\n}}", self.addr), + match self.addr.is_none() { + false => write!(f, "FuncAddr {{\n\taddr: {}\n}}", self.addr.unwrap()), true => write!(f, "FuncAddr {{ NULL }}"), } } @@ -322,28 +320,24 @@ impl FuncAddr { pub fn new(addr: Option) -> Self { match addr { None => Self::null(), - Some(u) => Self { - addr: u, - is_null: false, - }, + Some(u) => Self { addr: Some(u) }, } } pub fn null() -> Self { - Self { - addr: 0, - is_null: true, - } + Self { addr: None } } pub fn get_value(&self) -> usize { - if self.is_null { - Self::get_reserved_value() as usize - } else { - self.addr + match self.addr { + None => Self::get_reserved_value() as usize, + Some(val) => val, } } pub fn get_reserved_value() -> u32 { u32::MAX } + pub fn is_null(&self) -> bool { + self.addr.is_none() + } } impl Default for FuncAddr { @@ -352,43 +346,25 @@ impl Default for FuncAddr { } } -impl Display for FuncAddr { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - if self.is_null { - write!(f, "FuncAddr {{ NULL }}") - } else { - write!(f, "FuncAddr {{ addr: {} }}", self.addr) - } - } -} - #[derive(Clone, Copy, Debug, PartialEq)] pub struct ExternAddr { - pub is_null: bool, - pub addr: usize, + pub addr: Option, } impl ExternAddr { pub fn new(addr: Option) -> Self { match addr { None => Self::null(), - Some(u) => Self { - addr: u, - is_null: false, - }, + Some(u) => Self { addr: Some(u) }, } } pub fn null() -> Self { - Self { - addr: 0, - is_null: true, - } + Self { addr: None } } pub fn get_value(&self) -> usize { - if self.is_null { - Self::get_reserved_value() as usize - } else { - self.addr + match self.addr { + None => Self::get_reserved_value() as usize, + Some(val) => val, } } pub fn get_reserved_value() -> u32 { @@ -402,16 +378,6 @@ impl Default for ExternAddr { } } -impl Display for ExternAddr { - fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { - if self.is_null { - write!(f, "ExternAddr {{ NULL }}") - } else { - write!(f, "ExternAddr {{ addr: () }}") // No actual addr, so printing () - } - } -} - #[derive(Clone, Copy, Debug, PartialEq)] pub enum RefValueTy { Func, diff --git a/src/validation/read_constant_expression.rs b/src/validation/read_constant_expression.rs index dac0ab75..d760c530 100644 --- a/src/validation/read_constant_expression.rs +++ b/src/validation/read_constant_expression.rs @@ -132,7 +132,7 @@ pub fn read_constant_instructions( stack.push_valtype(ValType::RefType(RefType::read(wasm)?)); } REF_FUNC => { - let func_idx = wasm.read_var_u32().unwrap() as usize; + let func_idx = wasm.read_var_u32()? as usize; match funcs { Some(funcs) => { if func_idx >= funcs.len() { diff --git a/tests/table.rs b/tests/table.rs index c51bc821..c6a1a00a 100644 --- a/tests/table.rs +++ b/tests/table.rs @@ -129,8 +129,8 @@ fn table_elem_test() { .for_each(|(i, rref)| match *rref { wasm::value::Ref::Extern(_) => panic!(), wasm::value::Ref::Func(func_addr) => { - assert!(!func_addr.is_null); - assert!(wanted[i] == func_addr.addr) + assert!(func_addr.addr.is_some()); + assert!(wanted[i] == func_addr.addr.unwrap()) } }); // assert!(instance.store.tables) @@ -170,7 +170,7 @@ fn table_get_set_test() { match rref { Ref::Func(funcaddr) => { - assert!(!funcaddr.is_null) + assert!(!funcaddr.is_null()) } _ => panic!("Expected a FuncRef"), } @@ -186,7 +186,7 @@ fn table_get_set_test() { match rref { Ref::Func(funcaddr) => { - assert!(funcaddr.is_null) + assert!(funcaddr.is_null()) } _ => panic!("Expected a FuncRef"), } @@ -204,7 +204,7 @@ fn table_get_set_test() { match rref { Ref::Func(funcaddr) => { - assert!(!funcaddr.is_null) + assert!(!funcaddr.is_null()) } _ => panic!("Expected a FuncRef"), } diff --git a/tests/table_fill.rs b/tests/table_fill.rs index 9e615a7d..bf091093 100644 --- a/tests/table_fill.rs +++ b/tests/table_fill.rs @@ -26,7 +26,7 @@ macro_rules! get_func { macro_rules! is_specific_func { ($self:expr, $func_id:expr) => { match $self { - Ref::Func(func_addr) => !func_addr.is_null && func_addr.addr == $func_id as usize, + Ref::Func(func_addr) => func_addr.addr == Some($func_id as usize), _ => unimplemented!(), } }; diff --git a/tests/table_grow.rs b/tests/table_grow.rs index 6df4121d..78401330 100644 --- a/tests/table_grow.rs +++ b/tests/table_grow.rs @@ -373,6 +373,7 @@ fn table_grow_with_exported_table_test() { // assert_result!(import2_instance, size, (), 3); // } +// TODO: we can NOT run this test yet because ??? #[ignore = "table grow type errors"] #[test_log::test] fn table_grow_type_errors() {