Conversation
This was referenced Feb 6, 2020
Contributor
Author
|
Unfortunately I discovered that I had overlooked the way the struct layout of Instead, I'm going to rework this to make tagged unions a new sort of datatype, use those in ephemeral & snapshot 2 onwards, and set us on a path to deprecate untagged unions in witx once snapshot 1 is no longer in use. |
Contributor
Author
|
I found a way to maintain ABI compatibility and move forward with this approach: The We can upgrade to using a tagged union for |
prior to this PR, event_u was a union with only one variant fd_readwrite. the change into tagged union ended up changing the memory layout of the event struct. so, rather than use tagged unions in the event struct, we just use a struct that you ignore in the case of the clock event, which has the same ABI as previously. in ephemeral, we use a tagged union for events, because we dont have any ABI stability guarantees to maintain there.
this is much more convenient for consumers, and it reflects the requirement of the syntax to provide an Id
pchickey
force-pushed
the
pch/tagged_unions
branch
from
300871a to
7219c5a
Compare
Contributor
Author
|
I now believe this work is complete. The PR comment on WebAssembly/wasi-libc#165 has been updated with a comparison of the layout of each union-related type before and after this change. The static assertions indicate that clang agrees with the layout calculations made in this PR, and manual inspection indicates that the layout is the same as prior to this PR. |
9 tasks
Closed
yoshuawuyts
pushed a commit
to yoshuawuyts/WASI
that referenced
this pull request
Nov 25, 2025
* unions are now tagged by an enum * union fields may be empty. TODO: write tests * unions: add more tests. compute layout. * snapshot 0: translate untagged union syntax to tagged union syntax * snapshot: update from untagged union to tagged union syntax * snapshot: update docs * snapshot 0: update docs * ephemeral: translate untagged unions to tagged unions * render: fix union output * bump witx to 0.8.0 - this is definitely semver breaking * snapshot: I forgot to delete the tag field `subscription.type` * regen docs * unionlayout: derive the usual suspects * snapshot 1 and 0: put tag back in event struct, flatten event_u prior to this PR, event_u was a union with only one variant fd_readwrite. the change into tagged union ended up changing the memory layout of the event struct. so, rather than use tagged unions in the event struct, we just use a struct that you ignore in the case of the clock event, which has the same ABI as previously. in ephemeral, we use a tagged union for events, because we dont have any ABI stability guarantees to maintain there. * layout: refactor so we have impls for NamedType and Type too * ast: change tag of UnionDatatype to be a NamedType this is much more convenient for consumers, and it reflects the requirement of the syntax to provide an Id * fixup docs
yoshuawuyts
pushed a commit
to yoshuawuyts/WASI
that referenced
this pull request
Nov 25, 2025
* unions are now tagged by an enum * union fields may be empty. TODO: write tests * unions: add more tests. compute layout. * snapshot 0: translate untagged union syntax to tagged union syntax * snapshot: update from untagged union to tagged union syntax * snapshot: update docs * snapshot 0: update docs * ephemeral: translate untagged unions to tagged unions * render: fix union output * bump witx to 0.8.0 - this is definitely semver breaking * snapshot: I forgot to delete the tag field `subscription.type` * regen docs * unionlayout: derive the usual suspects * snapshot 1 and 0: put tag back in event struct, flatten event_u prior to this PR, event_u was a union with only one variant fd_readwrite. the change into tagged union ended up changing the memory layout of the event struct. so, rather than use tagged unions in the event struct, we just use a struct that you ignore in the case of the clock event, which has the same ABI as previously. in ephemeral, we use a tagged union for events, because we dont have any ABI stability guarantees to maintain there. * layout: refactor so we have impls for NamedType and Type too * ast: change tag of UnionDatatype to be a NamedType this is much more convenient for consumers, and it reflects the requirement of the syntax to provide an Id * fixup docs
yoshuawuyts
pushed a commit
to yoshuawuyts/WASI
that referenced
this pull request
Nov 25, 2025
* unions are now tagged by an enum * union fields may be empty. TODO: write tests * unions: add more tests. compute layout. * snapshot 0: translate untagged union syntax to tagged union syntax * snapshot: update from untagged union to tagged union syntax * snapshot: update docs * snapshot 0: update docs * ephemeral: translate untagged unions to tagged unions * render: fix union output * bump witx to 0.8.0 - this is definitely semver breaking * snapshot: I forgot to delete the tag field `subscription.type` * regen docs * unionlayout: derive the usual suspects * snapshot 1 and 0: put tag back in event struct, flatten event_u prior to this PR, event_u was a union with only one variant fd_readwrite. the change into tagged union ended up changing the memory layout of the event struct. so, rather than use tagged unions in the event struct, we just use a struct that you ignore in the case of the clock event, which has the same ABI as previously. in ephemeral, we use a tagged union for events, because we dont have any ABI stability guarantees to maintain there. * layout: refactor so we have impls for NamedType and Type too * ast: change tag of UnionDatatype to be a NamedType this is much more convenient for consumers, and it reflects the requirement of the syntax to provide an Id * fixup docs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prior to this PR, witx's
unionwas exactly like a C union - described a set of variants that occupy the same location in memory, but gave no information about which variant was present/valid. All uses ofunionin the WASI spec were found in structs directly following anenumthat tagged the union (indicated which variant was present/valid). Doc comments described the mapping of enum tags to union variants.This implicit specification of tagged unions is hard for our growing suite of automated code-generation tools to deal with automatically. Many programming languages do not have a concept of an untagged union, and even in those that do, its typically safer and easier to use a tagged union concept. Additionally, the concept of an untagged union is not compatible with the interface types proposal, where we expect to have ADTs available (a generalized form of tagged unions) but not untagged unions.
This PR changes the way witx union is defined to require a tag (enum) type identifier to be specified as the first argument:
Each member of the tag enum must be given a corresponding
fieldin the union. A new empty field syntax(empty $c)is provided for tags that do not have data associated with them.This change to the witx syntax and semantics allows us to replace all of the implicit tagged union definitions in the WASI witx specs to be explicit tagged unions. All of the uses in the WASI specs are completely ABI-compatible with the new form. In other words, the changes in this PR do not change the functional specification of WASI, they just change the way in which we specify it. Old implementations of WASI snapshot 0 or 1 should continue to work against new implementations that use the new syntax.
This PR makes updates to all of the witx specs describing WASI phases in this repository, so that the existing
$event_u,$subscription_u, and$prestat_uunions are annotated with the enums that tag them. Inevent_uandsubscription_u, I had to make a separate variant (with the same type) forfd_readandfd_writerather than use the samefd_readwritevariant for each. This is a semantic difference that code generators can present to the programmer (i.e. the C union generated by wasi-headers will be slightly different) but at the ABI level, all fd_read and fd_write variants are represented in-memory exactly as before.The use sites of unions were simple to translate. The
prestat_utype was able to be renamed toprestat, and the existingprestatstruct erased, because it was just a pair of tag and union. Foreventandsubscription, I was able to erase the tag member from the struct and leave just the union. The memory layout specified for tagged unions insrc/layout.rsis equivelant to the tag, followed by the untagged union, as two struct members in sequence.Because ABI compatibility for existing WASI snapshots is critical to accepting this PR, I'm going to stage PRs on all of the witx-consuming tools in the ecosystem presently (
wasi-headersin https://github.com/cranestation/wasi-libc, https://github.com/bytecodealliance/wasi,wigin https://github.com/bytecodealliance/wasmtime- please ping me if I'm missing any) that update them to use these changes, and test all of the new implementations to make sure they are indeed ABI compatible with the existing WASI ecosystem.I will add comments and links to this PR as I complete work on those tools, and wait to merge this PR until all feedback has been addressed and everything is ready to go.