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

lib/wasi-types: add Socktype::Unused shim value #3878

Closed
wants to merge 2 commits into from
Closed

lib/wasi-types: add Socktype::Unused shim value #3878

wants to merge 2 commits into from

Conversation

waynr
Copy link
Contributor

@waynr waynr commented May 18, 2023

Description

In wasix-libc we derive the value of the C macro SOCK_DGRAM and SOCK_STREAM from the sock_type enum in wasix-witx. This sock_type witx enum corresponds roughly to the Wasmer socktype enum.

The problem with these corresponding enums as-written is that they result in a SOCK_DGRAM value of 0. This doesn't work for cpython (and potentially other programs depending on libc) because the value 0 is used for a cpython-specific macro that is used in a switch case arm alongside SOCK_DGRAM:

This cpython hack seems likely to me to be replicated elsewhere in the open source world and implies that most libc implementations likely don't have any SOCK_* macros with the value 0.

This PR is paired with a corresponding wasix-witx PR. Both PRs utilize the same hack: introduce a shim enum variant to ensure that none of the other variants take the value 0.

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

This will break all wasix code we have compiled that uses sockets.

I do think it makes sense to have this instead of doing the mapping in libc, because that creates confusing ambiguities, but we'll need to change the wasix crate and the libc in unison with this PR.

And then re-compile everything using sockets, including the snapshot tests.

Thoughts, @john-sharratt ?

@waynr
Copy link
Contributor Author

waynr commented May 23, 2023

This will break all wasix code we have compiled that uses sockets.

I do think it makes sense to have this instead of doing the mapping in libc, because that creates confusing ambiguities, but we'll need to change the wasix crate and the libc in unison with this PR.

And then re-compile everything using sockets, including the snapshot tests.

Thoughts, @john-sharratt ?

John and I discussed this last night and came to the same conclusion as you, but we decided we would wait to merge it until we are reasonably certain no more breaking changes are needed for what I'm working on.

@theduke
Copy link
Contributor

theduke commented May 25, 2023

This was included in #3906 .

@theduke theduke closed this May 25, 2023
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.

None yet

2 participants