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: Valid Wasm is not accepted if bulk-memory is enabled and reference-types is disabled #889

Open
Robbepop opened this issue Jan 24, 2023 · 3 comments

Comments

@Robbepop
Copy link
Collaborator

Robbepop commented Jan 24, 2023

I stumbled upon this when implementing bulk-memory Wasm proposal support in wasmi when integrating the new Wasm spec testsuite test cases for the bulk-memory Wasm proposal. Note: wasmi does not yet have support for the reference-types Wasm proposal and I guess this is important for this issue.

Namely, when parsing and validating the test cases via wasmparser (as always) it errors out with this message among others:

---- spec::bulk_memory::wasm_table_copy stdout ----
thread 'spec::bulk_memory::wasm_table_copy' panicked at 'tests/spec/testsuite/proposals/bulk-memory-operations/table_copy.wast: failed to execute `.wast` directive: reference types support is not enabled (at offset 0x74)', crates\wasmi\tests\spec\run.rs:44:9

---- spec::bulk_memory::wasm_table_init stdout ----
thread 'spec::bulk_memory::wasm_table_init' panicked at 'tests/spec/testsuite/proposals/bulk-memory-operations/table_init.wast: failed to execute `.wast` directive: reference types support is not enabled (at offset 0x74)', crates\wasmi\tests\spec\run.rs:44:9

The wasmparser error message is: reference types support is not enabled (at offset 0x74)
Querying the wasmparser codebase got me the following code segments:


Err("reference types support is not enabled")

Which filters out ValType::FuncRef | ValType::ExternRef types, however, the bulk-memory Wasm proposal explicitly allows ValType::FuncRef in so-called elemexpr which can only occur in Wasm element const experessions. I think this is the problematic code. (Not tested though.)

elemexpr = refnull | funcref <func_idx>

Source: bulk-memory Wasm Proposal Overview
elemexpr


(desc reference_types) => ("reference types");

Unfortunately I am on wasmparser 0.91 so this issue might have already been resolved in a newer version.
Here is a link to the WIP wasmi PR in case this helps. (It is very WIP ...)

@Robbepop Robbepop changed the title wasmparser improperly fails with reference types support is not enabled wasmparser improperly fails with: reference types support is not enabled Jan 24, 2023
@alexcrichton
Copy link
Member

The bulk memory and reference types proposals were heavily intertwined and very little care was given at the time when implementing them in wasmparser as to which precise feature went where, so I'm sure mistakes were made or the spec has drifted since. It's fine to recategorize features under different proposals retroactively.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Jan 24, 2023

It's fine to recategorize features under different proposals retroactively.

What does this mean with respect to this issue?

The elemexpr was introduced in bulk-memory alongside table.init because without them it is impossible to initialize a table to null via table.init from what I understood.

Fixing this issue in wasmparser is a bit tricky since ref.null and ref.func instructions are only accepted in certain context under the bulk-memroy Wasm proposal.

@Robbepop Robbepop changed the title wasmparser improperly fails with: reference types support is not enabled wasmparser incorrectly fails with: reference types support is not enabled Jan 24, 2023
@alexcrichton
Copy link
Member

I'm not entirely sure unfortunately, I haven't put any effort into thinking about how to rationalize the bulk memory proposal without reference types or vice versa.

@Robbepop Robbepop changed the title wasmparser incorrectly fails with: reference types support is not enabled wasmparser: Valid Wasm is not accepted if bulk-memory is enabled and reference-types is disabled Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants