Stream: git-wasmtime

Topic: wasmtime / PR #7995 Add a "custom" platform configuration...

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 00:49):

alexcrichton opened PR #7995 from alexcrichton:min-platform-upstream to bytecodealliance:main:

This commit leverages adds a new "platform" to Wasmtime to be supported in the crates/runtime/src/sys folder. This joins preexisting platforms such as Unix and Windows. The goal of this platform is to be an opt-in way to build Wasmtime for targets that don't have a predefined way to run.

The new "custom" platform requires --cfg wasmtime_custom_platform to be passed to the Rust compiler, for example by using RUSTFLAGS. This new platform bottoms out in a C API that is intended to be small and Linux-like. The C API is effectively the interface to virtual memory that Wasmtime requires. This C API is also available as a header file at examples/min-platform/embedding/wasmtime-platform.h (generated by cbindgen).

The main purpose of this is to make it easier to experiment with porting Wasmtime to new platforms. By decoupling a platform implementation from Wasmtime itself it should be possible to run these experiments out-of-tree. An example of this I've been working on is getting Wasmtime running on bare-metal with a custom kernel. This support enables defining the platform interface of the custom kernel's syscalls outside of Wasmtime.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 00:49):

alexcrichton requested fitzgen for a review on PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 00:49):

alexcrichton requested wasmtime-default-reviewers for a review on PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 00:49):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 00:53):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 01:01):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 01:02):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 01:09):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:19):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:19):

bjorn3 created PR review comment:

Is ptr guaranteed to stay valid until the memory image is freed or does the implementation need to copy the data elsewhere?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:21):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:21):

bjorn3 created PR review comment:

I assume wasmtime_munmap is used to unmap this memory range again. Is that correct?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:22):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:22):

bjorn3 created PR review comment:

Why does this need access to the memory image? It only modifies the VM mapping, right? And doesn't need to read the original content of the memory image.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:23):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:23):

bjorn3 created PR review comment:

Are all derived VM mappings guaranteed to be unmapped before calling this?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:24):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:24):

bjorn3 created PR review comment:

Why not return an error and have the rust side panic? That would allow recovering from the error. It would be pretty bad for a kernel to simply crash because of an allocation failure.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:27):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 24 2024 at 10:27):

bjorn3 created PR review comment:

What is the empty symbol used for? Is it for the STT_FILE symbols? If so skipping SymbolKind::File above makes more sense I think.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 15:08):

fitzgen submitted PR review:

r=me with answers to bjorn3's questions/comments

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 15:08):

fitzgen submitted PR review:

r=me with answers to bjorn3's questions/comments

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 15:08):

fitzgen created PR review comment:

Can we remove "probably" here? If I were attempting to implement this API, I wouldn't know what to do with that.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 15:08):

fitzgen created PR review comment:

The nomicon recommends against uninhabited types to represent C opaque types: https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs

Any reason not to follow their guidance here?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 15:08):

fitzgen created PR review comment:

Does len have to be a multiple of the system's page size? Is the resulting memory image rounded up to the page size? Is this stuff visible at all to Wasmtime?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 15:08):

fitzgen created PR review comment:

    /// Returns 1 if `wasmtime_longjmp` was not called and `callback` returned.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:31):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:31):

alexcrichton created PR review comment:

Good question, I'll expand the docs to indicate that a copy is required.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:33):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:33):

alexcrichton created PR review comment:

Correct, I'll expand the docs here

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:37):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:37):

alexcrichton created PR review comment:

I was mostly just reflecting the existing Wasmtime API, which has not been meticulously thought out, onto the C API. You're right though that this entire function I think is not necessary, and I've replaced it with a call to wasmtime_mmap_remap.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:39):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:39):

alexcrichton created PR review comment:

Not currently, no, I'll expand the documentation.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:41):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:41):

alexcrichton created PR review comment:

Ah I didn't question it much, but turns out it's SymbolKind::Null so I filtered that out to remove this case.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:46):

alexcrichton created PR review comment:

Sort of, but also sort of not. I've expanded the words here to describe that the meaning of an unhandled trap is embedder-specific. Basically if Wasmtime doesn't handle a SIGSEGV, for example, it's up to the application of what to do next. Wasmtime can't know whether that is surely an "abort the process" kind of exception.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:49):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:49):

alexcrichton created PR review comment:

Unfortunately that recommendation doesn't generate the right C header bindings.

I'm also not aware of any compiler issues using *mut Uninhabited, which is what these bindings are doing. While I know &mut Uninhabited is bad, the former is not plagued with the same issues as far as I'm aware.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:51):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:51):

alexcrichton created PR review comment:

Yes they're both guaranteed to be page aligned, updated docs to match.

Is this stuff visible at all to Wasmtime?

Could you elaborate on this? (e.g. what you mean by "stuff")

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:51):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:53):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 17:53):

alexcrichton created PR review comment:

I'd be tempted to say the embedding should crash in an embedding-defined way if possible to get better stack frames/debugging/etc. This API isn't intended to be set in stone until the end of time so I'm imagining it can change in the future to returning a result. In the meantime though whether the embedder aborts or Wasmtime aborts seems six-to-one-half-dozen-or-the-other to me

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 18:57):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 26 2024 at 18:57):

bjorn3 created PR review comment:

The embedder could return an error bringing down just a single wasm instance without crashing the entire process.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2024 at 18:09):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2024 at 18:09):

alexcrichton created PR review comment:

That's a good point yeah, and it's also true that Result is already in all these locations, so you've convinced me and I'll remove the assumed aborts here

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2024 at 18:28):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2024 at 18:29):

alexcrichton commented on PR #7995:

Ok I've updated to make the C API fallible and Wasmtime propagates the error codes (thanks for pushing back on that @bjorn3!)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2024 at 17:49):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2024 at 17:49):

alexcrichton has enabled auto merge for PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2024 at 19:22):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2024 at 20:00):

alexcrichton updated PR #7995.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 28 2024 at 20:52):

alexcrichton merged PR #7995.

Last updated: Nov 22 2024 at 16:03 UTC