Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fill in CanonicalABI.md #23

Merged
merged 21 commits into from
Apr 22, 2022
Merged

Fill in CanonicalABI.md #23

merged 21 commits into from
Apr 22, 2022

Conversation

lukewagner
Copy link
Member

@lukewagner lukewagner commented Apr 12, 2022

This PR rebases interface-types/#140 onto the Component Model, building on all the great work @alexcrichton already did and filling in some of the developments since then.

Compared to interface-types/#140, this PR makes the following updates:

  • The Python/Rust pseudo-code in interface-types/Adding a use directive to *.wit #140 has been replaced with real runnable Python (depending on the Structural Pattern Matching feature introduced in Python 3.10) and includes a small test suite. I didn't actually know Python before writing this, so apologies if it's not super idiomatic; patches welcome (but perhaps after merging, to avoid mixing semantic changes with Pythonic changes).
  • Support was adding for the 3 string encodings.
  • The per-element free calls were replaced with a single (optional) call to a single post-return function. This both reduces the number of function calls and allows core wasm more flexibility in allocation strategy.
  • The exact rules for the merging of payload types during variant flattening were tweaked a bit to have fewer cases.
  • Flags may be packed into a u8 or u16 in memory when there are less than 8 or 16 flags, resp.
  • The "size" measure was split into slightly different "element size" and "byte size" measures, the former used by lists and the latter used otherwise.
  • Support was added for putting all params and/or results in linear memory when flattening would produce too many params/results. The stopgap solution for lack of multi-value return gets to reuse this general support.
  • Since they aren't in the general explainer yet (and need to be tweaked before doing so), handle/buffer support is not included.

Lastly, this PR makes the following tweaks to the overall proposal (AST/binary/text) that came up as part of fleshing out the canonical ABI:

  • Variants (and specializations of variants) now explicitly require >=1 cases (and a comment explaining why in the "Type Definitions" section of Explainer.md)
  • unit was made a specialization of record (as it should've been in the first place). This is mostly a spec-internal change; the binary/text format isn't affected.
  • As a text-format change, the string canonopt was renamed string-encoding to be a bit more clear.
  • The into canonopt, which took an index to an instance, was replaced with separate canonopts for individual export of the into instance. This made sense given the post-return change (which doesn't belong as an export of the instance that exports memory).

Copy link
Collaborator

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this all up! I really like the idea of adding a Python test suite which we can run and ideally add tests for in the future as well to ensure that this is internally consistent.

Most of the changes from the original draft all make sense to me, although I left a few comments below about some that I didn't fully understand.

design/mvp/canonical-abi/run_tests.py Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
post_return: types.FunctionType

def load(opts, ptr, t):
assert(ptr == align_to(ptr, alignment(t)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As-is I don't think that assert is the right thing for alignment here. Above this mentions that assert should never fire, but the raw return of a function is currently passed to load (similar to the raw return of realloc is passed to store) meaning that this could fail at runtime.

Historically I vaguely remember concluding with you at one point that we should pass alignment around but not require it. If that's the case I think a few load/store algorithms need tweaking to respect misaligned pointers. Otherwise though I think this should change to trap_if and/or the initial calls to load and store should have trap_if alignment checks and this would continue to be an assert

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right! Yes, you're right, there should be a dynamic trap here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I ended up putting the traps on what I think are the minimal cut points: where we accept a pointer from wasm (viz. lifting/lowering lists and too-many-flat), instead of load/store.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify though why do we want to require alignment in the canonical ABI but not in core wasm itself? Personally I see alignment being passed so the language, if it requires it, can take advantage of it but it's not required to take advantage of it. We can already make all accesses on a host or similar work with unaligned addresses so this otherwise runs a small risk of making conversion of arguments/returns slower since it's another thing to check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, so you're saying to just drop the alignment assertion altogether?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a good idea to ask why since you're right that, in general, wasm doesn't enforce alignment. I think perhaps the difference here is that core wasm load and store and the bulk-mem ops are engine-internal, so that the engine can paper over any alignment restrictions in the host (with, e.g., traps and byte-loads). Also, we did actually want and consider traps, but the problem is that it would just be cost-prohibitive on many implementations. With the Canonical ABI, we're hoping to pass out raw pointers to the host which may have ambient alignment assumptions (not just in the CPU, but also perhaps the compiler), so if I pass out a (list (tuple u32 u32)), ideally I'd be able to pass it out with a pointer-to-struct { uint32_t, uint32_t }, but if it's unaligned, that may not be legal.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that when thinking about the raw-pointer-into-wasm-linear-memory issue another major issue to consider beyond alignment is endianness, for example the C struct you mentioned would only work well on little-endian hosts. Additionally C has such weird alignment of all its integers types across various platforms I found that at least in Wasmtime it's common to have wrappers around the pointers into wasm linear memory. In that sense I've never felt the need that anything would have been nicer/easier if the wasm pointers were aligned.

The consequence of this, though, is that for example in Rust if you get a list<u64> from wasm then what you actually get in Rust is something akin to &[Le<u64>] where Le is force-alignment-to-one and has an accessor that does the endian switch as appropriate. This would, though, be a hindrance to calling any native Rust API that requires something lke &[u64].

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know that there are a variety situations where the canonical ABI won't line up, but if it mostly lines up for many popular systems for simple types like list<u64>, alignment still seems like a net win?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least from a library author perspective I don't find it useful if the representation only lines up on some hosts but not others. For example I wouldn't want wasmtime to have an entirely different API on little-endian vs big-endian platforms since that's just pushing all of the portability issues to end-users rather than handling them in Wasmtime itself. The endianness issue is the sticking point for me, personally, where that means we'll never hand out "absolutely raw" views into linear memory, they'll always have a wrapper of one form or another around them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate from the question of whether it's worth it for Wasmtime in particular to be endian-agnostic, I think there are in general going to be implementations that are happy to specialize to little-endian and benefit from this simplification. Thus, there will be implementations (including perhaps Wasmtime at some future point, perhaps as a binding-generation option) that do want to take advantage of being able to pass out raw typed pointers to linear memory. This is also a conservative starting point, since one can always relax the trapping behavior in the future.

design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Show resolved Hide resolved
Add GitHub Actions workflow to test canonical abi
alexcrichton added a commit to alexcrichton/witx-bindgen that referenced this pull request Apr 19, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
alexcrichton added a commit to bytecodealliance/wit-bindgen that referenced this pull request Apr 19, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
@lukewagner
Copy link
Member Author

Thanks for all the great feedback! It looks like things have quieted down; if it's still quiet by the end of the week, I'll merge (then move on to fixing #11).

if not equal_modulo_string_encoding(got, lower_v):
fail("{} re-lift expected {} but got {}".format(test_name(), lower_v, got))

test(Unit(), [], {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a test for float NaN canonicalization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Rather than generalize the existing test function to handle nan not being equal itself, I made a separate special testing function.

design/mvp/Binary.md Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
design/mvp/Explainer.md Outdated Show resolved Hide resolved
design/mvp/CanonicalABI.md Outdated Show resolved Hide resolved
@lukewagner
Copy link
Member Author

Ok, merging. As always, we can continue to iterate on this in new issues and PRs.

@lukewagner lukewagner merged commit 06cc296 into main Apr 22, 2022
@lukewagner lukewagner deleted the canonical-abi branch April 22, 2022 22:00
willemneal pushed a commit to AhaLabs/wit-bindgen that referenced this pull request May 31, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
alexcrichton added a commit to alexcrichton/wasm-tools that referenced this pull request Nov 16, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
guybedford pushed a commit to bytecodealliance/jco that referenced this pull request Dec 17, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
guybedford pushed a commit to bytecodealliance/jco that referenced this pull request Dec 17, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
guybedford pushed a commit to bytecodealliance/jco that referenced this pull request Dec 21, 2022
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
rvolosatovs pushed a commit to bytecodealliance/wrpc that referenced this pull request May 23, 2024
I today started looking at updating this project to the new draft of the
canonical ABI specified at WebAssembly/component-model#23 and while
there's quite a few changes that need to happen the first thing that
struck me was that maintaining all of the support for old-style witx
files is pretty onerous. Especially the ABI-calculating code has lots of
special cases for various witx constructs and the Preview1 ABI, and I
wasn't really sure how to update this in many situations.

Overall the original purpose of the witx support was to prove out that
it's possible to support both the old witx abi and the new canonical ABI
in the same generator. The canonical ABI has changed significantly in
the meantime however and this doesn't necessarily make sense to do any
more. I think it would be best now to reevaluate at the point when WASI
is ready to switch to the component model what to do with the old witx
support. I no longer think that "build it in here" is the obvious
option. As this diff shows there's quite a bit of weight to carry the
old witx abis as this commit clocks in at nearly 7000 lines removed.

The specifics being dropped here are:

* Parsing support for `*.witx`
* Support for `Pointer` and `ConstPointer`
* Support for `WitxInstruction`
* Support for push/pull buffers

The push/pull buffer feature was never actually fully implemented, even
for Rust hosts they only kind-of worked. Most other generators never
even implemented support for them. Additionally support for other
`*.witx` constructs was somewhat spotty at best with very few tests.

My hope is that there are no existing users of this support. If there
are then I think it's best to re-evaluate how best to solve the scenario
on-hand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants