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.

compiler/rustc_target/src/callconv/x86_win64.rs

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.

RalfJung · 2025-01-09T07:51:28Z

lgtm assuming this matches clang

I hope it matches clang but I am relying on what others tell me about this.

tests/codegen/abi-win64-zst.rs

RalfJung · 2025-01-10T11:06:20Z

compiler/rustc_target/src/callconv/mod.rs

-                spec::abi::Abi::SysV64 { .. } => x86_64::compute_abi_info(cx, self),
-                spec::abi::Abi::Win64 { .. } => x86_win64::compute_abi_info(cx, self),
+                ExternAbi::SysV64 { .. } => x86_64::compute_abi_info(cx, self),
+                ExternAbi::Win64 { .. } | ExternAbi::Vectorcall { .. } => {


This is the one actual change in this file: we treat Vectorcall as x86_win64 even on non-windows targets.

RalfJung · 2025-01-10T11:36:31Z

Okay this should be ready then.

SparrowLii

LGTM. You can r=me if @programmerjake has no concerns

compiler/rustc_target/src/callconv/x86_win64.rs

RalfJung · 2025-01-13T07:21:04Z

@bors r=SparrowLii

bors · 2025-01-13T07:21:07Z

📌 Commit 675a103 has been approved by SparrowLii

It is now in the queue for this repository.

bors · 2025-01-13T13:05:57Z

⌛ Testing commit 675a103 with merge 7a202a9...

bors · 2025-01-13T15:49:48Z

☀️ Test successful - checks-actions
Approved by: SparrowLii
Pushing 7a202a9 to master...

rust-timer · 2025-01-13T17:05:56Z

Finished benchmarking commit (7a202a9): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.7%, secondary -1.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.7%	[1.5%, 3.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.4%	[-1.4%, -1.4%]	1
All ❌✅ (primary)	2.7%	[1.5%, 3.9%]	2

Cycles

Results (secondary -6.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.4%	[-6.4%, -6.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 764.23s -> 763.215s (-0.13%)
Artifact size: 326.05 MiB -> 326.11 MiB (0.02%)

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

compiler/rustc_target/src/callconv/x86_win64.rs Outdated Show resolved Hide resolved

SparrowLii reviewed Jan 8, 2025

View reviewed changes

tests/codegen/abi-win64-zst.rs Outdated 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 2 times, most recently from e61af85 to fa92272 Compare January 8, 2025 14:11

programmerjake approved these changes Jan 9, 2025

View reviewed changes

programmerjake reviewed Jan 10, 2025

View reviewed changes

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

RalfJung mentioned this pull request Jan 10, 2025

"Any two types with size 0 and alignment 1 are ABI-compatible" vs the Windows ABI rust-lang/unsafe-code-guidelines#552

Open

RalfJung force-pushed the win64-zst branch 2 times, most recently from df7bcd4 to 0b1d7d0 Compare January 10, 2025 11:04

RalfJung commented Jan 10, 2025

View reviewed changes

RalfJung force-pushed the win64-zst branch 2 times, most recently from 42c8afd to 34bd445 Compare January 10, 2025 11:07

RalfJung force-pushed the win64-zst branch from 34bd445 to 4a0e831 Compare January 10, 2025 11:13

fix ZST handling for Windows ABIs on MSVC target

d760bb6

RalfJung force-pushed the win64-zst branch from 4a0e831 to b7902e9 Compare January 10, 2025 11:16

on Windows, consistently pass ZST by-ref

675a103

RalfJung force-pushed the win64-zst branch from b7902e9 to 675a103 Compare January 12, 2025 12:32

SparrowLii approved these changes Jan 13, 2025

View reviewed changes

compiler/rustc_target/src/callconv/x86_win64.rs Show resolved Hide resolved

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 13, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2025

bors merged commit 7a202a9 into rust-lang:master Jan 13, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 13, 2025

bors mentioned this pull request Jan 13, 2025

x86: use SSE2 to pass float and SIMD types #135408

Merged

programmerjake mentioned this pull request Jan 13, 2025

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

Closed

RalfJung deleted the win64-zst branch January 14, 2025 15:16

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

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

RalfJung commented Jan 9, 2025

RalfJung Jan 10, 2025

RalfJung commented Jan 10, 2025

SparrowLii left a comment •

edited

Loading

RalfJung commented Jan 13, 2025

bors commented Jan 13, 2025

bors commented Jan 13, 2025

bors commented Jan 13, 2025

rust-timer commented Jan 13, 2025

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

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

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 9, 2025

Choose a reason for hiding this comment

RalfJung commented Jan 10, 2025

SparrowLii left a comment • edited Loading

Choose a reason for hiding this comment

RalfJung commented Jan 13, 2025

bors commented Jan 13, 2025

bors commented Jan 13, 2025

bors commented Jan 13, 2025

rust-timer commented Jan 13, 2025

Overall result: no relevant changes - no action needed

RalfJung commented 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

SparrowLii left a comment •

edited

Loading