Clarify and test extern abi workaround for variadic function support

[kennykerr]

Improve test coverage for extern abi discrepancy.

[RalfJung]

for rust-lang/rust#110505 which has since been closed

Well, it got closed by adding rust-lang/rust#136946 which is still unstable.

So what this PR here does would already have been possible back when #2458 was landed, as far as I can see. Or am I misunderstanding something?

[kennykerr]

As raw-dylib was being stabilized there was a number of bugs I was working around. This was necessary at the time although I don't recall a more specific bug for that but @dpaoliello may remember.

[RalfJung]

I think rust-lang/rust#110505 is mostly a misunderstanding, and no work-around was ever required... the error said "must have a compatible calling convention, like C or cdecl", and then you tried "cdecl" which would have worked for normal extern blocks but not with raw-dylib. What you should have done is use "C", which is 100% equivalent to "cdecl" except that it would have worked with raw-dylib as well. The error about C-variadics simply didn't know about raw-dylib's extra limitations and hence gave confusing advice.

You apparently came to the same conclusion though. And what this PR here does is still use "C" for variadics. (The user of the macro must use "C" for them or things won't work. That's already true today before this PR.) So if that is what you mean by work-around then the work-around is still around after this PR, isn't it?

[kennykerr]

This has been hard to pin down. @ChrisDenton tried to do this in #3622 and had to revert some of it in #3628.

By workarounds I just mean things the link macro has to do behind the scenes to make things work in practice. For this PR specifically that means removing the workaround of replacing the requested $abi with "C", so its up to the caller to specify the appropriate calling convention. Are you suggesting this is wrong?

[kennykerr]

So the build is clean which means that hardcoding "C" in some cases is not necessary (whether it was ever necessary in the great matrix of things is another question). 🙃

[kennykerr]

Mostly for my own FYI: the remaining discrepancy here is that import_name_type is required for x86 but not supported for x64. I just tested this locally and that difference is still evident so we can't fold the two macro definitions together.

[ChrisDenton]

The issue was that currently the windows crates will use a different ABI string depending on the options used (hence I assumed nobody was relying on it). But one thing my initial PR turned up is that people were relying on the specific ABI string even though it doesn't ultimately change the actually used ABI.

I would support just using $abi (aka pass through the given ABI) but I do wonder if it'll break people again or if we can get away with it here.

[kennykerr]

Ah, thanks for the reminder - that was the breaking change that @Berrysoft bumped into with storing function pointers. I'll test that here but if that's still an issue I'll probably abandon this change as much as I'd like to remove the workaround.

[kennykerr]

Yep, still an issue. Here is a minimal repro for clarity - assume that the definition of GetTickCount comes from another crate like windows-sys.

windows_link::link!("kernel32.dll" "system" fn GetTickCount() -> u32);

#[cfg(target_arch = "x86")]
type GetTickCountType = unsafe extern "system" fn() -> u32;

#[cfg(target_arch = "x86_64")]
type GetTickCountType = unsafe extern "C" fn() -> u32;

static GET_TICK_COUNT: GetTickCountType = GetTickCount;

#[test]
fn store_ptr() {
    unsafe {
        GET_TICK_COUNT();
    }
}

[kennykerr]

I will instead add a more specific test for this scenario and we can remove the workaround in future when we bump the major version of windows-link.

[RalfJung]

For this PR specifically that means removing the workaround of replacing the requested $abi with "C", so its up to the caller to specify the appropriate calling convention. Are you suggesting this is wrong?

No, I think that is fully the right thing to do. I am saying the reason given in the PR description doesn't make sense -- this cannot possibly have anything to do with rust-lang/rust#110505 since nothing changed in the compiler here (except that we made the wording of the error message less confusing). If you try the example from the issue, it still errors.

My current theory is that the initial introduction of the hack that replaces the $abi with "C" is based on a misunderstanding, and this was never actually necessary -- at least not for anything related to variadics. If we didn't have the "system" ABI, I would understand what happened: if users of the macro write something like "stdcall" fn foo then indeed the override with "C" would be needed. But "system" has existed since Rust 1.0 so I am left mostly confused about why the macro was added in the first place.

[kennykerr]

The windows-targets crate and its link macro is explained here: https://kennykerr.ca/rust-getting-started/understanding-windows-targets.html

The windows-link crate and its link macro is a modern replacement explained introduced in #3450 that uses raw-dylib exclusively and is much simpler as a result.

Either way, Rust requires different syntax depending on the target arch so a macro is still preferable.

[RalfJung]

Oh yeah, you need the macro for the different attributes. (Though it could likely be done with a bunch of cfg_attr instead of swapping out the entire macro, but that's a detail.)

I was talking only about the part where the macro hard-codes the ABI to "C" on some configurations. I am not aware of anything Rust does (or used to do) that would require this. I am all for removing it (which unfortunately is a breaking change as you noted), and don't understand why it was added.

For this PR, it just means that the description should be updated, since the current description does not make logical sense. This has nothing to do with rust-lang/rust#110505 being closed. Nothing changed in the stable compiler to close that issue, we just added an unstable feature you are not using. AFAIK, this exact PR could have been landed in 2023 and would have worked just the same. Or maybe there's some actual change in rustc. But rust-lang/rust#110505 doesn't describe that change, and the PR that closed that issue (rust-lang/rust#119587) cannot be that change.

[RalfJung]

Tracking this back a bit, it seems like #3450 just copied the work-around that was originally introduced in #2458. But that just replaced a hard-coded "system" (which, yeah, won't work for variadics) by a hard-coded "C", so we have to go back even further to #2412. But that seems to just move the macros around and deduplicate their implementation. So that brings us all the way back to #2164. There's a comment thread there saying this was a workaround for rust-lang/rust#110505 but that just doesn't check out -- there's no variadics in that PR, and the PR was written half a year before rust-lang/rust#110505 was filed.

So the justification seems to be

Because the ABI is generally only relevant for x86 and "system" should work for all ARM64 and x64 Windows APIs.

which isn't quite correct for variadics, and which also doesn't explain why $abi was not used. $abi should equally work for all ARM64 and x64 Windows APIs if the functions are declared properly.

So I think there was some sort of misunderstanding, not documented anywhere publicly I could find, that led to the original hard-coding of "system" here, and since then there've been various adjustments (overwriting with "system" stopped working when variadics got added so it got changed to "C") without ever questioning the original choice of overwriting $abi, but there wasn't ever a need to overwrite $abi in the first place. (And in particular, if $abi had been used, there would never have been any problem with variadics. The users of the macro could have just written "C" fn foo for variadic functions, and things would have just worked.)

Put differently, rust-lang/rust#110505 explains #2458 (changing hard-coded "system" to "C"), but doesn't explain #2164 (hard-coding "system" in the first place).

[RalfJung]

I've filed #3672 to track entirely getting rid of this work-around.

[kennykerr]

Thanks!