-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Keep Config options for disabling compiled-out features #10455
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
/// | ||
/// # Errors | ||
/// | ||
/// The validation of this feature are deferred until the engine is being built, | ||
/// 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_types
but enable the GC feature? - Should
wasm_gc
toggleGC_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.
/// By default parallel compilation is enabled. | ||
#[cfg(feature = "parallel-compilation")] | ||
/// 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?
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):Config::wasm_reference_types
.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 awasmtime::Engine
:Config::wasm_threads
.Config::wasm_gc
.Config::wasm_component_model
.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 #10454