[lightbeam] Refactoring Error handling + remove panicking calls in lightbeam backend

[pventuzelo]

Hi guys,

I triggered some new bugs during fuzzing of lightbeam. Those bugs are mainly related to usage of unreachable! and expect.
I remove a lot of the panic!, assert!, expect, etc. from backend.rs and function_body.rs. Removing all those calls make me refactoring completely the Error handling of backend.rs as well as some modification in function_body.rs i.e. that's why it's a huge PR.

Related bugs:

• [lightbeam] panic due to assert! in function_body.rs #673
• [lightbeam] panic due to unreachable! in backend.rs  #674

This code has been successfully tested with: cargo test --release --features lightbeam

[pventuzelo]

Any ETA for review/merging?
Do you need something from my side?

[Robbepop]

Any ETA for review/merging?
Do you need something from my side?

I guess it is hindersome to review this PR as long as CI fails for it, especially the Lightbeam CI is failing, indicating that something is off.

[pventuzelo]

Any ETA for review/merging?
Do you need something from my side?

I guess it is hindersome to review this PR as long as CI fails for it, especially the Lightbeam CI is failing, indicating that something is off.

CI seems to failed when lightbeam feature is excluded. so it's maybe more related to wasi in that case.

For the tests on macos, there is not output, it's strange.
Do you know if there is any issue with CI?

[Robbepop]

Any ETA for review/merging?
Do you need something from my side?

I guess it is hindersome to review this PR as long as CI fails for it, especially the Lightbeam CI is failing, indicating that something is off.

CI seems to failed when lightbeam feature is excluded. so it's maybe more related to wasi in that case.

For the tests on macos, there is not output, it's strange.
Do you know if there is any issue with CI?

I don't know of any CI bugs but also I am not a wasmtime maintainer - just filed my own first PR here but it passes CI.
Maybe a wasmtime team member could politely restart the CI for this branch to see if it was just an error.

[pventuzelo]

Any ETA for review/merging?
Do you need something from my side?

I guess it is hindersome to review this PR as long as CI fails for it, especially the Lightbeam CI is failing, indicating that something is off.

CI seems to failed when lightbeam feature is excluded. so it's maybe more related to wasi in that case.
For the tests on macos, there is not output, it's strange.
Do you know if there is any issue with CI?

I don't know of any CI bugs but also I am not a wasmtime maintainer - just filed my own first PR here but it passes CI.
Maybe a wasmtime team member could politely restart the CI for this branch to see if it was just an error.

Thanks for your help ;)

I've update wasmparser to last version and correct the error due to refactoring of wasmparser in this commit bytecodealliance/wasmparser@8643144

Maybe that's why wasi tests don't pass.

[pventuzelo]

Nice so everything good ;)

[Robbepop]

CI seems to be happy now.
I will review this PR later today.
Can you resolve the conflicts with master branch before?
My PR just got merged that also already updated to wasmparser 0.44 - I guess that's why there are conflicts now.

[pventuzelo]

CI seems to be happy now.
I will review this PR later today.
Can you resolve the conflicts with master branch before?
My PR just got merged that also already updated to wasmparser 0.44 - I guess that's why there are conflicts now.

Yes i'm working on the conflicts right now.
I let you know when it's done ;)

[pventuzelo]

@Robbepop Everything seems good ;)

[Robbepop]

Not a real code review yet but just ran cargo clippy on your PR and identified some new warnings that haven't been there before, namely:

warning: redundant clone
   --> crates/lightbeam/src/backend.rs:351:62
    |
351 |                     format!("Double-freed register: {}", gpr).to_string(),
    |                                                              ^^^^^^^^^^^^ help: remove this
    |
    = note: `#[warn(clippy::redundant_clone)]` on by default
note: this value is dropped without further use
   --> crates/lightbeam/src/backend.rs:351:21
    |
351 |                     format!("Double-freed register: {}", gpr).to_string(),
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone

warning: redundant clone
   --> crates/lightbeam/src/function_body.rs:468:78
    |
468 | ...                   format!("br_if unimplemented case: {:#?}", other).to_string(),
    |                                                                        ^^^^^^^^^^^^ help: remove this
    |
note: this value is dropped without further use
   --> crates/lightbeam/src/function_body.rs:468:29
    |
468 | ...                   format!("br_if unimplemented case: {:#?}", other).to_string(),
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_clone

warning: passing a unit value to a function
   --> crates/lightbeam/src/function_body.rs:399:86
    |
399 |                           | (ref mut other @ (None, _), (Some(Left(ref cc)), _)) => Ok({
    |  ______________________________________________________________________________________^
400 | |                             let mut cc = ctx.serialize_block_args(cc, max_params)?;
401 | |                             if let Some(to_drop) = other.1 {
402 | |                                 drop_elements(&mut cc.arguments, to_drop.clone());
403 | |                             }
404 | |                             *other.0 = Some(Left(cc));
405 | |                         }),
    | |_________________________^
    |
    = note: `#[warn(clippy::unit_arg)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
help: if you intended to pass a unit value, use a unit literal instead
    |
399 |                         | (ref mut other @ (None, _), (Some(Left(ref cc)), _)) => Ok(()),
    |                                                                                      ^^

[pventuzelo]

@alexcrichton @sunfishcode seems that the CI for .NET bindings can't start properly.
It failed on Run /./.github/actions/install-rust.

Can you restart the CI?

[sunfishcode]

Ok, I've now clicked the button to re-run the tests.

[pventuzelo]

@sunfishcode Thx, everything seems good now.

[eira-fransham]

Could you switch to using proper error types please? Here's a good overview of crates in the ecosystem to define error types https://users.rust-lang.org/t/the-state-of-error-handling-in-the-2018-edition/23263

Apart from that this seems good and ready to merge.

EDIT: I thought that I'd mentioned this in the review but my comments were still pending. Sorry! Basically, I mean that the error type in a lot of this code is essentially String or an enum containing such. All the cases of String in errors should be replaced with real error types where possible.

[pventuzelo]

We discuss about error types with @Vurich, and it seems better to merge the current pull request and then create a new one only for error types uniformisation.