wasmtime / PR #9230 Make host trap handlers optional · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9230 Make host trap handlers optional

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:08):

alexcrichton requested wasmtime-fuzz-reviewers for a review on PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:08):

alexcrichton opened PR #9230 from alexcrichton:no-signals to bytecodealliance:main:

This commit introduces a new configuration option
Config::host_trap_handlers which enables disabling the reliance on host trap handlers (e.g. signal handlers on Unix) at runtime. This is intended to help increase portability in cases where signal handlers are otherwise difficult. This is achieved by plumbing the translation environment to more locations which now conditionally lowers to calls to a function to raise a trap instead of a trap instruction.

The caveats of this support are:

This is not yet implemented for Winch

This requires disabling spectre mitigations

This does not yet support shared memories since it forces dynamic memories to be used.

These points are all possible to address but it might be best to see how this feature evolves over time. I'll also note that this is not an optimized implementation and likely has a fair bit of overhead. For example the solution in #6926 of a more optimized stub to jump-and-trap is not implemented yet.

Closes #6926

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:08):

alexcrichton requested elliottt for a review on PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:08):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:08):

alexcrichton requested wasmtime-core-reviewers for a review on PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:09):

alexcrichton requested fitzgen for a review on PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 04:12):

alexcrichton updated PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 06:46):

github-actions[bot] commented on PR #9230:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "fuzzing", "wasmtime:api", "wasmtime:config"

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

fitzgen: fuzzing

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 (Sep 12 2024 at 07:44):

github-actions[bot] commented on PR #9230:

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 (Sep 12 2024 at 15:16):

alexcrichton updated PR #9230.

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen submitted PR review:

Very nice, thanks!

Did we also want to run the full spec test suite under this configuration, or do you think the differential fuzzing and spec test fuzzer are good enough?

r=me with a bunch of nitpicks and such below

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Can we debug assert or something that env.can_use_virtual_memory_traps() when spectre mitigations are enabled?

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Great!

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

    /// is here with an explicit check instead. Note that the explicit check is
    /// always present even if this is a "leaf" function, as we have to call into
    /// the host to trap when signal handlers are disabled.

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Maybe a little more context here? And/or a follow up issue to reference?

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

I could interpret "host trap handlers" in two different and opposite ways:

The host is allowed to rely on the virtual memory subsystem for traps, and should install signal handlers as it sees fit.

Trapping should be implemented and handled "in the host" rather than in Wasm code and/or via the OS (i.e. emitting ud2, simply accessing memory and relying on a virtual memory fault if it is OoB, etc...)

What do you think about renaming this, and the actual wasmtime::Config knob to virtual_memory_traps? I personally think that is much clearer, probably will be for external users too, I'd guess.

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

This is to assert that we never return from the trap libcall, right? Care to add a quick comment noting that?

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Double checking, as I am not an ieee754 guru: always promoting f32 to f64 here cannot affect rounding later on when converting to a 32-bit integer, right? Thinking specifically about very large (positive or negative) numbers where ieee754 has is non-contiguous and has holes between representable integers, but those holes will be different for f32 vs f64. But I guess those would always be out of bounds of the 32-bit integer and cause a trap?

cc @sunfishcode, who has delved into these depths more than I have...

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Can this take a code: u8 instead of a code: u32 if it is just going to code.try_into().unwrap() it? Is there a reason for the weird mismatch of bit widths?

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Yeah, expanding on my previous naming bikeshed, I think phrasing/renaming this stuff as virtual_memory_traps or maybe signals_based_traps will clarify things for users a lot here.

The latter is slightly more precise in that a ud2 isn't actually related to virtual memory, but opens the can of worms that is "windows has something else that is similar to signals but not actually called signals" I think.

The other thing that might be good to flush out a tiny bit more here is the portability hazard bit and say something about which environments might want to disable these handlers. Even just adding something like "for example, embedded operating systems that do not support virtual memory" in parens after "can be a portability hazard in some environments" is probably good enough.

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

Thanks for writing this test! Could you also add a comment at the top of the file detailing

what this test is checking, and

why it needs to be isolated in its own test binary

?

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

It would be mildly nice/tidy/future-proof to scope down the allows to just the relevant statements.

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

And maybe just a quick one-line comment about why this is unix-only and (I presume) how we don't have a way of checking whether we installed exception handlers on windows or not.

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

This seems to be identical to the component trapping libcall -- is there a reason we need two copies and can't just use this one everywhere?

Wasmtime GitHub notifications bot (Sep 12 2024 at 16:23):

fitzgen created PR review comment:

This reminds me that we should definitely be doing differential fuzzing between virtual-memory-traps={yes,no} configs. I assume you probably added that, but I haven't finished reading the PR yet. Just noting this down just in case.