search

microsoft / windows-rs

Rust for Windows
https://kennykerr.ca/rust-getting-started/
Apache License 2.0
10.1k stars 473 forks source link

Update Win32 metadata #3111

Closed kennykerr closed 2 weeks ago

kennykerr commented 2 weeks ago

This is the first update to the Win32 and WDK metadata for some time so there's a fair bit of ripple.

ChrisDenton commented 2 weeks ago

Hm, that failure looks similar to one I've seen elsewhere. Rustup stopped modifying PATH but it seems that -gnu was sometimes relying on this modification to find its own libraries. Try adding RUSTUP_WINDOWS_PATH_ADD_BIN=1 to the environment for gnu builders and see if it fixes it?

riverar commented 2 weeks ago

Here's the real error:

The procedure entry point CoRevokeDeviceCatalog could not be located in the dynamic link library C:\Sources\windows-rs\target\x86_64-pc-windows-gnu\debug\deps\com-bd87402ef0e70998.exe.

Interestingly, this function doesn't live in OLE32 as documented but is rather exported by an apiset (and ultimately now lives in COMBASE). Investigating further.

riverar commented 2 weeks ago

I corrected metadata locally and regenerated libs/bindings, and tests pass. Filed an issue https://github.com/microsoft/win32metadata/issues/1928 to get this fixed on the metadata side.

Now to investigate why we're just now pulling that in.

kennykerr commented 2 weeks ago

CoRevokeDeviceCatalog isn't something we use in any way, but perhaps its being used by the GNU CRT/startup code? Still the import for this function hasn't changed in windows-rs by this metadata so it's unclear how this change was caused by metadata.

riverar commented 2 weeks ago

We added it for RAII purposes about a month ago https://github.com/microsoft/win32metadata/commit/d7c12d81bdae31871dc9f4b1e763a7bf7a2b1e3b#diff-3aa5b50df6faebc7292a072a8060e1ec5d900ed37522da9168f3a35a9f33da7cR696 and with the linker stuffing everything under the sun into the import tables, ... 💥

So think we'll quickly fix up metadata and maybe try again soon.

kennykerr commented 2 weeks ago

Makes total sense - thanks Rafael!

kennykerr commented 2 weeks ago

Part of the issue is the eagerness the Rust compiler has for generating code for functions even when they're not called on the assumption that the optimizer will strip it out in the end. This is why I originally added the #[inline] attribute to all function wrappers in the windows crate. The immediate solution here is to also add the #[inline] attribute to the free functions from #3013 - this is the correct fix even if the Win32 metadata didn't have any invalid imports. So we still need to deal with https://github.com/microsoft/win32metadata/issues/1928 but at least https://github.com/microsoft/windows-rs/pull/3111/commits/b379b7f975e98113a7816b7e34ef5a1a41f80a82 unblocks windows-rs.

kennykerr commented 2 weeks ago

@ChrisDenton please take it for a spin and let me know if the new pointer handles is a better fit.

ChrisDenton commented 2 weeks ago

I've been trying it out in a number of projects and so far it's mostly just meant no longer needing as casts when using from/as/into raw_handle on std types. So yeah that's a nice, if modest, change. Though honestly the overall impact has be roughly on par with other changes (such as places where we now correctly use PCWSTR instead of *mut c_void) so it's not as big a deal as I thought it might be. Mind you, I wouldn't be surprised if there are libraries out there that will see a bigger impact.

riverar commented 2 weeks ago

@ChrisDenton Maybe a dumb question--should we be using that std HANDLE type directly instead of *mut c_void? Or is that going to cause MSRV issues?

ChrisDenton commented 2 weeks ago

We could do except that it's std only so no_std projects can't use it. This would affect the windows-sys crate and potentially people who generate their own bindings. However it ultimately doesn't matter because the std type is just an alias of *mut core::ffi::c_void so they're the exact same type in any case.