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

Update Win32 metadata #3111

Merged
merged 12 commits into from
Jun 19, 2024
Merged

Update Win32 metadata #3111

merged 12 commits into from
Jun 19, 2024

Conversation

kennykerr
Copy link
Collaborator

@kennykerr kennykerr commented Jun 18, 2024

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

@kennykerr kennykerr requested review from riverar and ChrisDenton June 18, 2024 14:32
@ChrisDenton
Copy link
Collaborator

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
Copy link
Collaborator

riverar commented Jun 18, 2024

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
Copy link
Collaborator

riverar commented Jun 18, 2024

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

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

@kennykerr
Copy link
Collaborator Author

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
Copy link
Collaborator

riverar commented Jun 18, 2024

We added it for RAII purposes about a month ago microsoft/win32metadata@d7c12d8#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
Copy link
Collaborator Author

Makes total sense - thanks Rafael!

@kennykerr
Copy link
Collaborator Author

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 microsoft/win32metadata#1928 but at least b379b7f unblocks windows-rs.

@kennykerr kennykerr merged commit d331301 into master Jun 19, 2024
89 checks passed
@kennykerr kennykerr deleted the win32metadata branch June 19, 2024 17:38
@kennykerr
Copy link
Collaborator Author

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

@ChrisDenton
Copy link
Collaborator

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
Copy link
Collaborator

riverar commented Jun 20, 2024

@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
Copy link
Collaborator

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.

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.

Consider using pointers for handle types rather than isize
3 participants