-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Keep Config options for disabling compiled-out features #10455
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| /// and thus may cause `Engine::new` fail if the `bulk_memory` feature is disabled. | ||
| /// | ||
| /// [proposal]: https://github.com/webassembly/reference-types | ||
| #[cfg(feature = "gc")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by the test failures, this feature flag shouldn't be here in the first place and this feature is enabled by-default regardless, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is a bit subtle, and this also changed historically with Wasmtime. In essence though REFERENCE_TYPES should always be enabled regardless of what the crate features are, it's the GC_TYPES feature which is actually gated based on feature = "gc". There's no wasm_gc_types(...) configuration method, though.
I think this can be fixed by updating the validator to allow reference-types being enabled with feature = "gc" being turned off. It's feature = "gc" being turned off plus GC_TYPES being turned on that should be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What happens if I turn off
wasm_reference_typesbut enable the GC feature? - Should
wasm_gctoggleGC_TYPES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does this apply to function references? That is, can I enable function references without the GC feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If reference-types is turned off but GC is enabled, then that means GC-using modules will fail to load/compile because the feature is turned off, and the active feature at compile time is basically bloat/inert code and doesn't get used.
For now the hope is we can internalize GC_TYPES to exclusively the feature = "gc" toggle, so wasm_gc should not turn on GC_TYPES. If wasm_gc is turned on though and GC_TYPES is off then you can still load modules, but only those that don't use types that require a GC.
For function-references we don't require a GC for typed function references so you should be able to use such a module without the GC feature.
The end goal is to have wasm_gc on-by-default in the future, and in such a world we still want GC-disabled builds of Wasmtime to still load modules, so that's what GC_TYPES is doing, basically disallowing loading modules at runtime which use features disabled at compile time.
|
I think it might make sense to un-gate |
88bab50 to
ef13941
Compare
|
I've updated the PR per the new description to:
|
This patch keeps config options for disabling default-enabled wasmtime features, even when those features are disabled via crate feature flags. That way, these wasm features can be disabled by libraries even if the crate-level features are later enabled by some other crate in the build-tree. Specifically, the following `wasmtime::Config` methods are now enabled regardless of compile-time features (removed dependency on the GC feature): 1. `Config::wasm_reference_types`. 2. `Config::wasm_function_references`. The following methods are available on the `wasmtime::Config` type, regardless of the enabled crate features but will lead to runtime errors when constructing a `wasmtime::Engine`: 1. `Config::wasm_threads`. 2. `Config::wasm_gc`. 3. `Config::wasm_component_model`. 4. `Config::wasm_component_model_async`. Finally, `Config::parallel_compilation` is defined regardless of whether or not the parallel compilation feature is enabled. However, without the corresponding crate feature, this config option is a no-op. fixes bytecodealliance#10454
ef13941 to
655afbd
Compare
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
| #[cfg(not(feature = "gc"))] | ||
| if features.gc_types() || features.gc() { | ||
| bail!("gc support was requested but is not enabled in this build (requires the `gc` crate feature)"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this we should be able to remove this check entirely. I commented a bit above but on this specifically it's where gc is safe to enable and it's just GC-using-things that aren't safe to enable. This is mostly because the GC proposal was larger than just GC references, it also introduced many other highly-related but still somewhat orthogonal things. The goal is to use GC_TYPES to separate out the runtime requirement but otherwise all the other stuff in the GC proposal is still valid.
| /// This feature is enabled by default as long as the `parallel-compilation` | ||
| /// crate feature (enabled by default) is also enabled. | ||
| pub fn parallel_compilation(&mut self, parallel: bool) -> &mut Self { | ||
| self.parallel_compilation = parallel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a check below to return an error if this is explicitly enabled but disabled at compile time?
|
Status update: I'm no longer working on the project that needed this PR so I'm going to close it for now. @alexcrichton thanks for taking the time to review this. In case anyone else is interested in implementing this, the main remaining part is to resolve #10455 (comment). |
This patch keeps config options for disabling default-enabled wasmtime features, even when those features are disabled via crate feature flags. That way, these wasm features can be disabled by libraries even if the crate-level features are later enabled by some other crate in the build-tree.
Specifically, the following
wasmtime::Configmethods are now enabled regardless of compile-time features (removed dependency on the GC feature):Config::wasm_reference_types.Config::wasm_function_references.The following methods are available on the
wasmtime::Configtype, regardless of the enabled crate features but will lead to runtime errors when constructing awasmtime::Engine:Config::wasm_threads.Config::wasm_gc.Config::wasm_component_model.Config::wasm_component_model_async.Finally,
Config::parallel_compilationis defined regardless of whether or not the parallel compilation feature is enabled. However, without the corresponding crate feature, this config option is a no-op.fixes #10454