Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Robbepop opened this issue Dec 28, 2020 · 0 comments
Open

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

Robbepop opened this issue Dec 28, 2020 · 0 comments
Labels
wasmparser Related to the binary format of WebAssembly (wasmparser)

Comments

@Robbepop
Copy link
Contributor

Robbepop commented Dec 28, 2020

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.

  2. 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
  3. 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.

@alexcrichton alexcrichton added the wasmparser Related to the binary format of WebAssembly (wasmparser) label May 16, 2022
alexcrichton pushed a commit to alexcrichton/wasm-tools that referenced this issue Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmparser Related to the binary format of WebAssembly (wasmparser)
Projects
None yet
Development

No branches or pull requests

2 participants