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

ABI mismatch between rustc and clang for passing ZSTs using the win64 ABI. #132893

Closed
programmerjake opened this issue Nov 11, 2024 · 14 comments · Fixed by #135204
Closed

ABI mismatch between rustc and clang for passing ZSTs using the win64 ABI. #132893

programmerjake opened this issue Nov 11, 2024 · 14 comments · Fixed by #135204
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-FFI Area: Foreign function interface (FFI) C-bug Category: This is a bug. O-windows-msvc Toolchain: MSVC, Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@programmerjake
Copy link
Member

programmerjake commented Nov 11, 2024

tldr: the win64 ABI is intended to be the exact same ABI across all x86-64 windows targets, however rustc on one target currently doesn't match what clang and gcc do when passing ZSTs as function arguments.

I tried this code:
https://clang.godbolt.org/z/eT5xEz757
rustc -O -C debuginfo=0 --target=x86_64-pc-windows-msvc

use std::ffi::c_int;

#[repr(C)]
pub struct Z {
    v: [c_int; 0],
}

const _: () = assert!(std::mem::size_of::<Z>() == 0);

#[no_mangle]
extern "win64" fn f(a: Z, b: c_int) -> c_int {
    let _ = a;
    b + 1
}

#[no_mangle]
extern "win64" fn g(a: c_int) -> c_int {
    a
}

clang -O -g0 --target=x86_64-pc-windows-gnu

struct Z {
    int v[0];
};

_Static_assert(sizeof(struct Z) == 0, "Z should be a ZST");

int __attribute__((ms_abi)) f(struct Z a, int b) {
    return b + 1;
}

int __attribute__((ms_abi)) g(int a) {
    return a;
}

I expected to see this happen:
the generated assembly reads from the same register no matter if using clang or rustc.

Instead, this happened: rustc's f reads from rcx whereas clang's f reads from rdx.

Meta

rustc --version --verbose:

rustc 1.82.0 (f6e511eec 2024-10-15)
binary: rustc
commit-hash: f6e511eec7342f59a25f7c0534f1dbea00d01b14
commit-date: 2024-10-15
host: x86_64-unknown-linux-gnu
release: 1.82.0
LLVM version: 19.1.1

It also does the exact same thing on:

rustc 1.84.0-nightly (b91a3a056 2024-11-07)
binary: rustc
commit-hash: b91a3a05609a46f73d23e0995ae7ebb4a4f429a5
commit-date: 2024-11-07
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3

clang version: 19.1.0

Generated Assembly

clang's assembly:

f:
        lea     eax, [rdx + 1]
        ret

g:
        mov     eax, ecx
        ret

rustc's assembly

f:
        lea     eax, [rcx + 1]
        ret

g:
        mov     eax, ecx
        ret

@programmerjake programmerjake added the C-bug Category: This is a bug. label Nov 11, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 11, 2024
@workingjubilee
Copy link
Member

@rustbot label: +A-ABI +A-FFI +O-windows-msvc

@rustbot rustbot added A-ABI Area: Concerning the application binary interface (ABI) A-FFI Area: Foreign function interface (FFI) O-windows-msvc Toolchain: MSVC, Operating system: Windows labels Nov 11, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 11, 2024
@Fulgen301
Copy link
Contributor

This code fails to compile if you target x86_64-pc-windows-msvc in Clang instead of x86_64-pc-windows-gnu due to sizeof(struct Z) no longer being 0. This is therefore a Clang bug between the interaction of [[gnu::ms_abi]] and its GNU target and not related to Rust.

@programmerjake
Copy link
Member Author

clang matches gcc here, so I think this is a rustc bug rather than a clang bug. the reason the assert fails is instead because the msvc target has different struct layout rules so you don't get a ZST by default, there may be some attribute or pragma that lets you get a ZST on the msvc target, but I haven't found it (though didn't particularly look).

@Fulgen301
Copy link
Contributor

Fulgen301 commented Nov 11, 2024

ZSTs aren't a thing in the MSVC ABI (nor are they in standard C or standard C++, they're a GCC extension). The only reason you can have a ZST in a MS ABI C function is because you're targeting the GNU ABI, but as the MSVC ABI doesn't have ZSTs, Clang doesn't elide the parameter. It would have made more sense for GCC and Clang to recognize that ZSTs aren't a thing in the ABI and completely elide the parameter, since you can never call that function with a non-ZST struct anyway due an to ABI mismatch, and you can't define that struct as a ZST in MSVC, but they didn't do that.

(How much sense it makes for GCC and Clang to allow that construct without even a single diagnostic is a different question.)

It would be a rustc bug if rustc promised to have exactly the same ABI for its ZST as a compiler extension to standard C, which to my knowledge it doesn't. And even if it did and therefore were to pessimize Rust for an obscure usecase, you're also comparing Clang targeting the GNU target to Rust targeting the MSVC target, which is rather pointless - why should rustc targeting MSVC care what Clang does in the GNU target?

@bjorn3
Copy link
Member

bjorn3 commented Nov 11, 2024

We only preserve ZST arguments for the win64 call conv on windows gnu targets, not on msvc targets or non-windows targets:

// x86_64-pc-windows-gnu doesn't ignore ZSTs.
if cx.target_spec().os == "windows"
&& cx.target_spec().env == "gnu"
&& arg.layout.is_zst()
{
arg.make_indirect_from_ignore();
}

@programmerjake
Copy link
Member Author

why should rustc targeting MSVC care what Clang does in the GNU target?

because they're both using the win64 ABI here, which is supposed to interoperate between those two targets, since that's how you access windows APIs using the MSVC calling convention from the GNU target

@ChrisDenton
Copy link
Member

ZSTs will be a non-issue for Windows APIs. Surely it's only a subset of the ABI that GNU declares compatible with system APIs?

@programmerjake
Copy link
Member Author

programmerjake commented Nov 11, 2024

well, the way I see it, if you can pass a ZST through the win64 ABI from both GCC and clang and they agree on an ABI (which they do), then rustc should also support that and have the same ABI for any targets that can access the x86-64 win64 ABI (so, windows msvc, windows gnu, linux (for Wine-style things or assembly or JIT-compiled code), etc.).

@ChrisDenton
Copy link
Member

I'm not sure that we can promise that because it's outside our control if it depends on everyone else agreeing on an Windows x86-64 ABI beyond what's needed to interop with the system.

If clang with a windows-gnu target has an ABI that differs from clang with a windows-msvc target then what can we do about it?

@programmerjake
Copy link
Member Author

If clang with a windows-gnu target has an ABI that differs from clang with a windows-msvc target then what can we do about it?

afaict they don't differ for passing ZSTs, the issue is more that that is vacuously true on the windows-msvc target since I don't think you can make a ZST (if you can figure out how, please share!).

@RalfJung
Copy link
Member

RalfJung commented Nov 16, 2024

If clang with a windows-gnu target has an ABI that differs from clang with a windows-msvc target then what can we do about it?

We can discuss that case when it comes up. These are all case-by-case decisions anyway.

But that's not the case we have here. What we have here is two older compilers implementing this ABI in a certain way, and us doing something else. Seems fairly clear to me that rustc should change here. Maybe we should change to "stop compilation if a ZST is passed via that ABI", but that's generally not what we do for any ABI, so "skip ZST like clang and gcc do" seems best.

@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2025

@programmerjake the original example is quite confusing since it compares a x86_64-pc-windows-msvc Rust build with a x86_64-pc-windows-gnu clang build. There is no reason to expect those different targets to use the same ABI. Is it possible to make an example that avoids this mismatch? Otherwise, it'd be good to update the issue description with an explanation for why one would expect the ABIs to match despite the targets being different. (This came up in the subsequent discussion, but we should have a good summary of this somewhere so one doesn't have to re-read the entire issue.)

@programmerjake
Copy link
Member Author

Otherwise, it'd be good to update the issue description with an explanation for why one would expect the ABIs to match despite the targets being different.

added a short tldr to the top comment

@bors bors closed this as completed in 7a202a9 Jan 13, 2025
@programmerjake
Copy link
Member Author

note for future readers that #135204 only fixes the function argument ABI, the function return ABI isn't fixed for ZSTs pending the decision in rust-lang/unsafe-code-guidelines#521

Kobzol pushed a commit to Kobzol/rustc-dev-guide that referenced this issue Jan 20, 2025
fix handling of ZST in win64 ABI on windows-msvc targets

The Microsoft calling conventions do not really say anything about ZST since they do not seem to exist in MSVC. However, both GCC and clang allow passing ZST over  `__attribute__((ms_abi))` functions (which matches our `extern "win64" fn`) on `windows-gnu` targets, and therefore implicitly define a de-facto ABI for these types (and lucky enough they seem to define the same ABI). This ABI should be the same for windows-msvc and windows-gnu targets, so we use this as a hint for how to implement this ABI everywhere: we always pass ZST by-ref.

The best alternative would be to just reject compiling functions which cannot exist in MSVC, but that would be a breaking change.

Cc `@programmerjake` `@ChrisDenton`
Fixes rust-lang/rust#132893
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-FFI Area: Foreign function interface (FFI) C-bug Category: This is a bug. O-windows-msvc Toolchain: MSVC, Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants