Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Keep Config options for disabling compiled-out features #10455

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions crates/wasmtime/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,8 @@ impl Config {
/// WebAssembly will execute and what compute resources will be allotted to
/// it. If Wasmtime doesn't support exactly what you'd like just yet, please
/// feel free to open an issue!
///
/// This feature is `false` by default.
#[cfg(feature = "async")]
pub fn async_support(&mut self, enable: bool) -> &mut Self {
self.async_support = enable;
Expand Down Expand Up @@ -837,13 +839,11 @@ impl Config {
/// Embeddings of Wasmtime are able to build their own custom threading
/// scheme on top of the core wasm threads proposal, however.
///
/// The default value for this option is whether the `threads`
/// crate feature of Wasmtime is enabled or not. By default this crate
/// feature is enabled.
/// This feature is enabled by default as long as the `threads` crate feature
/// (enabled by default) is also enabled.
///
/// [threads]: https://github.com/webassembly/threads
/// [wasi-threads]: https://github.com/webassembly/wasi-threads
#[cfg(feature = "threads")]
pub fn wasm_threads(&mut self, enable: bool) -> &mut Self {
self.wasm_feature(WasmFeatures::THREADS, enable);
self
Expand All @@ -865,7 +865,6 @@ impl Config {
/// and thus may cause `Engine::new` fail if the `bulk_memory` feature is disabled.
///
/// [proposal]: https://github.com/webassembly/reference-types
#[cfg(feature = "gc")]
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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 toggle GC_TYPES?

Copy link
Contributor Author

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?

Copy link
Member

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.

pub fn wasm_reference_types(&mut self, enable: bool) -> &mut Self {
self.wasm_feature(WasmFeatures::REFERENCE_TYPES, enable);
self
Expand All @@ -884,7 +883,6 @@ impl Config {
/// This feature is `false` by default.
///
/// [proposal]: https://github.com/WebAssembly/function-references
#[cfg(feature = "gc")]
pub fn wasm_function_references(&mut self, enable: bool) -> &mut Self {
self.wasm_feature(WasmFeatures::FUNCTION_REFERENCES, enable);
self
Expand Down Expand Up @@ -916,7 +914,6 @@ impl Config {
/// progress and generally not ready for primetime.**
///
/// [proposal]: https://github.com/WebAssembly/gc
#[cfg(feature = "gc")]
pub fn wasm_gc(&mut self, enable: bool) -> &mut Self {
self.wasm_feature(WasmFeatures::GC, enable);
self
Expand Down Expand Up @@ -1100,12 +1097,10 @@ impl Config {
/// [`Component`](crate::component::Component) instead of
/// [`Module`](crate::Module) for example anyway.
///
/// The default value for this option is whether the `component-model`
/// crate feature of Wasmtime is enabled or not. By default this crate
/// feature is enabled.
/// This feature is enabled by default as long as the `component-model` crate
/// feature (enabled by default) is also enabled.
///
/// [proposal]: https://github.com/webassembly/component-model
#[cfg(feature = "component-model")]
pub fn wasm_component_model(&mut self, enable: bool) -> &mut Self {
self.wasm_feature(WasmFeatures::COMPONENT_MODEL, enable);
self
Expand All @@ -1118,7 +1113,6 @@ impl Config {
/// Please note that Wasmtime's support for this feature is _very_ incomplete.
///
/// [proposal]: https://github.com/WebAssembly/component-model/blob/main/design/mvp/Async.md
#[cfg(feature = "component-model-async")]
pub fn wasm_component_model_async(&mut self, enable: bool) -> &mut Self {
self.wasm_feature(WasmFeatures::COMPONENT_MODEL_ASYNC, enable);
self
Expand Down Expand Up @@ -1837,8 +1831,8 @@ impl Config {
/// Disabling this will result in a single thread being used to compile
/// the wasm bytecode.
///
/// 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;
Copy link
Member

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?

self
Expand Down Expand Up @@ -2165,6 +2159,26 @@ impl Config {
bail!("wmemcheck (memory checker) was requested but is not enabled in this build");
}

#[cfg(not(feature = "component-model"))]
if features.component_model() {
bail!("component-model support was requested but is not enabled in this build (requires the `component-model` crate feature)");
}

#[cfg(not(feature = "component-model-async"))]
if features.component_model_async() {
bail!("component-model-async support was requested but is not enabled in this build (requires the `component-model-async` crate feature)");
}

#[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)");
}
Comment on lines +2172 to +2175
Copy link
Member

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.


#[cfg(not(feature = "threads"))]
if features.threads() {
bail!("threads support was requested but is not enabled in this build (requires the `threads` crate feature)");
}

let mut tunables = Tunables::default_for_target(&self.compiler_target())?;

// If no target is explicitly specified then further refine `tunables`
Expand Down