bytecodealliance / bytecodealliance/wasm-tools
[wasmparser] Split Operator::BrTable into multiple sub-operators
- Dominant language
- Rust
- Stars
- 1.8k
- Forks
- 351
- Avg merge
- 16h 57m
- Merged PRs (30d)
- 38
Description
Currently the `Operator` enum has a lifetime attached which makes it a bit harder to work with in some contexts.
The only reason for this is its `BrTable { table: BrTable<'a> }` variant with which it is possible as user of this crate to lazily (but necessarily) parse the targets of the branching table.
## Proposed Design
The solution I wanted to propose here is to split the single `Operator::BrTable` variant into 3 variants:
1. `Operator::BrTableStart { count_targets: u32 }`
2. `Operator::BrTableTarget { depth: u32 }`
3. `Operator::BrTableDefault { depth: u32 }`
These variants will appear in exactly this order for every `BrTable` in the Wasm input if parsed by the `wasmparser` crate.
The `count_targets: u32` field tells the user how many `Operator::BrTableTarget` variants are to be expected directly afterwards, followed by a single `Operator::BrTableDefault` variant.
## Alternative Design
An alternative design would be to treat the `Operator::BrTableDefault` as just another `Operator::BrTableTarget` and therefore the `count_targets: u32` field in `BrTableStart` simply reflects the number of all targets as well as the additional default target.
## What does this solve?
1. This proposal would eliminate the lifetime in the very big `Operator` enum. However, it would also be a breaking change and would probably result in potentially significant changes in how users handle `BrTable` inputs.
1. Additional benefits are that the `Operator` enum could then more or less easily derive a lot more useful traits such as:
- `Copy`
- `PartialEq` and `Eq`
- `PartialOrd` and `Ord`
- `Hash`
1. Also this proposal solves the current double parsing of `BrTable` as convenience type.
The current implementation first parses the `br_table` just for validating the structure but throws all the information away so that the user can use this convenience type to re-parse and store the information. With this proposal the double parsing would be gone.
## Streamlined Design
In my opinion this design would streamline the design of the `Operator` enum since it already splits some entities into multiple operators such as `If`, `Else` and `End` or `Block` and `End` or `Loop` and `End` etc. The split of `BrTable` would be just another.
## Existing Works
It seems that the widely used `wasmi` interpreter is already using a similar technique to handle branching tables as can be seen here: (Please note that I am not familiar with its codebase.)
https://github.com/paritytech/wasmi/blob/master/src/isa.rs#L359
I am curious if this idea has already been discussed and what others think about it.
Contributor guide
Assessment
This issue has not been assessed yet.