Start using windows sys for Windows FFI bindings in std

[ChrisDenton]

Switch to using windows-sys for FFI. In order to avoid some currently contentious issues, this uses windows-bindgen to generate a smaller set of bindings instead of using the full crate.

Unlike the windows-sys crate, the generated bindings uses *mut c_void for handle types instead of isize. This to sidestep opsem concerns about mixing pointer types and integers between languages. Note that SOCKET remains defined as an integer but instead of being a usize, it's changed to fit the standard library definition:

#[cfg(target_pointer_width = "32")]
pub type SOCKET = u32;
#[cfg(target_pointer_width = "64")]
pub type SOCKET = u64;

The generated bindings also customizes the #[link] imports. I hope to switch to using raw-dylib but I don't want to tie that too closely with the switch to windows-sys.

---

Changes outside of the bindings are, for the most part, fairly minimal (e.g. some differences in *mut vs. *const or a few types differ). One issue is that our own bindings sometimes mix in higher level types, like BorrowedHandle. This is pretty adhoc though.

[rustbot]

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

[ChrisDenton]

(rebased on master branch)

[kennykerr]

Definitely want to avoid using a hacked windows-bindgen. 😅 Happy to discuss your needs.

[ChrisDenton]

It's mostly just making the following the edits, which I believe could be done by using the windows-metadata writer to produce our own metadata file which windows-bindgen could consume.

pub type HANDLE = *mut ::core::ffi::c_void;
pub type FindFileHandle = *mut ::core::ffi::c_void;
pub type BCRYPT_ALG_HANDLE = *mut ::core::ffi::c_void;
pub type HMODULE = *mut ::core::ffi::c_void;

pub const INVALID_HANDLE_VALUE: HANDLE = ::core::ptr::invalid_mut(!0);

#[cfg(target_pointer_width = "32")]
pub type SOCKET = u32;
#[cfg(target_pointer_width = "64")]
pub type SOCKET = u64;

The most hacky thing it does is change the way imports work to fit std's currently style. E.g.:

#[link(name = "kernel32")]
extern "system" {
    pub fn CloseHandle(hobject: HANDLE) -> BOOL;
    pub fn GetLastError() -> WIN32_ERROR;
    // etc,
}

But I do hope we can move to raw-dylib.

[kennykerr]

For the type changes, can we find some middle ground with the Win32 metadata or is there a technical reason you need these just so? I’d prefer that we don’t manage forked metadata in the short term as I’m working on some Rust-friendly metadata authoring support which will make this a lot easier in the long run.

For the imports, is that because std cannot include a crate dependency on windows-targets? Absolutely raw-dylib is the future, but you are leaving yourself susceptible to the same issues I described here:

Amanieu/parking_lot#378

We could also plausibly add a StandaloneFlags with a std mode for the standalone code generator if needed. Then at least we can manage the differences in one place and not have to resort to pre/post processing.

[kennykerr]

For that matter, I'd be happy to discuss changes to windows-sys so you can simply use that directly.

[ChrisDenton]

To take the changes one by one...

pub type HANDLE = *mut ::core::ffi::c_void;
pub type FindFileHandle = *mut ::core::ffi::c_void;
pub type BCRYPT_ALG_HANDLE = *mut ::core::ffi::c_void;
pub type HMODULE = *mut ::core::ffi::c_void;
pub const INVALID_HANDLE_VALUE: HANDLE = ::core::ptr::invalid_mut(!0);

There are still some discussion about how handle types should be... handled. By keeping the status quo within the std I was hoping to avoid trying to force that issue at this time. I suspect it may be a blocker otherwise.

#[cfg(target_pointer_width = "32")]
pub type SOCKET = u32;
#[cfg(target_pointer_width = "64")]
pub type SOCKET = u64;

This is less of an issue but Rust stably defines the SOCKET handles this way. We could have a difference between public and private types (as they are ultimately the same bit size). However, doing this is somewhat more involved due to all the places socket code is used.

#[link(name = "kernel32")]
extern "system" {
    pub fn CloseHandle(hobject: HANDLE) -> BOOL;
    pub fn GetLastError() -> WIN32_ERROR;
    // etc,
}

The linking changes again are there to keep the status quo for now. Using windows-targets means distributing windows.lib with the stdlib which is a bigger change to make, especially if it's only temporary (i.e. until we're ready to use raw-dylib in std itself). It also affects people who use custom build systems. I'd rather avoid too much churn here.

[kennykerr]

For the handle types, I would like the metadata to actually store the true native type def rather than tip the scales in favor of C#. We'll likely get that resolved on the Rust side with the new metadata authoring support.

Anyway, sounds like adding standalone flags to windows-bindgen might be a good approach here - happy to get that started if you're happy with that approach.

[ChrisDenton]

For sure! That sounds good to me.

[kennykerr]

Take this for a spin: microsoft/windows-rs#2439

[ChrisDenton]

Ok, I've updated this PR and the description to reflect the current code gen. This no longer uses a hacked windows-bindgen (see above) but a few things still differ between regular windows-sys and these bindings (see OP).

[kennykerr]

The update looks great! Let me know when you need me to publish an update of the windows-bindgen crate.

[ChrisDenton]

Sure thing. I think I'd want to wait for the next metadata update but there's no rush, I'm happy to let this sit for a little bit to give people time to comment, if anyone is inclined to do so.

[bors]

☔ The latest upstream changes (presumably #110331) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #109133) made this pull request unmergeable. Please resolve the merge conflicts.

[ChrisDenton]

Ok, I think with the latest metadata this is looking good to go.

@kennykerr, A new release of windows-bindgen would be great whenever you're ready, then I can run tests in earnest. The only other thing that I thought might be useful is a simple switch to make it simpler when we're ready to start testing raw-dylib. But I don't think that's essential.

[thomcc]

Sorry for the delay here, I've been occupied with family stuff.

This looks great and avoids most of the problems I'd normally complain about in something like this 😆. (In particular, windows-sys is heavyweight enough compared to what we need from it that I prefer this approach to a direct dep)

Only real nit is that IMO we should require the .lst file be sorted, (and verify that in CI). I'd accept an argument for why it's fine as-is if you feel really strongly, though.

[thomcc]

@rustbot author

[ChrisDenton]

I've sorted windows-sys.lst alphabetically (checked by tidy) and rebased.

@rustbot ready

[thomcc]

checked by tidy

Sorry if I'm blind, but I don't see a change to tidy.

[ChrisDenton]

Tidy checks everything between // tidy-alphabetical-start and // tidy-alphabetical-end. So I just added that to the lst file rather than making any bigger changes.

[thomcc]

Oh, my bad. Yeah, thanks

@bors r+

[bors]

📌 Commit e314a3b has been approved by thomcc

It is now in the queue for this repository.

[bors]

⌛ Testing commit e314a3b with merge 7e7483d...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 7e7483d to master...

[rust-timer]

Finished benchmarking commit (7e7483d): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.1%, -0.9%]	2
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	-1.0%	[-1.1%, -0.9%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 656.874s -> 656.5s (-0.06%)

[uweigand]

It appears this merge has broken ./x.py test on s390x-unknown-linux-gnu. I now see this failure:

   Doc-tests std
error[E0412]: cannot find type `WSADATA` in this scope
   --> /home/uweigand/rust/library/std/src/sys/windows/c/windows_sys.rs:715:63
    |
715 |     pub fn WSAStartup(wversionrequested: u16, lpwsadata: *mut WSADATA) -> i32;
    |                                                               ^^^^^^^ not found in this scope

error[E0412]: cannot find type `CONTEXT` in this scope
    --> /home/uweigand/rust/library/std/src/sys/windows/c/windows_sys.rs:3034:29
     |
3034 |     pub ContextRecord: *mut CONTEXT,
     |                             ^^^^^^^ not found in this scope

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0412`.
error: doctest failed, to rerun pass `-p std --doc`

It's unclear to me why doctest wants to build this Windows-specific file on a non-Windows platform (and why this apparently does not cause failures on any other non-Windows platform?).

[ChrisDenton]

Yes that puzzled me too so I made a PR to remove it (#111401). Hopefully that should fix the issue.