Convert panics to errors

[george-cosma]

Pull Request Overview

This pull request attempts to convert most panics into errors.

Testing Strategy

This pull request was tested by running cargo test

TODO or Help Wanted

There are some .expects() that I'm unsure HOW to turn to errors, or if we should. I'm also a bit afraid that our Error type is becoming "too precise" or "over-specific" (i.e. An error variant is ever created only once). I'd like some input on this.

Formatting

• Ran cargo fmt
• Ran cargo check
• Ran cargo build
• Ran cargo doc
• Ran nix fmt
• Ran treefmt

[wucke13]

In general, really good! I'm struggling whether I like having "not yet implemented" as an error variant.

Further on, we might need its own trap error enum, that explicitly handles all reasons for a trap of the interpreter.

[wucke13]

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

[george-cosma]

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.

[florianhartung]

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.

[wucke13]

My vote is yes!

[wucke13]

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

[wucke13]

One general note on this: I think we should .expect("something") whenever the validation ensures that we are always on the Ok/Some side of things. It would be misleading to introduced ? on places where it is statically known that the None/Err case never occurs. This might ease MC/DC testing later, as it at least clearly discerns the branches that we believe are actually impossible.

[florianhartung]

One general note on this: I think we should .expect("something") whenever the validation ensures that we are always on the Ok/Some side of things. It would be misleading to introduced ? on places where it is statically known that the None/Err case never occurs. This might ease MC/DC testing later, as it at least clearly discerns the branches that we believe are actually impossible.

@wucke13 Just a quick note.. That's what we have this for:

wasm-interpreter/src/execution/assert_validated.rs

Lines 5 to 19 in 6f91fc5

	pub(crate) trait UnwrapValidatedExt<T> {
	fn unwrap_validated(self) -> T;
	}
	impl<T> UnwrapValidatedExt<T> for Option<T> {
	fn unwrap_validated(self) -> T {
	self.expect("this to be `Some` because of prior validation")
	}
	}
	impl<T, E: Debug> UnwrapValidatedExt<T> for Result<T, E> {
	fn unwrap_validated(self) -> T {
	self.expect("this to be `Ok` because of prior validation")
	}
	}

[cemonem]

Is there a reason we are trying to specify why the traps arise? There is no mention of specific trap codes in the spec, all traps are encoded with the trap instruction https://webassembly.github.io/spec/core/exec/runtime.html#administrative-instructions

The trap instruction represents the occurrence of a trap. Traps are bubbled up through nested instruction sequences, ultimately reducing the entire program to a single instruction, signalling abrupt termination.

Maybe we can simply get away without implementing an error for everything that causes a trap and simply have an error for trap. Otherwise there might always be things we miss, or traps that correspond to more than one error.

[cemonem]

or maybe there might be benefit, other wasm interpreters seem to be doing this. Copying the codes off of them and iterating would be the best I guess

[george-cosma]

For me, it's not super clear what a trap ought to be in terms of core wasm. My interpretation of the spec is that a trap is something similar to rust's panic, though I don't know for sure.

Perhaps it would be correct to say that our compiler can exit due to 3 reasons at runtime:

1. Invalid runtime state (tried to run and indirect_call instruction to a NULL function reference) - in this case that is an issue with the code, we return an appropriate error and we stop all execution
2. Invalid wasm code state / "panic" in wasm code. Let's say that someone compiled some wasm code to represent a game of tic tac toe and the turn order variable goes to player 3 (or something else equally invalid). This could be caught by an unwrap, index out of bounds, or just a plain if turn > 2 {panic!("AAAaa")}. In this case, our interpreter would hit the "trap" instruction and we would send out a generic error for "trap".
3. Illegal interpreter state - we have a fatal bug, or we hit a todo!()

or maybe there might be benefit, other wasm interpreters seem to be doing this. Copying the codes off of them and iterating would be the best I guess

@cemonem Could you link some examples, I'm curious now 😄. Also @nerodesu017 with the instant 👍 reaction 😆

[nerodesu017]

While we could get away with simply having a general Error that resembles a Trap (see https://github.com/WebAssembly/wabt/blob/2e23b86f2716877f6e24df0c39ac8f0f0e64c770/src/interp/interp.cc#L1238 for example), it helps with code readability, maintainability and testing (especially this part, I love me the intellisense when asserting for traps)

Besides we have more granular control (for example seeing if an error was at runtime or not, etc)

[cemonem]

@cemonem Could you link some examples, I'm curious now 😄. Also @nerodesu017 with the instant 👍 reaction 😆
The latest memory implementation from @nerodesu017 had a link: https://github.com/wasmi-labs/wasmi/blob/37d1449524a322817c55026eb21eb97dd693b9ce/crates/core/src/trap.rs#L220

[george-cosma]

Yhea, imo I think we can do it how wasmi is doing it