Stream: git-wasmtime

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


view this post on Zulip Wasmtime GitHub notifications bot (May 25 2022 at 20:05):

PureWhiteWu commented on issue #4188:

r? @alexcrichton

view this post on Zulip Wasmtime GitHub notifications bot (May 25 2022 at 20:10):

cfallin commented on issue #4188:

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

view this post on Zulip 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:

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

Learn more.
</details>

view this post on Zulip 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:

[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>

view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip 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:

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.

view this post on Zulip 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: Dec 23 2024 at 12:05 UTC