bytecodealliance / bytecodealliance/wasm-tools

[wasmparser] Split Operator::BrTable into multiple sub-operators

Open
#182 0 comments 0 reactions 0 assignees View on GitHub
wasmparser
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.