Use extern "C" instead of extern "cdecl"

[ChrisDenton]

Rust is planning on getting stricter about calling conventions so has started issuing future incomparability warnings (similar to how raw-dylib is stricter).

This PR changes uses of extern "cdecl" to extern "C". This matches the C semantics where cdecl is used on x86 and ignored for other platforms (i.e. the default C calling convention is used).

It also changes the link macros on not(windows) targets to use extern "C" exclusively. This matches the windows not(target_arch = "x86") targets so should be fine.

fixes rust-lang/rust#142330

[RalfJung]

It also changes the link macros on not(windows) targets to use extern "C" exclusively. This matches the windows not(target_arch = "x86") targets so should be fine.

"C" and "system" work on all targets, so why does not(target_arch = "x86") use "C" exclusively? Are there other ABI strings being used?

[ChrisDenton]

It also changes the link macros on not(windows) targets to use extern "C" exclusively. This matches the windows not(target_arch = "x86") targets so should be fine.

"C" and "system" work on all targets, so why does not(target_arch = "x86") use "C" exclusively? Are there other ABI strings being used?

It's due to varargs. You can't currently use "system" if the function is variadic, only "C" or "cdecl":

error[E0658]: using calling conventions other than `C` or `cdecl` for varargs functions is unstable
 --> variadic.rs:3:5
  |
3 |     pub fn DbgPrint(format: *const u8, ...) -> u32;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #136946 <https://github.com/rust-lang/rust/issues/136946> for more information

[RalfJung]

I don't understand this answer... target_arch = "x86" still uses "system" so changing the ABI just for not(target_arch = "x86") doesn't help here, does it?

[ChrisDenton]

I recall it being done this way as a workaround for rust-lang/rust#110505 but maybe that was specific to raw-dylibs at the time.

[ChrisDenton]

In any case it would be nice to be able to update the windows_link and windows_target crates independently of regenerating the bindings. Which hard coding extern "C" would allow us to do.

[kennykerr]

Is there an urgency or need for an update of these crates to be published?

Given how slow adoption is for newer versions of windows-sys and how infrequently it is updated, I am also tempted to switch it to the newer windows-link crate, noting that it would also push the MSRV for the latest version to 1.71 of the Rust toolchain (as it requires raw-dylib support).

[kennykerr]

@ChrisDenton thanks for jumping in with a fix!

[RalfJung]

Is there an urgency or need for an update of these crates to be published?

The tracking issue for this lint is rust-lang/rust#137018. It has just been added. The usual timeline for moving the lint to a hard error is on the order of 6-12 months. With a high-profile crate like this being affected, we may have to move a bit slower, but it'd still be nice to get the ball rolling. :)

It is not clear to me yet what the fallout from this is here -- does this affect all users of the crate, or just non-Windows users? I guess it used to use "cdecl" on an x86_64 windows target so it'd be all users.

[ChrisDenton]

It'll be all users that enable a cdecl function at least.

If it's possible to do a point release of the link! macro crates then that would mean there's less pressure on releasing new versions of the windows and windows-sys crates as they both go through a macro. If that's not possible then that does mean it'll take much longer for this change to take effect in practice.

[kennykerr]

If we're comfortable that this is not a breaking change to the link macro then I can push out a minor update to windows-link and windows-targets now and follow that with a major update to windows-sys et al.

[kennykerr]

Oh, the latest version of windows-sys depends on an older version of windows-targets so realistically we'd need a windows-sys update anyway to give those dependents a path forward.

[ChrisDenton]

That does complicate that idea. In that case I think it's ok to wait until the next release of windows-sys.

Then it's on rust to wait until crates have updated their windows/windows-sys dependency before making this an error.

[kennykerr]

Here's what I'm planning to release in response: #3624

[RalfJung]

So there's no way to do this without a semver break? Bummer :/

[kennykerr]

Yes for windows but not for the windows-sys crate as it is so old already (and entirely generated from metadata). In theory we could maintain multiple versioning streams but that's a lot more work to maintain and support. That's why I encourage folks to consider using the windows-bindgen crate and generate their own bindings (as the Rust Standard Library does) in order to avoid this versioning dependency.

[kennykerr]

And they have been published: https://github.com/microsoft/windows-rs/releases/tag/66

[RalfJung]

Does "release 66" match v0.60.0 on crates.io?

Btw the readme on crates.io still says to insert this into my cargo.toml:

[dependencies.windows-sys]
version = "0.59"
...

[kennykerr]

Does "release 66" match v0.60.0 on crates.io?

Yes, the release number/name is just an incremental counter of releases. They used to line up but that became impractical as different crates were published on different frequencies.

Btw the readme on crates.io still says to insert this into my cargo.toml

Grr! I forgot to update the readme. Will bump those shortly. Thanks for the heads up. One more thing to automate. 🙃

[RalfJung]

Yes, the release number/name is just an incremental counter of releases. They used to line up but that became impractical as different crates were published on different frequencies.

That's fair. But right now, reading the announcement at https://github.com/microsoft/windows-rs/releases/tag/66, I have no clue which versions of which crate it is talking about.

[kennykerr]

#3625 fixes the readmes - I'll update the release notes with specific crate version numbers once that update is published.

[briansmith]

That's why I encourage folks to consider using the windows-bindgen crate and generate their own bindings (as the Rust Standard Library does) in order to avoid this versioning dependency.

As a consumer of crates that use windows-sys or windows-bindgen, it really sucks when crates use windows-bindgen and I have to audit them, as they usually provide no help whatsoever in verifying that the source code they checked in was actually generated from windows-bindgen. Quite frequently they tweak it because they don't like the output of windows-bindgen, and then suddenly every consumer of every such crate has to be an Windows ABI expert and has to do a bunch of reverse-engineering just for the audit. "Luckily" most people just seem to YOLO it.

To the extent that having multiple versions of windows-sys in a dependency tree is a significant problem (IMO not really), it would really be great to focus on SemVer-compatible updates instead. I understand that is easier said than done, though.

[kennykerr]

Indeed, I always use a yml workflow to ensure that the generated code is consistent with what windows-bindgen actually generates. Still, I do hope to eventually get to a point where we can provide more version stability for the windows and windows-sys crates. I have not given up on that entirely. 😉

[shepmaster]

Is there an urgency or need for an update of these crates to be published?

As a concrete point, the playground will now print this out for every user using the nightly toolchain:

warning: the following packages contain code that will be rejected by a future version of Rust: windows-sys v0.59.0
n

[RalfJung]

That'll be fixed soon, we just need rust-lang/rust#142353 to land.

[shepmaster]

That'll be fixed soon

I'm also updating the crates in the playground so that specific instance of the message should go away in an hour or two.

[Berrysoft]

It also changes the link macros on not(windows) targets to use extern "C" exclusively. This matches the windows not(target_arch = "x86") targets so should be fine.

"C" and "system" work on all targets, so why does not(target_arch = "x86") use "C" exclusively? Are there other ABI strings being used?

It's due to varargs. You can't currently use "system" if the function is variadic, only "C" or "cdecl":

error[E0658]: using calling conventions other than `C` or `cdecl` for varargs functions is unstable
 --> variadic.rs:3:5
  |
3 |     pub fn DbgPrint(format: *const u8, ...) -> u32;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: see issue #136946 <https://github.com/rust-lang/rust/issues/136946> for more information

Varargs method is already __cdecl in Windows headers and should be cdecl in the winmd metadata. I don't think it needs additional handling. Now all Windows APIs are marked extern "C" and it is a bit annoying. Ref: #3626