wasmtime / issue #4188 breaking change: remove support fo... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #4188 breaking change: remove support fo...

Wasmtime GitHub notifications bot (May 25 2022 at 20:05):

r? @alexcrichton

Wasmtime GitHub notifications bot (May 25 2022 at 20:10):

@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.

Wasmtime GitHub notifications bot (May 25 2022 at 20:20):

github-actions[bot] commented on issue #4188:

Subscribe to Label Action

cc @peterhuene

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (May 25 2022 at 20:20):

github-actions[bot] commented on issue #4188:

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.

<details>

Our documentation should be of the following form:

```text
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.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

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][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][fuzzing-docs] for more details.

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

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

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

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

Learn more.

</details>

Wasmtime GitHub notifications bot (May 25 2022 at 20:29):

PureWhiteWu commented on issue #4188:

@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.

Wasmtime GitHub notifications bot (May 25 2022 at 20:51):

bjorn3 commented on issue #4188:

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

Wasmtime GitHub notifications bot (May 25 2022 at 21:14):

alexcrichton commented on issue #4188:

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.

Wasmtime GitHub notifications bot (May 26 2022 at 07:05):

PureWhiteWu commented on issue #4188:

@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.

Last updated: Nov 22 2024 at 16:03 UTC