wasmtime / PR #4188 breaking change: remove support for s... · git-wasmtime

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

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

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

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

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.

Stream: git-wasmtime

Topic: wasmtime / PR #4188 breaking change: remove support for s...

Wasmtime GitHub notifications bot (May 25 2022 at 19:59):

TL;DR