Skip to content

Add #[rustc_pass_indirectly_in_non_rustic_abis] #144529

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add #[rustc_pass_indirectly_in_non_rustic_abis] #144529

wants to merge 1 commit into from

Conversation

beetrees
Copy link
Contributor

@beetrees beetrees commented Jul 27, 2025 •
edited by rustbot
Loading

This PR adds an internal #[rustc_pass_indirectly_in_non_rustic_abis] attribute that can be applied to structs. Structs marked with this attribute will always be passed using PassMode::Indirect { on_stack: false, .. } when being passed by value to functions with non-Rustic calling conventions. This is needed by #141980; see that PR for further details.

cc @joshtriplett

@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2025

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

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 Jul 27, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2025

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

@beetrees beetrees mentioned this pull request Jul 27, 2025
Open
RalfJung
RalfJung reviewed Jul 27, 2025
View reviewed changes
compiler/rustc_target/src/callconv/amdgpu.rs
where
Ty: TyAbiInterface<'a, C> + Copy,
C: HasDataLayout,
{
if arg.layout.pass_indirectly_in_non_rustic_abis(cx) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we rely on every single callconv getting this right, we're toast. It's way too easy to forget this somewhere.

Is there some way we can do this centrally for all ABIs?
For instance, we could apply this logic after the target-specific ABI stuff has been done.
Cc @workingjubilee

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The individual calling conventions sometimes still need to be aware of the parameters, to update the number of remaining general-purpose registers, and the current design of the calling convention code makes it hard to abstract this. Separately from this PR, I've been planning to refactor the calling convention handling a bit as even without this change there's a lot of code duplication already (all the compute_abi_info functions are essentially variants of the same function with calls to classify_arg and classify_ret); this refactoring should make it possible to do this in a more centralised way.

For now, this PR previously had a cfg!(debug_assertions)-guarded check at the end of adjust_for_foreign_abi in callconv/mod.rs that asserts that individual calling convention correctly set all the #[rustc_pass_indirectly_in_non_rustic_abis] arguments to be passed indirectly. I've updated the check so it now always run rather than just running when debug_assetions are enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The individual calling conventions sometimes still need to be aware of the parameters, to update the number of remaining general-purpose registers,

Urgh, right, I forgot we need to care about low-level nonsense like that here. :/

Regarding refactoring the ABI code, also see #119183. I think @workingjubilee also has some thoughts in that direction. I'm happy to discuss design options and provide feedback.

@beetrees beetrees force-pushed the pass-indirectly-attr branch from c0357c1 to 61196cb Compare July 27, 2025 17:23
@RalfJung
Copy link
Member

@jdonszelmann could you have a look at the attribute code here? This is my first time actually seeing the new infrastructure so I can't say if the way it is used here is correct.

RalfJung
RalfJung reviewed Jul 29, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler part LGTM apart from these comments, but I can't really review these ABI adjustments.

compiler/rustc_target/src/callconv/riscv.rs
@@ -293,6 +293,10 @@ fn classify_arg<'a, Ty, C>(
// Not touching this...
return;
}
if arg.layout.pass_indirectly_in_non_rustic_abis(cx) {
arg.make_indirect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually loongarch and riscv do exactly the same thing. Why does this one here not update avail_gprs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I've also gone through the other calling conventions and checked that state was being updated correctly.

compiler/rustc_target/src/callconv/x86_64.rs
@@ -186,6 +186,11 @@ where
// Not touching this...
return;
}
if is_arg && arg.layout.pass_indirectly_in_non_rustic_abis(cx) {
int_regs = int_regs.saturating_sub(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very odd that this would happen here but not for unsized arguments... it's pre-existing, but can you add a FIXME? (Also for the other files with the same pattern)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXMEs added.

@beetrees beetrees force-pushed the pass-indirectly-attr branch from 61196cb to ed746a3 Compare July 29, 2025 15:27
@bors
Copy link
Collaborator

bors commented Jul 31, 2025

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

@beetrees beetrees force-pushed the pass-indirectly-attr branch from ed746a3 to d91160a Compare August 1, 2025 01:43
@rustbot
Copy link
Collaborator

rustbot commented Aug 1, 2025

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@beetrees beetrees force-pushed the pass-indirectly-attr branch from d91160a to 0401bf3 Compare August 1, 2025 01:59
@SparrowLii
Copy link
Member

r? @joshtriplett :)

@rustbot rustbot assigned joshtriplett and unassigned SparrowLii Aug 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2025

joshtriplett is not on the review rotation at the moment.
They may take a while to respond.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@joshtriplett joshtriplett

Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@beetrees @rustbot @RalfJung @bors @rust-log-analyzer @SparrowLii @joshtriplett