Move ZST ABI handling to `rustc_target` #125854

beetrees · 2024-06-01T11:49:35Z

Currently, target specific handling of ZST function call ABI (specifically passing them indirectly instead of ignoring them) is handled in rustc_ty_utils, whereas all other target specific function call ABI handling is located in rustc_target. This PR moves the ZST handling to rustc_target so that all the target-specific function call ABI handling is in one place. In the process of doing so, this PR fixes #125850 by ensuring that ZST arguments are always correctly ignored in the x86-64 "sysv64" ABI; any code which would be affected by this fix would have ICEd before this PR. Tests are also added using #[rustc_abi(debug)] to ensure this behaviour does not regress.

Fixes #125850

rustbot · 2024-06-01T11:49:44Z

r? @estebank

rustbot has assigned @estebank.
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

rustbot · 2024-06-01T11:49:46Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

bors · 2024-06-23T13:33:45Z

☔ The latest upstream changes (presumably #124733) made this pull request unmergeable. Please resolve the merge conflicts.

estebank · 2024-07-12T17:27:26Z

r=me after rebase

beetrees · 2024-07-12T18:18:46Z

@estebank Rebased. I've also added a normalize-stderr-test to the tests (taken from tests/ui/abi/debug.rs) to prevent the tests being sensitive to preferred alignment differences between targets.

@rustbot label -S-waiting-on-author

bors · 2024-07-31T18:00:34Z

☔ The latest upstream changes (presumably #128435) made this pull request unmergeable. Please resolve the merge conflicts.

beetrees · 2024-08-02T22:37:29Z

I've rebased to fix the merge conflict.

beetrees · 2024-08-17T03:04:09Z

Friendly ping @estebank: I think this can be r+'d now that I've rebased it (I don't have r+ permissions myself).

Dylan-DPC · 2024-08-18T18:22:24Z

@bors r=estebank

bors · 2024-08-18T18:22:27Z

📌 Commit b1493ba has been approved by estebank

It is now in the queue for this repository.

RalfJung · 2024-08-18T20:05:41Z

compiler/rustc_target/src/abi/call/mod.rs

@@ -652,6 +652,11 @@ impl<'a, Ty> ArgAbi<'a, Ty> {
        }
    }

+    /// Same as make_indirect, but doesn't check the current `PassMode`.


Would be good to give some kind of guidance when it is okay to use this. Presumably make_indirect has these checks for a reason?

Since this is only needed for making ignored ZSTs indirect (a specific niche use-case), I've made the method more specific. Since this PR has already merged, I've submitted the changes as #129339.

RalfJung · 2024-08-18T20:07:29Z

compiler/rustc_target/src/abi/call/sparc64.rs

+            // sparc64-unknown-linux-{gnu,musl,uclibc} doesn't ignore ZSTs.
+            if cx.target_spec().os == "linux"
+                && matches!(&*cx.target_spec().env, "gnu" | "musl" | "uclibc")
+                && arg.layout.is_zst()


This treats all ZST the same, no matter their alignment. So for instance [u32; 0] is also a ZST, but it has alignment 4. To only catch types like (), we have is_1zst.

Is catching even aligned ZST the right thing here?

Clang/GCC pass ZSTs indirectly on these targets regardless of alignment.

Move ZST ABI handling to `rustc_target` Currently, target specific handling of ZST function call ABI (specifically passing them indirectly instead of ignoring them) is handled in `rustc_ty_utils`, whereas all other target specific function call ABI handling is located in `rustc_target`. This PR moves the ZST handling to `rustc_target` so that all the target-specific function call ABI handling is in one place. In the process of doing so, this PR fixes rust-lang#125850 by ensuring that ZST arguments are always correctly ignored in the x86-64 `"sysv64"` ABI; any code which would be affected by this fix would have ICEd before this PR. Tests are also added using `#[rustc_abi(debug)]` to ensure this behaviour does not regress. Fixes rust-lang#125850

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#125854 (Move ZST ABI handling to `rustc_target`) - rust-lang#127623 (fix: fs::remove_dir_all: treat internal ENOENT as success) - rust-lang#128084 (Suggest adding Result return type for associated method in E0277.) - rust-lang#128902 (doc: std::env::var: Returns None for names with '=' or NUL byte) - rust-lang#129187 (bootstrap: fix clean's remove_dir_all implementation) - rust-lang#129235 (Check that `#[may_dangle]` is properly applied) - rust-lang#129245 (Typo) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2024-08-18T22:15:44Z

⌛ Testing commit b1493ba with merge d0293c6...

bors · 2024-08-19T00:39:10Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing d0293c6 to master...

rust-timer · 2024-08-19T02:54:03Z

Finished benchmarking commit (d0293c6): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results (secondary 1.2%)

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)	3.9%	[3.7%, 4.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-4.3%, -4.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 1.9%)

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)	1.9%	[1.6%, 2.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 751.033s -> 751.219s (0.02%)
Artifact size: 339.18 MiB -> 339.13 MiB (-0.01%)

…, r=RalfJung Make `ArgAbi::make_indirect_force` more specific As the method is only needed for making ignored ZSTs indirect on some ABIs, rename and add a doc-comment and `self.mode` check to make it harder to accidentally misuse. Addresses review feedback from rust-lang#125854 (comment). r? `@RalfJung`

…, r=RalfJung Make `ArgAbi::make_indirect_force` more specific As the method is only needed for making ignored ZSTs indirect on some ABIs, rename and add a doc-comment and `self.mode` check to make it harder to accidentally misuse. Addresses review feedback from rust-lang#125854 (comment). r? ``@RalfJung``

Rollup merge of rust-lang#129339 - beetrees:make-indirect-from-ignore, r=RalfJung Make `ArgAbi::make_indirect_force` more specific As the method is only needed for making ignored ZSTs indirect on some ABIs, rename and add a doc-comment and `self.mode` check to make it harder to accidentally misuse. Addresses review feedback from rust-lang#125854 (comment). r? ``@RalfJung``

rustbot assigned estebank Jun 1, 2024

estebank added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 12, 2024

beetrees force-pushed the zst-arg-abi branch from 5236426 to 04655f3 Compare July 12, 2024 18:18

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 12, 2024

Move ZST ABI handling to rustc_target

b1493ba

beetrees force-pushed the zst-arg-abi branch from 04655f3 to b1493ba Compare August 2, 2024 22:35

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 Aug 18, 2024

RalfJung reviewed Aug 18, 2024

View reviewed changes

matthiaskrgr mentioned this pull request Aug 18, 2024

Rollup of 7 pull requests #129254

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 19, 2024

bors merged commit d0293c6 into rust-lang:master Aug 19, 2024
7 checks passed

rustbot added this to the 1.82.0 milestone Aug 19, 2024

bors mentioned this pull request Aug 19, 2024

Async drop codegen #123948

Open

beetrees deleted the zst-arg-abi branch August 21, 2024 01:57

beetrees mentioned this pull request Aug 21, 2024

Make ArgAbi::make_indirect_force more specific #129339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move ZST ABI handling to `rustc_target` #125854

Move ZST ABI handling to `rustc_target` #125854

beetrees commented Jun 1, 2024

rustbot commented Jun 1, 2024

rustbot commented Jun 1, 2024

bors commented Jun 23, 2024

estebank commented Jul 12, 2024

beetrees commented Jul 12, 2024 •

edited

Loading

bors commented Jul 31, 2024

beetrees commented Aug 2, 2024

beetrees commented Aug 17, 2024

Dylan-DPC commented Aug 18, 2024

bors commented Aug 18, 2024

RalfJung Aug 18, 2024

beetrees Aug 21, 2024

RalfJung Aug 18, 2024

beetrees Aug 21, 2024

bors commented Aug 18, 2024

bors commented Aug 19, 2024

rust-timer commented Aug 19, 2024

Move ZST ABI handling to rustc_target #125854

Move ZST ABI handling to rustc_target #125854

Conversation

beetrees commented Jun 1, 2024

rustbot commented Jun 1, 2024

rustbot commented Jun 1, 2024

bors commented Jun 23, 2024

estebank commented Jul 12, 2024

beetrees commented Jul 12, 2024 • edited Loading

bors commented Jul 31, 2024

beetrees commented Aug 2, 2024

beetrees commented Aug 17, 2024

Dylan-DPC commented Aug 18, 2024

bors commented Aug 18, 2024

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

bors commented Aug 18, 2024

bors commented Aug 19, 2024

rust-timer commented Aug 19, 2024

Overall result: ✅ improvements - no action needed

Move ZST ABI handling to `rustc_target` #125854

Move ZST ABI handling to `rustc_target` #125854

beetrees commented Jul 12, 2024 •

edited

Loading