-
Notifications
You must be signed in to change notification settings - Fork 574
Description
Suggestion
The two link!
macros currently, under some circumstances, ignore the ABI given by the user and overwrite it with "C":
windows-rs/crates/libs/link/src/lib.rs
Lines 18 to 28 in d73ed7e
#[cfg(all(windows, not(target_arch = "x86")))] | |
#[macro_export] | |
macro_rules! link { | |
($library:literal $abi:literal $($link_name:literal)? fn $($function:tt)*) => ( | |
#[link(name = $library, kind = "raw-dylib", modifiers = "+verbatim")] | |
extern "C" { | |
$(#[link_name=$link_name])? | |
pub fn $($function)*; | |
} | |
) | |
} |
windows-rs/crates/libs/targets/src/lib.rs
Lines 18 to 28 in d73ed7e
#[cfg(all(windows_raw_dylib, not(target_arch = "x86")))] | |
#[macro_export] | |
macro_rules! link { | |
($library:literal $abi:literal $($link_name:literal)? fn $($function:tt)*) => ( | |
#[link(name = $library, kind = "raw-dylib", modifiers = "+verbatim")] | |
extern "C" { | |
$(#[link_name=$link_name])? | |
pub fn $($function)*; | |
} | |
) | |
} |
This has the unfortunate downside that users storing function pointers need to use matching cfg
declarations themselves, as the function pointer type will vary across configuration. It also means that turning on windows_raw_dylib
can break compilation since, again, the function type changes.
I tried digging through the history to figure out why this was done, but was unable to find a good answer. Relevant PRs are:
- Add optional support for
raw-dylib
#2164 originally introduced the ABI override, forcing the "system" ABI in some configurations - Improve target version reliability #2412 merged the two macros, preserving the ABI override
- Workaround for variadic API function support #2458 changed "system" to "C"
- Introducing the
windows-link
crate #3450 copied the "C" override to the new macro in thewindows-link
crate
There are many references to rust-lang/rust#110505, but that issue merely explains why the ABI override was changed from "system" to "C", it does not explain why there is such an ABI override in the first place.
Unfortunately, fixing this is a breaking change since existing code might already do the cfg
dance that is required to create pointers to these functions, and such code would break if the cfg
dance was suddenly not required any more. So fixing this will require a major version bump. It's probably not worth a major version bump on its own, but it'd be great to do this when a major version bump is necessary anyway.