alexcrichton opened PR #4623 from adapt-string
to main
:
This commit is a hefty addition to Wasmtime's support for the component
model. This implements the final remaining type (in the current type
hierarchy) unimplemented in adapter module trampolines: strings. Strings
are the most complicated type to implement in adapter trampolines
because they are highly structured chunks of data in memory (according
to specific encodings). Additionally each lift/lower operation can
choose its own encoding for strings meaning that Wasmtime, the host, may
have to convert between any pairwise ordering of string encodings.The
CanonicalABI.md
in the component-model repo in general specifies
all the fiddly bits of string encoding so there's not a ton of wiggle
room for Wasmtime to get creative. This PR largely "just" implements
that. The high-level architecture of this implementation is:
Fused adapters are first identified to determine src/dst string
encodings. This statically fixes what transcoding operation is being
performed.The generated adapter will be responsible for managing calls to
realloc
and performing bounds checks. The adapter itself does not
perform memory copies or validation of string contents, however.
Instead each transcoding operation is modeled as an imported function
into the adapter module. This means that the adapter module
dynamically, during compile time, determines what string transcoders
are needed. Note that an imported transcoder is not only parameterized
over the transcoding operation but additionally which memory is the
source and which is the destination.The imported core wasm functions are modeled as a new
CoreDef::Transcoder
structure. These transcoders end up being small
Cranelift-compiled trampolines. The Cranelift-compiled trampoline will
load the actual base pointer of memory and add it to the relative
pointers passed as function arguments. This trampoline then calls a
transcoder "libcall" which enters Rust-defined functions for actual
transcoding operations.Each possible transcoding operation is implemented in Rust with a
unique name and a unique signature depending on the needs of the
transcoder. I've tried to document inline what each transcoder does.This means that the
Module::translate_string
in adapter modules is by
far the largest translation method. The main reason for this is due to
the management around calling the imported transcoder functions in the
face of validating string pointer/lengths and performing the dance of
realloc
-vs-transcode at the right time. I've tried to ensure that each
individual case in transcoding is documented well enough to understand
what's going on as well.Additionally in this PR is a full implementation in the host for the
latin1+utf16
encoding which means that both lifting and lowering host
strings now works with this encoding.Currently the implementation of each transcoder function is likely far
from optimal. Where possible I've leaned on the standard library itself
and for latin1-related things I'm leaning on theencoding_rs
crate. I
initially tried to implement everything withencoding_rs
but was
unable to uniformly do so easily. For now I settled on trying to get a
known-correct (even in the face of endianness) implementation for all of
these transcoders. If an when performance becomes an issue it should be
possible to implement more optimized versions of each of these
transcoding operations.Testing this commit has been somewhat difficult and my general plan,
like with the(list T)
type, is to rely heavily on fuzzing to cover
the various cases here. In this PR though I've added a simple test that
pushes some statically known strings through all the pairs of encodings
between source and destination. I've attempted to pick "interesting"
strings that one way or another stress the various paths in each
transcoding operation to ideally get full branch coverage there.
Additionally a suite of "negative" tests have also been added to ensure
that validity of encoding is actually checked.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->Closes #4309
alexcrichton requested fitzgen for a review on PR #4623.
alexcrichton updated PR #4623 from adapt-string
to main
.
alexcrichton updated PR #4623 from adapt-string
to main
.
fitzgen created PR review comment:
This isn't us being optimistic tho, right? This optimism is prescribed by the spec, correct?
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
I feel like this method is already super large, so adding a bunch of large closures into it is not helping. How much stuff is this really closing over? Can it and the other closures be a separate method?
fitzgen created PR review comment:
I thought that we settled on bounds checks regardless if the string is empty or not?
alexcrichton updated PR #4623 from adapt-string
to main
.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Sure, I've split out everything now.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Indeed!
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This was written before the spec changed, and I was originally going to land this first and then fixup all our handling of zero-length arrays and things but I went ahead and updated this.
alexcrichton has enabled auto merge for PR #4623.
alexcrichton merged PR #4623.
Last updated: Dec 23 2024 at 12:05 UTC