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 usingRUSTFLAGS
. 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 atexamples/min-platform/embedding/wasmtime-platform.h
(generated bycbindgen
).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:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton requested fitzgen for a review on PR #7995.
alexcrichton requested wasmtime-default-reviewers for a review on PR #7995.
alexcrichton requested wasmtime-core-reviewers for a review on PR #7995.
alexcrichton updated PR #7995.
alexcrichton updated PR #7995.
alexcrichton updated PR #7995.
alexcrichton updated PR #7995.
bjorn3 submitted PR review.
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?
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I assume wasmtime_munmap is used to unmap this memory range again. Is that correct?
bjorn3 submitted PR review.
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.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Are all derived VM mappings guaranteed to be unmapped before calling this?
bjorn3 submitted PR review.
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.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
What is the empty symbol used for? Is it for the
STT_FILE
symbols? If so skippingSymbolKind::File
above makes more sense I think.
fitzgen submitted PR review:
r=me with answers to bjorn3's questions/comments
fitzgen submitted PR review:
r=me with answers to bjorn3's questions/comments
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.
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?
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?
fitzgen created PR review comment:
/// Returns 1 if `wasmtime_longjmp` was not called and `callback` returned.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Good question, I'll expand the docs to indicate that a copy is required.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Correct, I'll expand the docs here
alexcrichton submitted PR review.
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
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Not currently, no, I'll expand the documentation.
alexcrichton submitted PR review.
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.
alexcrichton submitted PR review.
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.
alexcrichton submitted PR review.
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.
alexcrichton submitted PR review.
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")
alexcrichton updated PR #7995.
alexcrichton submitted PR review.
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
bjorn3 submitted PR review.
bjorn3 created PR review comment:
The embedder could return an error bringing down just a single wasm instance without crashing the entire process.
alexcrichton submitted PR review.
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
alexcrichton updated PR #7995.
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!)
alexcrichton updated PR #7995.
alexcrichton has enabled auto merge for PR #7995.
alexcrichton updated PR #7995.
alexcrichton updated PR #7995.
alexcrichton merged PR #7995.
Last updated: Dec 23 2024 at 12:05 UTC