breaking change: remove support for strategy

[PureWhiteWu]

TL;DR

This commit removes support for users to set the strategy field.
I'd propose to do this because:

1. This field is easy to misuse / cause problems and hard to debug.
2. This field isn't useful these days (nor likely in the near future).

Misuse

I updated from wasmtime 0.35 to 0.37 these days, and it breaks with the
following error:

Error: compilation settings are not compatible with the native host

Caused by:
    setting "enable_safepoints" is configured to Bool(false) which is not supported

And here's what my config code looks like:

pub fn default_config() -> Config {
    let mut cfg = Config::new();

    // Running related configs
    cfg.module_version(ModuleVersionStrategy::WasmtimeVersion)
        .unwrap();
    cfg.profiler(ProfilingStrategy::None).unwrap();
    cfg.allocation_strategy(InstanceAllocationStrategy::OnDemand);
    cfg.wasm_backtrace_details(WasmBacktraceDetails::Enable);
    cfg.memory_init_cow(true);
    cfg.async_support(true);
    cfg.consume_fuel(true);
    cfg.epoch_interruption(true);

    // Wasm related configs
    cfg.wasm_threads(false);
    cfg.wasm_reference_types(true);
    cfg.wasm_simd(true);
    cfg.wasm_bulk_memory(true);
    cfg.wasm_multi_value(true);
    cfg.wasm_multi_memory(false);
    cfg.wasm_memory64(false);

    // Compiler settings
    cfg.strategy(Strategy::Cranelift).unwrap();
    cfg.parallel_compilation(true);
    cfg.generate_address_map(true);
    cfg.cranelift_opt_level(OptLevel::Speed);
    // disable avx512
    #[cfg(target_arch = "x86_64")]
    unsafe {
        for (k, v) in vec![
            ("has_avx", "true"),
            ("has_avx2", "true"),
            ("has_avx512bitalg", "false"),
            ("has_avx512dq", "false"),
            ("has_avx512f", "false"),
            ("has_avx512vbmi", "false"),
            ("has_avx512vl", "false"),
            ("has_bmi1", "true"),
            ("has_bmi2", "true"),
            ("has_lzcnt", "true"),
            ("has_popcnt", "true"),
            ("has_sse3", "true"),
            ("has_sse41", "true"),
            ("has_sse42", "true"),
            ("has_ssse3", "true"),
        ] {
            cfg.cranelift_flag_set(k, v).unwrap();
        }
    };
    cfg
}

It works fine in 0.35 but breaks in 0.37, and I found that it's because runtime checks
have been added in #3899. But it is weird that I've already set cfg.wasm_reference_types(true)
which should have set enable_safepoints to true:

pub fn wasm_reference_types(&mut self, enable: bool) -> &mut Self {
    ...
        self.compiler
            .set("enable_safepoints", if enable { "true" } else { "false" })
            .unwrap();
    }
    ...
}

And finally I found that it's because config.strategy creates a new compiler builder instead,
which invalidates all the compiler flags.

To use this field correctly, users should set the strategy first, and this is not documented.

This in fact caused a lot of configs to not work in the previous release(<0.36) without any errors
and really hard to discover. If I did set references_type to false, then I would not discover this
error together with other not working configs such as simd.

So I think this is easy to misuse / causes problems and hard to discover / debug.

Not Useful

Per RFC 14 there seems only one Strategy
left(Cranelift), and I searched through issues and infomation, there's no plan to add new new backends
in the (near) futures.

This config field itself also did nothing useful other than creates the only Cranelift backend compiler builder,
which is already created as the default value of Config.

Alternatives

Noop

The first is to make strategy config field to a no-op, such as:

pub fn strategy(&mut self, strategy: Strategy) -> Result<&mut Self> {
    Ok(self)
}

But I think this field is useless now, and more important is that it is also likely to be useless in the future,
so I prefer to remove it directly.

Panic

The second is to panic if the compiler field has been set.

But I don't find a clean way to check if the compiler has been changed (maybe need to iter / add another
field to mark), and same as the above reason, I'd prefer to remove it.

Document

The third is to document that this field must be called at the start of Config.

Besides the above reasons, there's also two cons of this way:

1. Users are likely to miss this detail in the document.
2. Old misused users cannot discover this error.

Conclusion

So I think directly remove this config field is the best way to avoid this problem.

And besides the problems it causes, it's also reasonable to remove this since this
is really useless these days and in the foreseeable future.

[PureWhiteWu]

r? @alexcrichton

[cfallin]

@PureWhiteWu thank you for this report!

I agree that the current behavior is somewhat surprising and error-prone. The root issue seems to be that we have settings that silently update other settings as well. IMHO we should strive for the property that the order of calls to methods on Config doesn't matter.

@alexcrichton Any thoughts on that invariant? I suspect there may be a way to achieve requirements like "turning on reftypes enables safepoints under the hood" in another way, by having a "fixup phase" when the Config is actually used. So basically, (i) setter methods each set an independent field, and no two methods touch overlapping state; then (ii) we "legalize" or "fix up" the config by, e.g., turning on safepoints unconditionally when reftypes are enabled, once the user hands us the config.

I feel that a focus on the strategy in particular is somewhat tangential. It's one place where the above proposed invariant is violated, but there could be others (we should audit this). And FWIW, there are at least early talks to build some other "strategies" (a baseline compiler with linear time guarantees, at least) so at some point we would add it back if we removed it today. But, I'm not totally firm on that and could be convinced to have it removed for now if that's the direction we go.

[github-actions]

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api", "wasmtime:config", "wasmtime:docs"

Thus the following users have been cc'd because of the following labels:

• peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[github-actions]

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.

[PureWhiteWu]

@cfallin Thanks for your quick reply!
The problem is not only about reference_type and enable_safepoints, there's also many other flags affected.

Here's the log I got.

The problematic version:

[shared]
opt_level = "speed"
tls_model = "none"
libcall_call_conv = "isa_default"
baldrdash_prologue_words = 0
probestack_size_log2 = 12
regalloc_checker = false
enable_verifier = true
is_pic = false
use_colocated_libcalls = false
avoid_div_traps = true
enable_float = true
enable_nan_canonicalization = false
enable_pinned_reg = false
use_pinned_reg_as_heap_base = false
enable_simd = false
enable_atomics = true
enable_safepoints = false
enable_llvm_abi_extensions = false
unwind_info = true
machine_code_cfg_info = false
emit_all_ones_funcaddrs = false
enable_probestack = false
probestack_func_adjusts_sp = false
enable_jump_tables = true
enable_heap_access_spectre_mitigation = true
enable_table_access_spectre_mitigation = true

The correct version:

[shared]
opt_level = "speed"
tls_model = "none"
libcall_call_conv = "isa_default"
baldrdash_prologue_words = 0
probestack_size_log2 = 12
regalloc_checker = false
enable_verifier = false
is_pic = false
use_colocated_libcalls = false
avoid_div_traps = true
enable_float = true
enable_nan_canonicalization = false
enable_pinned_reg = false
use_pinned_reg_as_heap_base = false
enable_simd = true
enable_atomics = true
enable_safepoints = true
enable_llvm_abi_extensions = false
unwind_info = true
machine_code_cfg_info = false
emit_all_ones_funcaddrs = false
enable_probestack = false
probestack_func_adjusts_sp = false
enable_jump_tables = true
enable_heap_access_spectre_mitigation = true
enable_table_access_spectre_mitigation = true

And I think there's also other flags and isa_flags that may be affected by this, so it's not only about reference_type.

by having a "fixup phase" when the Config is actually used

In fact, I didn't find a easy way to achieve this, because flags and isa_flags are strongly associated with the compiler backend, so it's not easy to move it into Config.

And FWIW, there are at least early talks to build some other "strategies" (a baseline compiler with linear time guarantees, at least) so at some point we would add it back if we removed it today.

Firstly, maybe we could make this another config field instead of strategy, such as a flag field of Cranelift(if it's also based on Cranelift with some optimizations disabled)?
Secondly, I wonder for how long this will become a reality, and if this will take half a year or even a year to implement, I think leave the problem to that time is a better choice.

[bjorn3]

Maybe have Config::with_strategy and remove the strategy method?

[alexcrichton]

To echo Chris thanks for bringing this up @PureWhiteWu! This has actually bit me many times in the past in one way or another so it's definitely something I'd like to see fixed. Additionally sorry for the breakage, it wasn't intended to be so impactful to validate runtime settings but in a sense I also think it's good to turn up bugs like this since this was something that needed fixing anyway.

I would agree with everything Chris said as well. The intention of Config is that the order of method calls don't matter. Unfortunately as you can see Config has not stood the test of time well and it doesn't actually achieve this today. To highlight another location that breaks these invariants that's independent of strategy, the Config::target method will completely overwrite the ISA flags which will remove some previously configured options, notably the cranelift settings.

Overall I think that we should probably change all the methods of Config to be idempotent. To expand a bit on what Chris mentioned:

• When setting a field, no other fields should ever be modified (e.g. enabling reference types shouldn't enable bulk memory in the method)
• The CompilerBuilder trait should be entirely removed from Config
• Remove all other "state" in Config that represents some sort of owned resource like CacheConfig, ProfilingAgent, etc. I don't think state like RuntimeMemoryCreator can be removed but that's ok.
• Defer all validation an inter-option relations to Engine::new. Here we would do the sanitization Chris mentioned where for example if reference types are enabled we'd enable unwind information and bulk memory. This would also be where we'd do things like load cache config, configure cranelift, etc.

I think that would make it so the strategy option would still make sense (and we could add to it in the future with a baseline compiler). Additionaly it would fix preexisting issues around target and other inter-related fields. @PureWhiteWu would you be up for a refactoring like this? I'd be happy to help with any issues that arose.

[PureWhiteWu]

@alexcrichton Thanks for your instructions! I'm happy to have a try!

I'll close this pr, since this seems isn't the right way to fix the problems.

I've opened an issue for this #4189, and will submit a new pr asap.