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

Windows SOCKET type defined inconsistently between libc vs std::os::windows #76253

Open
infinity0 opened this issue Sep 2, 2020 · 9 comments
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@infinity0
Copy link
Contributor

For some reason official MS SDK docs likes to keep the actual definition a secret

Probably the std definition should be changed to usize to match the libc crate.

@infinity0 infinity0 added the C-bug Category: This is a bug. label Sep 2, 2020
@jonas-schievink jonas-schievink added the O-windows Operating system: Windows label Sep 2, 2020
@mati865
Copy link
Contributor

mati865 commented Sep 2, 2020

From MSVC headers:

$ rg "SOCKET;" '/d/Program Files (x86)/Windows Kits/10/Include/10.0.18362.0'
D:/Program Files (x86)/Windows Kits/10/Include/10.0.18362.0\um\winsock.h
51:typedef UINT_PTR        SOCKET;

D:/Program Files (x86)/Windows Kits/10/Include/10.0.18362.0\um\WinSock2.h
122:typedef UINT_PTR        SOCKET;

Unfortunately this would be breaking change since that type is public.

@infinity0
Copy link
Contributor Author

The types have the same representation, so I imagine if anyone is converting between the two (std vs libc) they are already using try_into which is what the current rustc error message recommends.

Otherwise, any code that depends on the current incorrect definition (being identical to u32/u64) should be fixed anyway.

@retep998
Copy link
Member

retep998 commented Sep 3, 2020

From what I remember back when winapi was switching from u32/u64 to usize, it was the breaking change risk that stopped std from changing this definition. I'd love if it could be fixed.

Is crater able to actually test Windows specific changes like this yet?

@infinity0
Copy link
Contributor Author

Since rust is a young language, I'd say that breaking some old incorrectly-written code is better than forcing all future as-yet-unwritten code to contain pointless workarounds like try_into.

@tesuji
Copy link
Contributor

tesuji commented Sep 4, 2020

Could we land the changing type and revert it back to get a working compiler for testing ?

@ghost
Copy link

ghost commented Feb 11, 2021

I changed the type definition locally (8ec8939a3e11fa4953639b6794db5ecb33b444ad) and ./x.py check --stage 2 --host x86_64-pc-windows-gnu --target x86_64-pc-windows-gnu (I'm on x86_64-unknown-linux-gnu) succeeded. It seems that the compiler itself can be built sucessfully with the change.

@recatek
Copy link

recatek commented Nov 13, 2023

It looks like Rust's std::os::windows::raw::SOCKET type is defined as

#[cfg(target_pointer_width = "32")]
#[doc(cfg(all()))]
#[stable(feature = "raw_ext", since = "1.1.0")]
pub type SOCKET = u32;
#[cfg(target_pointer_width = "64")]
#[doc(cfg(all()))]
#[stable(feature = "raw_ext", since = "1.1.0")]
pub type SOCKET = u64;

Would it make more sense for this to be a usize? The equivalent windows_sys::Win32::Networking::WinSock::SOCKET in the first-party Windows crate uses a usize.

@ChrisDenton
Copy link
Member

Making a breaking change like this would require a crater run (that's where we test all the code on crates.io and open source code on GitHub). Unfortunately all our runners are for Linux, not Windows.

@ChrisDenton ChrisDenton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 13, 2023
@recatek
Copy link

recatek commented Nov 13, 2023

Linking this to rust-lang/crater#149 in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants