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

Keep Config options for disabling compiled-out features #10455

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

Conversation

Stebalien
Copy link
Contributor

@Stebalien Stebalien commented Mar 21, 2025

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 #10454

@Stebalien Stebalien requested a review from a team as a code owner March 21, 2025 20:13
@Stebalien Stebalien requested review from alexcrichton and removed request for a team March 21, 2025 20:13
@Stebalien

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")]
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.

@alexcrichton
Copy link
Member

I think it might make sense to un-gate wasm_* configuration while you're here, but for the others I think it's ok to leave them as-is and deal with them as they come up later. Although if you'd like to handle them here that's also ok.

@Stebalien Stebalien force-pushed the steb/wasm-config branch 2 times, most recently from 88bab50 to ef13941 Compare March 21, 2025 22:48
@Stebalien
Copy link
Contributor Author

I've updated the PR per the new description to:

  1. Always define the Config::wasm_* functions.
  2. Allow setting Config::wasm_reference_types and Config::wasm_function_references (I'm not sure if this second one is correct, pending discussion in Keep Config options for disabling compiled-out features #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::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
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Mar 21, 2025
Copy link

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

  • If you added a new Config method, you wrote extensive documentation for
    it.

    Our documentation should be of the following form:

    Short, simple summary sentence.
    
    More details. These details can be multiple paragraphs. There should be
    information about not just the method, but its parameters and results as
    well.
    
    Is this method fallible? If so, when can it return an error?
    
    Can this method panic? If so, when does it panic?
    
    # Example
    
    Optional example here.
    
  • If you added a new Config method, or modified an existing one, you
    ensured that this configuration is exercised by the fuzz targets.

    For example, if you expose a new strategy for allocating the next instance
    slot inside the pooling allocator, you should ensure that at least one of our
    fuzz targets exercises that new strategy.

    Often, all that is required of you is to ensure that there is a knob for this
    configuration option in wasmtime_fuzzing::Config (or one
    of its nested structs).

    Rarely, this may require authoring a new fuzz target to specifically test this
    configuration. See our docs on fuzzing for more details.

  • If you are enabling a configuration option by default, make sure that it
    has been fuzzed for at least two weeks before turning it on by default.


To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

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

/// 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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a mechanism for disabling on-by-default config options regardless of the enabled cargo feature flags
2 participants