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

Experimental raw-dylib support #2149

Closed
wants to merge 5 commits into from
Closed

Experimental raw-dylib support #2149

wants to merge 5 commits into from

Conversation

kennykerr
Copy link
Collaborator

@kennykerr kennykerr commented Nov 10, 2022

This is a branch to validate and experiment with raw-dylib support in the windows and windows-sys crates. I don't think I'll merge this until the necessary features are stabilized but it will let us kick the tires. The main change is the introduction of the windows_link macro that abstracts away all of the differences between the old link attribute and the new link modes necessary for raw-dylib to work. The windows-bindgen crate can then generate something like this:

windows_link!("kernel32.dll", "stdcall" fn CloseHandle(handle: HANDLE) -> BOOL);

Rather than the traditional form:

#[link(name = "windows")]
extern "system" {
    pub fn CloseHandle(handle: HANDLE) -> BOOL;
}

The windows_link macro will generate the traditional form if the raw_dylib feature is not activated. In that mode, it continues to rely on the target libs provided by the windows and windows-sys crates. However, the raw_dylib feature ensures that no libs are needed and the name of the DLL is used to instruct the Rust compiler to inject the appropriate import table entry directly and without the use of an import lib.

You can try this today with the nightly compiler. I added the optional raw_dylib feature to a few of the samples so that you can experiment with this as follows:

D:\git\windows-rs>cargo run -p sample_create_window_sys --features raw_dylib
    Finished dev [unoptimized + debuginfo] target(s) in 0.08s
     Running `target\debug\sample_create_window_sys.exe`
WM_PAINT
WM_DESTROY
D:\git\windows-rs>cargo run -p sample_dcomp --features raw_dylib
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
     Running `target\debug\sample_dcomp.exe`
deck:
 o a H Z t z
 n T I A O Y
 i N N y n h
initial dpi: (144.0, 144.0)
build device

Some thoughts:

  • The new windows_link macro generates a lot more code than the traditional form but it doesn't appear to impact build time in any meaningful way.
  • It's no longer very practical to use the doc attribute on imports since it cannot be applied to the windows_link macro.
  • I can't easily turn off the target (lib) dependencies conditionally, so they'll likely remain until the raw-dylib feature is stable and ubiquitous, but it would be great if we didn't have to download those anymore.
  • I'll probably switch the windows crate over to raw-dylib unconditionally but keep the windows-sys optional to retain support for an older MSRV.
  • The windows_link macro could enable optional support for delay loading, similar to the MSVC linker's /DELAYLOAD option. I'm not sure how popular that would be, but I do know a few crates that would benefit.
  • rustfmt has a hard time formatting the windows_link macros. Instead of this desirable format:
windows_link!("kernel32.dll", "stdcall" fn CloseHandle(handle: HANDLE) -> BOOL);

We end up with something like this:

windows_link ! ( "kernel32.dll" ,"system" fn CloseHandle ( handle : HANDLE ) -> BOOL );

@kennykerr
Copy link
Collaborator Author

#[doc(hidden)]
#[macro_export]
macro_rules! windows_link {
($library:literal, $abi:literal fn $name:ident($($arg:ident: $argty:ty),*)->$ret:ty) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comma between the library and abi seems a little awkward...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can remove it - wasn't sure if that was better/worse esthetically.

#[macro_export]
macro_rules! windows_link {
($library:literal, $abi:literal fn $name:ident($($arg:ident: $argty:ty),*)->$ret:ty) => (
#[cfg(all(feature = "raw_dylib", target_arch = "x86"))]
Copy link

@bjorn3 bjorn3 Nov 10, 2022

Choose a reason for hiding this comment

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

Maybe move the cfg's from the macro body to the macro definition. That is have 4 windows_link macros each producing only a single extern block with the cfg determining which macro is enabled. This should reduce the amount of code rustc has to churn through to a fourth of what it currently has to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good idea!

@@ -5,6 +5,7 @@ Learn more about Rust for Windows here: <https://github.com/microsoft/windows-rs
#![no_std]
#![doc(html_no_source)]
#![allow(non_snake_case, clashing_extern_declarations)]
#![cfg_attr(feature = "raw_dylib", feature(raw_dylib), feature(native_link_modifiers_verbatim))]
Copy link

@bjorn3 bjorn3 Nov 10, 2022

Choose a reason for hiding this comment

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

You can enable multiple featurs with a single #![feature(a, b)].

@bjorn3
Copy link

bjorn3 commented Nov 10, 2022

rustfmt has a hard time formatting the windows_link macros.

Rustfmt doesn't format macro invocation bodies at all as it has no semantic knowledge about if it should format as expression, item, pattern, ... I would have expected it to be able to turn foo ! () into foo!(), but it seems not.

@bjorn3
Copy link

bjorn3 commented Nov 10, 2022

It's no longer very practical to use the doc attribute on imports since it cannot be applied to the windows_link macro.

You could still use it within the body of the macro. A macro will see a doc comment as #[doc = "..."], so you could do $(#[$(attr:tt)*])* and then apply $(#[$($attr)*)* before the fn $name I think.

@@ -374,3 +374,7 @@ Win32_UI_WindowsAndMessaging = ["Win32_UI"]
Win32_UI_Wpf = ["Win32_UI"]
Win32_UI_Xaml = ["Win32_UI"]
Win32_UI_Xaml_Diagnostics = ["Win32_UI_Xaml"]

# These features are unstable and require the nightly Rust compiler:
raw_dylib = []
Copy link

@udoprog udoprog Nov 10, 2022

Choose a reason for hiding this comment

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

If you're confident in your compatibility layer and want to merge this feature; you could opt for putting it behind a --cfg windows_nightly option instead of a feature flag. Such a flag wouldn't be prone to transitive activation through dependencies, and it would allow people to more easily experiment with unstable features and avoid the bitrot associated with maintaining a branch.

See e.g. tokio_unstable for reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooh, that looks handy - thanks! Features really aren't great for this kind of thing.

@kennykerr
Copy link
Collaborator Author

Thanks for the great feedback everyone! I'm going to close this draft PR and open a new PR to introduce raw_dylib, debugger_visualizer, and delay-load support using --cfg options as @udoprog suggested.

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.

4 participants