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

Config::wasm_bulk_memory(false) cannot be usefully specified without enabling the gc feature. #9622

Closed
nagisa opened this issue Nov 19, 2024 · 2 comments · Fixed by #9623
Closed
Labels
bug Incorrect behavior in the current implementation that needs fixing

Comments

@nagisa
Copy link
Contributor

nagisa commented Nov 19, 2024

Test Case

N/A

Steps to Reproduce

  1. Depend on wasmtime with the runtime feature, but without the gc feature;
  2. Set Config::wasm_bulk_memory(false) (cannot set Config::wasm_reference_types);
  3. Invoke the runtime.

Expected Results

The wasm_reference_types feature should be considered disabled (and/or it should be possible to disable the feature without enabling the entire gc feature.)

Actual Results

wasmtime returns Err(feature 'reference_types' requires 'bulk_memory' to be enabled)

Versions and Environment

Wasmtime version or commit: 26.0.1

Operating system: Linux

Architecture: x86_64

Extra Info

wasm_reference_types config was not hidden behind a feature in the past. I'm attempting to update wasmtime from 14.0.1.

REFERENCE_TYPES gets set by the default set of supported features via WasmFeatures::WASM2.

My suggestion here is that Config parameters perhaps should not be gated by crate features.

@nagisa nagisa added the bug Incorrect behavior in the current implementation that needs fixing label Nov 19, 2024
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Nov 19, 2024
Some of this originated in bytecodealliance#917 but nowadays it shouldn't be necessary
to control proposals like this. Instead it's probably best nowadays to
throw configuration at `wasmparser` and use its definition of features
to determine whether constructs make sense or not. This reduces the
amount of bits and pieces Wasmtime has to do and avoids interactions
such as bytecodealliance#9622.

Closes bytecodealliance#9622
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Nov 19, 2024
Some of this originated in bytecodealliance#917 but nowadays it shouldn't be necessary
to control proposals like this. Instead it's probably best nowadays to
throw configuration at `wasmparser` and use its definition of features
to determine whether constructs make sense or not. This reduces the
amount of bits and pieces Wasmtime has to do and avoids interactions
such as bytecodealliance#9622.

Closes bytecodealliance#9622
@alexcrichton
Copy link
Member

Thanks for the report! I've updated this in #9623 to remove this now-unnecessary validation.

It's also perhaps worth noting that this is something where you may want to run wasmparser::validate ahead of time with a custom feature set if you're looking to turn off some features. That way you can be sure to disable reference-types even when the gc feature is disabled (as otherwise it'll always be enabled, it's just gc types that are disabled)

@nagisa
Copy link
Contributor Author

nagisa commented Nov 19, 2024

Cool, yeah that would work for me as I'm already validating with wasmparser first.

github-merge-queue bot pushed a commit that referenced this issue Nov 19, 2024
Some of this originated in #917 but nowadays it shouldn't be necessary
to control proposals like this. Instead it's probably best nowadays to
throw configuration at `wasmparser` and use its definition of features
to determine whether constructs make sense or not. This reduces the
amount of bits and pieces Wasmtime has to do and avoids interactions
such as #9622.

Closes #9622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants