Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert panics to errors #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/core/error.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::core::indices::GlobalIdx;
use crate::core::indices::{FuncIdx, GlobalIdx};
use core::fmt::{Display, Formatter};
use core::str::Utf8Error;

Expand Down Expand Up @@ -38,8 +38,12 @@ pub enum Error {
InvalidMutType(u8),
MoreThanOneMemory,
InvalidGlobalIdx(GlobalIdx),
InvalidFunctionIdx(FuncIdx),
GlobalIsConst,
RuntimeError(RuntimeError),
NotYetImplemented(&'static str),
InvalidParameterTypes,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opposing to introduce this, is there any reason to not just use todo!()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While sketching out a runner for the wasm specsuite, I ran into the todo!s for the sections a lot of times. I wanted to have avoid a catch_unwind call more due to esthetic reasons.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also against introducing this error variant. IMO it's okay to have catch_unwind in the test runner. Maybe we could even check if the panic message includes "not yet implemented" and display a simple statistic.

InvalidResultTypes,
}

impl Display for Error {
Expand Down Expand Up @@ -108,12 +112,24 @@ impl Display for Error {
Error::InvalidGlobalIdx(idx) => f.write_fmt(format_args!(
"An invalid global index `{idx}` was specified"
)),
Error::InvalidFunctionIdx(idx) => f.write_fmt(format_args!(
"An invalid function index `{idx}` was specified"
)),
Error::GlobalIsConst => f.write_str("A const global cannot be written to"),
Error::RuntimeError(err) => match err {
RuntimeError::DivideBy0 => f.write_str("Divide by zero is not permitted"),
RuntimeError::UnrepresentableResult => f.write_str("Result is unrepresentable"),
RuntimeError::FunctionNotFound => f.write_str("Function not found"),
},
Error::NotYetImplemented(msg) => {
f.write_fmt(format_args!("Not yet implemented: {msg}"))
}
Error::InvalidParameterTypes => {
f.write_str("The given parameter types do not match the function's signature")
}
Error::InvalidResultTypes => {
f.write_str("The given result types do not match the function's signature")
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/reader/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ impl<'a> WasmReader<'a> {
let bytes = &self.full_wasm_binary[self.pc..(self.pc + N)];
self.pc += N;

Ok(bytes.try_into().expect("the slice length to be exactly N"))
bytes.try_into().map_err(|_err| Error::Eof)
}

/// Read the current byte without advancing the [`pc`](Self::pc)
Expand Down
2 changes: 1 addition & 1 deletion src/core/reader/section_header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl WasmReadable for SectionHeader {
let size: u32 = wasm.read_var_u32().unwrap_validated();
let contents_span = wasm
.make_span(size as usize)
.expect("TODO remove this expect");
.expect("Expected to be able to read section due to prior validation");

SectionHeader {
ty,
Expand Down
6 changes: 3 additions & 3 deletions src/core/reader/types/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ impl WasmReadable for ImportDesc {
fn read(wasm: &mut WasmReader) -> Result<Self> {
let desc = match wasm.read_u8()? {
0x00 => Self::Func(wasm.read_var_u32()? as TypeIdx),
0x01 => todo!("read TableType"),
0x02 => todo!("read MemType"),
0x03 => todo!("read GlobalType"),
0x01 => return Err(Error::NotYetImplemented("read TableType")),
0x02 => return Err(Error::NotYetImplemented("read MemType")),
0x03 => return Err(Error::NotYetImplemented("read GlobalType")),
other => return Err(Error::InvalidImportDesc(other)),
};

Expand Down
59 changes: 35 additions & 24 deletions src/execution/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use crate::execution::store::{FuncInst, GlobalInst, MemInst, Store};
use crate::execution::value::Value;
use crate::validation::code::read_declared_locals;
use crate::value::InteropValueList;
use crate::Error::RuntimeError;
use crate::Error::{self, RuntimeError};
use crate::RuntimeError::{DivideBy0, FunctionNotFound, UnrepresentableResult};
use crate::{Result, ValidationInfo};

Expand Down Expand Up @@ -51,7 +51,7 @@ where
pub fn new_with_hooks(validation_info: &'_ ValidationInfo<'b>, hook_set: H) -> Result<Self> {
trace!("Starting instantiation of bytecode");

let store = Self::init_store(validation_info);
let store = Self::init_store(validation_info)?;

let mut instance = RuntimeInstance {
wasm_bytecode: validation_info.wasm,
Expand Down Expand Up @@ -100,15 +100,20 @@ where
params: Param,
) -> Result<Returns> {
// -=-= Verification =-=-
let func_inst = self.store.funcs.get(func_idx).expect("valid FuncIdx");
// TODO(george-cosma): Do we want this to be a RuntimeError(FunctionNotFound)?
let func_inst = self
.store
.funcs
.get(func_idx)
.ok_or(Error::InvalidFunctionIdx(func_idx))?;
let func_ty = self.types.get(func_inst.ty).unwrap_validated();

// Check correct function parameters and return types
if func_ty.params.valtypes != Param::TYS {
panic!("Invalid `Param` generics");
return Err(Error::InvalidParameterTypes);
}
if func_ty.returns.valtypes != Returns::TYS {
panic!("Invalid `Returns` generics");
return Err(Error::InvalidResultTypes);
}

// -=-= Invoke the function =-=-
Expand Down Expand Up @@ -143,19 +148,24 @@ where
ret_types: &[ValType],
) -> Result<Vec<Value>> {
// -=-= Verification =-=-
let func_inst = self.store.funcs.get(func_idx).expect("valid FuncIdx");
// TODO(george-cosma): Do we want this to be a RuntimeError(FunctionNotFound)?
let func_inst = self
.store
.funcs
.get(func_idx)
.ok_or(Error::InvalidFunctionIdx(func_idx))?;
let func_ty = self.types.get(func_inst.ty).unwrap_validated();

// Verify that the given parameters match the function parameters
let param_types = params.iter().map(|v| v.to_ty()).collect::<Vec<_>>();

if func_ty.params.valtypes != param_types {
panic!("Invalid parameters for function");
return Err(Error::InvalidParameterTypes);
}

// Verify that the given return types match the function return types
if func_ty.returns.valtypes != ret_types {
panic!("Invalid return types for function");
return Err(Error::InvalidResultTypes);
}

// -=-= Invoke the function =-=-
Expand All @@ -169,7 +179,12 @@ where
let error = self.function(func_idx, &mut stack);
error?;

let func_inst = self.store.funcs.get(func_idx).expect("valid FuncIdx");
// TODO(george-cosma): Do we want this to be a RuntimeError(FunctionNotFound)?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote is yes!

let func_inst = self
.store
.funcs
.get(func_idx)
.ok_or(Error::InvalidFunctionIdx(func_idx))?;
let func_ty = self.types.get(func_inst.ty).unwrap_validated();

// Pop return values from stack
Expand Down Expand Up @@ -263,9 +278,9 @@ where
let address = address as usize;
mem.data.get(address..(address + 4))
})
.expect("TODO trap here");
.expect("TODO trap here"); // ???
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an out-of-bounds access to linear memory?


let data: [u8; 4] = data.try_into().expect("this to be exactly 4 bytes");
let data: [u8; 4] = data.try_into().expect("this to be exactly 4 bytes"); // ???
u32::from_le_bytes(data)
};

Expand All @@ -289,7 +304,7 @@ where
let address = address as usize;
mem.data.get_mut(address..(address + 4))
})
.expect("TODO trap here");
.expect("TODO trap here"); // ???

memory_location.copy_from_slice(&data_to_store.to_le_bytes());
trace!("Instruction: i32.store [{relative_address} {data_to_store}] -> []");
Expand Down Expand Up @@ -663,7 +678,7 @@ where
Ok(())
}

fn init_store(validation_info: &ValidationInfo) -> Store {
fn init_store(validation_info: &ValidationInfo) -> Result<Store> {
let function_instances: Vec<FuncInst> = {
let mut wasm_reader = WasmReader::new(validation_info.wasm);

Expand All @@ -673,25 +688,21 @@ where
functions
.zip(func_blocks)
.map(|(ty, func)| {
wasm_reader
.move_start_to(*func)
.expect("function index to be in the bounds of the WASM binary");
wasm_reader.move_start_to(*func)?;

let (locals, bytes_read) = wasm_reader
.measure_num_read_bytes(read_declared_locals)
.unwrap_validated();

let code_expr = wasm_reader
.make_span(func.len() - bytes_read)
.expect("TODO remove this expect");
let code_expr = wasm_reader.make_span(func.len() - bytes_read)?;

FuncInst {
Ok(FuncInst {
ty: *ty,
locals,
code_expr,
}
})
})
.collect()
.collect::<Result<Vec<FuncInst>>>()?
};

let memory_instances: Vec<MemInst> = validation_info
Expand All @@ -714,10 +725,10 @@ where
})
.collect();

Store {
Ok(Store {
funcs: function_instances,
mems: memory_instances,
globals: global_instances,
}
})
}
}
12 changes: 9 additions & 3 deletions src/validation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,17 @@ pub fn validate(wasm: &[u8]) -> Result<ValidationInfo> {
while (skip_section(&mut wasm, &mut header)?).is_some() {}

let _: Option<()> = handle_section(&mut wasm, &mut header, SectionTy::Element, |_, _| {
todo!("element section not yet supported")
Err(Error::NotYetImplemented(
"element section not yet supported",
))
})?;

while (skip_section(&mut wasm, &mut header)?).is_some() {}

let _: Option<()> = handle_section(&mut wasm, &mut header, SectionTy::DataCount, |_, _| {
todo!("data count section not yet supported")
Err(Error::NotYetImplemented(
"data count section not yet supported",
))
})?;

while (skip_section(&mut wasm, &mut header)?).is_some() {}
Expand All @@ -142,7 +146,7 @@ pub fn validate(wasm: &[u8]) -> Result<ValidationInfo> {
while (skip_section(&mut wasm, &mut header)?).is_some() {}

let _: Option<()> = handle_section(&mut wasm, &mut header, SectionTy::Data, |_, _| {
todo!("data section not yet supported")
Err(Error::NotYetImplemented("data section not yet supported"))
})?;

while (skip_section(&mut wasm, &mut header)?).is_some() {}
Expand Down Expand Up @@ -183,6 +187,8 @@ fn handle_section<T, F: FnOnce(&mut WasmReader, SectionHeader) -> Result<T>>(
) -> Result<Option<T>> {
match &header {
Some(SectionHeader { ty, .. }) if *ty == section_ty => {
// We just checked that the header is of type "Some", so
// it is safe to unwrap here.
let h = header.take().unwrap();
trace!("Handling section {:?}", h.ty);
let ret = handler(wasm, h)?;
Expand Down
Loading