refactor: magic bytes to constant op codes

[nerodesu017]

Pull Request Overview

This pull request changes the way we handle opcodes: from using magic bytes to using constants.

Testing Strategy

This pull request was tested locally.

TODO or Help Wanted

N/A

Formatting

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

Github Issue

N/A

Author

Signed-off-by: Popescu Adrian [email protected]

[wucke13]

Should we maybe remove the read_instruction method? At least in the code, with that we are matching two times the bytes then.

[nerodesu017]

good observation, will discuss with @george-cosma too the perfect way to go about it

[nerodesu017]

Removed it and made it look cleaner. Sadly we still have to use the strip_bytes command quite often (especially on multibyte instructions - please see executor/mod.rs, specifically and look at multibyte instructions).

[wucke13]

Wherever an instruction is followed by arguments, we could take these directly:

[LOCAL_SET, local_idx, ..] => {
  let local = locals.get_mut(local_idx);
  let value = stack.pop_value(local.to_ty());
  trace!("Instruction: local.set [{local:?}] -> []");
  *local = value;
}

[nerodesu017]

I think this could work, we also have to make sure we increment the pc and modify the current of the WasmReader instance accordingly

[wucke13]

Ideally we could get rid of the current in WasmReader IMHO

[nerodesu017]

Moved from current being a &[u8] to having a pc being a usize

[nerodesu017]

Still an architectural design that I need your input on. For each argument we still need to do a call to strip_bytes, do you want me to continue on this path? (read directly from the slice and call strip_bytes) Or do you want me to keep the current implementation?

[florianhartung]

I would just like to add a few arguments against reading the arguments of operations while matching like this: [SOME_OPCODE, some_argument, ..] => { ... }.

1. This only works for arguments of fixed size. In WASM, however, integers are encoded with variable length (see here). Unfortunately, as far as I know, there is no elegant solution for matching them directly in the slice.
2. By matching the arguments directly in the slice, we decouple reading the argument bytes from advancing the program counter, which in my opinion is not very rusty. I think it is more error-prone, and puts the additional responsibility on the developer to make sure that the program counter is always incremented by the correct amount.

Don't get me wrong: The strip_bytes method is far from the best solution.
Maybe the best way is to write a macro with macro_rules! for the whole match statement, which allows us to match variable-length arguments. This macro could then also increment the program counter accordingly.

[wucke13]

@nerodesu017 Are you ok if I take over this branch, trying to finish the work?

[nerodesu017]

@nerodesu017 Are you ok if I take over this branch, trying to finish the work?

I am fine with that!

[wucke13]

I just figured that all the clear changes that we want already landed in 761bd78 . Considering that and the comment in #23 (comment), I would suggest we close this PR. This take on the matching of opcodes (which I unfortunately proposed) has some serious flaws as pointed out in the comment above.