fix handling of ZST in win64 ABI on windows-msvc targets #135204

RalfJung · 2025-01-07T15:00:00Z

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 #132893

rustbot · 2025-01-07T15:00:09Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

ChrisDenton · 2025-01-07T15:08:48Z

cc @mati865 as this makes a change to the windows-gnu target. Does it sound right to you?

mati865 · 2025-01-07T16:13:34Z

I don't remember the details about win64 ABI but following what GCC does is the right approach.

programmerjake · 2025-01-07T20:27:04Z

compiler/rustc_target/src/callconv/x86_win64.rs

            if cx.target_spec().os == "windows"
                && cx.target_spec().env == "gnu"
                && arg.layout.is_zst()
+                && fn_abi.conv != Conv::X86_64Win64


this needs to also cover at least fastcall and vectorcall which have basically the same issue: https://clang.godbolt.org/z/75fo4cbhn

Using fastcall and vectorcall with GCC causes the binaries to segfault, so GCC might be wrong here...

hmm, at least gcc matches clang for the short functions i tested above...

Do we have anything matching __attribute__((sysv_abi))?

If the binaries segfault then maybe it's not so clear what fastcall and vectorcall should do?

Do we have anything matching __attribute__((sysv_abi))?

yes, sysv64, however it isn't broken afaict.

If the binaries segfault then maybe it's not so clear what fastcall and vectorcall should do?

perhaps, however I think it's at least worth matching clang where they do work.

Strangely, the test fails for fastcall on windows-gnu (i.e., we do pass the ZST argument there). I don't yet understand why.

Ah, we currently have explicit logic in adjust_abi which says that on x86-64, "fastcall" should be like "C". This is obviously in contradiction with saying that "fastcall" should ignore ZST while "C" should pass them by-pointer.

I'm not a Windows expert so I don't know what is correct here.

this needs to also cover at least fastcall and vectorcall which have basically the same issue: https://clang.godbolt.org/z/75fo4cbhn

Odd, so vectorcall was correct before this PR already? Not quite sure how that is happening.

Anyway the latest version should handle vectorcall and fastcall, hopefully.

perhaps, however I think it's at least worth matching clang where they do work.

Sure, just pointed out this is a mess with GCC.
A couple years ago I noticed some fastcall tests disabled on windows-gnu because they were failing when objects were built with GCC but worked fine when built with Clang. So yeah, while GCC de facto defines the standard, it's broken in many ways.

SparrowLii

This fix makes sense!

tests/codegen/abi-win64-zst.rs

compiler/rustc_abi/src/extern_abi/mod.rs

rustbot · 2025-01-08T10:09:36Z

These commits modify compiler targets.
(See the Target Tier Policy.)

tests/codegen/abi-win64-zst.rs

programmerjake · 2025-01-08T10:13:12Z

tests/codegen/abi-win64-zst.rs

+#[lang = "sized"]
+trait Sized {}
+
+// Make sure the argument is always ignored when explicitly requesting "win64" or "sysv64" ABI.


clang has the pointer to ZST argument for win64, fastcall, and vectorcall, but not for sysv64:
https://clang.godbolt.org/z/Wr4jMWq3P

Wait, I thought the point of #132893 was that ZST arguments should get ignored for win64?

Can you translate "rustc's f reads from rcx whereas clang's f reads from rdx" into something understandable that does not involve register names?

The test passes as-is on windows-msvc. So if "win64" on windows-gnu is wrong, then it must be the case that we should be ignoring ZST everywhere. Now you say ZST are not ignored by clang. Something does not make sense here.

yeah, I'm saying both the test and rustc ABI lowering code are wrong aka. don't match clang.

Can you translate "rustc's f reads from rcx whereas clang's f reads from rdx" into something understandable that does not involve register names?

that was mostly so I could show we generate wrong assembly, it's much easier to instead match the llvm-ir that is produced:
https://clang.godbolt.org/z/en55T7Tnj

The test passes as-is on windows-msvc. So if "win64" on windows-gnu is wrong

it's the handling on rustc's msvc target that's wrong. I was using clang's gnu target merely so I could obtain a ZST to demonstrate. (rustc's gnu target may also be wrong).

Hm, that doesn't seem right to me. I'm pretty sure struct Z {} should be 1 byte. And struct Z { ty v[]; } should have the same layout as struct Z { ty v[1]; }.

if you're referencing https://clang.godbolt.org/z/en55T7Tnj from above,

No, I am talking about Rust targets.
The status quo is that extern "C" on windows-msvc skips ZST, but on windows-gnu it does not. I don't know where that comes from, but your bugreport is only about extern "win64" so I assume extern "C" is correct. If extern "win64" is meant to be the same cross-target then it should match either windows-gnu-C or windows-msvc-C. It seems you are saying that it should match windows-gnu-C, which seems rather surprising to me -- but that is what this PR does now.

The latest version of this PR always passes ZST by-ref, except for windows-msvc extern "C" functions. This makes no sense at all to me.

Should we just always use by-ref passing x86_win64? Does anyone know why we skip ZST on windows-msvc extern "C"?

I was able to track this back to e177207, which landed in #32080 which is a huge PR and I can't tell if this is new logic or carried over from earlier. The PR description indicates that this PR is where we started skipping ZST arguments, and then an exception was added to not do that on 64bit-windows-gnu. Since then the ABI adjustment logic has been refactored many times and now it looks like windows-msvc is the exception, not windows-gnu... so it seems to me like we could just consistently do this for all windows targets.

When it comes to windows-gnu*, matching whatever Clang does when targeting windows-gnu is the best approach. Dunno about msvc.

RalfJung · 2025-01-08T14:15:57Z

Cc @beetrees

programmerjake

lgtm assuming this matches clang. I'm just reviewing the test cases since i'm not that familiar with the compiler's internals.

ChrisDenton · 2025-01-09T02:22:22Z

Ok so to sum up because I was confused:

this only affects MSVC
this changes function argument passing:
- previously ZSTs were simply ignored
- now they're passed by pointer indirection
passing ZSTs is not possible with MSVC so we're using the clang behaviour

This seems like it should be fine. It's the sort of situation where I'd love if we could do a crater run just in case but that's not an option. However, considering it's a portability hazard, I would doubt people are relying on the old behaviour. I've been wrong before though.

fix handling of ZST in win64 ABI on windows-gnu targets

6241d35

rustbot assigned SparrowLii Jan 7, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 7, 2025

jieyouxu added the O-windows-gnu Toolchain: GNU, Operating system: Windows label Jan 7, 2025

programmerjake reviewed Jan 7, 2025

View reviewed changes

SparrowLii reviewed Jan 8, 2025

View reviewed changes

tests/codegen/abi-win64-zst.rs Show resolved Hide resolved

RalfJung commented Jan 8, 2025

View reviewed changes

compiler/rustc_abi/src/extern_abi/mod.rs Show resolved Hide resolved

RalfJung force-pushed the win64-zst branch from 64916c9 to ab65b22 Compare January 8, 2025 10:09

RalfJung commented Jan 8, 2025

View reviewed changes

tests/codegen/abi-win64-zst.rs Outdated Show resolved Hide resolved

programmerjake reviewed Jan 8, 2025

View reviewed changes

RalfJung force-pushed the win64-zst branch 2 times, most recently from 618c2cf to 84624b2 Compare January 8, 2025 11:58

RalfJung changed the title ~~fix handling of ZST in win64 ABI on windows-gnu targets~~ fix handling of ZST in win64 ABI on windows-msvc targets Jan 8, 2025

RalfJung force-pushed the win64-zst branch from 84624b2 to e61af85 Compare January 8, 2025 14:08

fix ZST handling for Windows ABIs on MSVC target

fa92272

RalfJung force-pushed the win64-zst branch from e61af85 to fa92272 Compare January 8, 2025 14:11

on Windows, consistently pass ZST by-ref

035bd46

programmerjake approved these changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix handling of ZST in win64 ABI on windows-msvc targets #135204

fix handling of ZST in win64 ABI on windows-msvc targets #135204

RalfJung commented Jan 7, 2025 •

edited

Loading

rustbot commented Jan 7, 2025

ChrisDenton commented Jan 7, 2025

mati865 commented Jan 7, 2025

programmerjake Jan 7, 2025 •

edited

Loading

mati865 Jan 7, 2025

programmerjake Jan 7, 2025

RalfJung Jan 8, 2025

programmerjake Jan 8, 2025

RalfJung Jan 8, 2025

RalfJung Jan 8, 2025

RalfJung Jan 8, 2025

RalfJung Jan 8, 2025

mati865 Jan 8, 2025

SparrowLii left a comment

rustbot commented Jan 8, 2025

programmerjake Jan 8, 2025 •

edited

Loading

RalfJung Jan 8, 2025 •

edited

Loading

RalfJung Jan 8, 2025

programmerjake Jan 8, 2025

programmerjake Jan 8, 2025 •

edited

Loading

ChrisDenton Jan 8, 2025 •

edited

Loading

RalfJung Jan 8, 2025

RalfJung Jan 8, 2025

RalfJung Jan 8, 2025

mati865 Jan 8, 2025

RalfJung commented Jan 8, 2025

programmerjake left a comment

ChrisDenton commented Jan 9, 2025

fix handling of ZST in win64 ABI on windows-msvc targets #135204

Are you sure you want to change the base?

fix handling of ZST in win64 ABI on windows-msvc targets #135204

Conversation

RalfJung commented Jan 7, 2025 • edited Loading

rustbot commented Jan 7, 2025

ChrisDenton commented Jan 7, 2025

mati865 commented Jan 7, 2025

programmerjake Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparrowLii left a comment

Choose a reason for hiding this comment

rustbot commented Jan 8, 2025

programmerjake Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

RalfJung Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

programmerjake Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

ChrisDenton Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jan 8, 2025

programmerjake left a comment

Choose a reason for hiding this comment

ChrisDenton commented Jan 9, 2025

RalfJung commented Jan 7, 2025 •

edited

Loading

programmerjake Jan 7, 2025 •

edited

Loading

programmerjake Jan 8, 2025 •

edited

Loading

RalfJung Jan 8, 2025 •

edited

Loading

programmerjake Jan 8, 2025 •

edited

Loading

ChrisDenton Jan 8, 2025 •

edited

Loading