Add #[rustc_pass_indirectly_in_non_rustic_abis]

[beetrees]

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]

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]

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

[RalfJung]

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

[beetrees]

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.

[RalfJung]

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.

[tgross35]

Not the most robust idea, but we there could be some kind of ICE-causing bomb that gets defused when checking an arg's pass_indirectly_in_non_rustic_abis and ignored if there are no args. This at least makes sure that new targets don't get very far if they miss this important detail.

Or a codegen test that gets run on all targets?

[RalfJung]

We have some code doing sanity checks on the ABI after it got computed. We could probably add an assertion there.

rust/compiler/rustc_ty_utils/src/abi.rs

Lines 368 to 372 in d71ed8d

	fn fn_abi_sanity_check<'tcx>(
	cx: &LayoutCx<'tcx>,
	fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
	spec_abi: ExternAbi,
	) {

[bjorn3]

The indirect_argument function would not be codegened if it doesn't get called, so that wouldn't trigger the ABI check. It should probably just be a ui test. This won't run in CI for tier 3 targets, but the worst that can happen is that you get an ICE, not a silent miscompilation.

[RalfJung]

How do we make sure that triggers for every ABI though?

It's an unstable attribute, so worst case we get an ICE when building libcore. That seems fine.

[folkertdev]

The indirect_argument function would not be codegened if it doesn't get called, so that wouldn't trigger the ABI check.

what if we use a const fn? Maybe using some const _: () = assert!(/* ... */), would that work?

[folkertdev]

Using const does work, I added that and a fix for transparent wrappers ignoring the attribute at master...folkertdev:rust:pass-indirectly-attr-updates

@beetrees feel free to steal or chery-pick from that. I'd also happily force-push to this branch if you don't have time/interest. (this is on the critical path for c-variadics now that the error messages are in a good state, and I'd hate to waste the reviewer momentum).

[beetrees]

I've cherry-picked your patch for this.

[RalfJung]

@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]

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

[bors]

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

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann

[SparrowLii]

r? @joshtriplett :)

[rustbot]

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

[folkertdev]

Besides Jubilee, who is not available, the only other person I can think of to review ABI code is

r? @bjorn3

[bors]

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

[bors]

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

[joshtriplett]

Broadly speaking, this seems like a reasonable way to tag types for use with this special calling convention.

I can't speak to the maintainability of this approach, but generally speaking, I'm not sure it's possible to handle va_args any easier than this, for any ABIs where it's a special case. I'd suggest, in general, that when you add this attribute to a type, there should be copious cross-references between the type, the ABI tests that confirm it's passed correctly, and the compiler sources implementing this attribute, so that people who are adding a new ABI know to carefully add tests and check the passing of va_args. (And, hopefully, not duplicate the weird mistakes of existing architectures.)

[folkertdev]

Some reference to array-to-pointer decay might be useful here to motivate why this function exists?

[beetrees]

I've added a paragraph with a brief explanation.

[folkertdev]

so right now the type is only passed indirectly if the base type is passed indirectly. This just confused me for a while, I think this should also work:

#[repr(C)]
struct Vanilla(i32);

#[repr(transparent)]
#[rustc_pass_indirectly_in_non_rustic_abis]
struct Indirectly(Vanilla);

I.e. if any of the transparent wrappers is rustc_pass_indirectly_in_non_rustic_abis then the whole type should be passed indirectly.

[beetrees]

I did consider whether this should just be an error, as the primary purpose of repr(transparent) (as compared to repr(C)) is that a repr(transparent) struct has the same ABI as the non-ZST field, but since rustc_pass_indirectly_in_non_rustic_abis only affects non-Rustic ABIs the repr(transparent) would still have some affect, and the behaviour you suggest would seem to be the most consistent. I've cherry-picked your patch for this.

[RalfJung]

I think an error makes most sense -- the behavior you now implemented seems to violate the promise that repr(transparent) wrappers have the same ABI as their underlying type on all ABIs.

[folkertdev]

Hmm yeah, looking at this again, given e.g. https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7e1399219e6854e5ca5e5765cb8e6993 an error is probably better. #[rustc_pass_indirectly_in_non_rustic_abis] does feel morally like a repr hint.

[RalfJung]

Broadly speaking, this seems like a reasonable way to tag types for use with this special calling convention.

I can't speak to the maintainability of this approach, but generally speaking, I'm not sure it's possible to handle va_args any easier than this, for any ABIs where it's a special case. I'd suggest, in general, that when you add this attribute to a type, there should be copious cross-references between the type, the ABI tests that confirm it's passed correctly, and the compiler sources implementing this attribute, so that people who are adding a new ABI know to carefully add tests and check the passing of va_args. (And, hopefully, not duplicate the weird mistakes of existing architectures.)

Correct use of the attribute depends not just on the type but also its surrounding conventions -- for instance, it is already UB in C to even look at a va_args after it has been passed to another function, if I understood correctly. That's crucial because a Rust "pass_indirectly" type still passes a copy of the argument value to the callee, whereas in C it passes effectively an implicit reference -- if the callee mutates the argument, a C caller will see the mutations, a Rust caller will not.

[bors]

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

[folkertdev]

@beetrees gave me permission (on zulip) to help get this PR over the finish line. I've rebased made it so that we now emit an error when #[rustc_pass_indirectly_in_non_rustic_abis] is used alongside with #[repr(transparent)].

@rustbot ready

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[bjorn3]

Unsized arguments are unstable and shouldn't be used outside of the rust ABI. If you try to use them with the C ABI we are forced to invent an ABI for them. Maybe we should just error when trying to use an unsized type with a non-rust ABI?

[folkertdev]

Can we add the error in a separate PR? Lots of ABIs bail with an early return currently.

[RalfJung]

We can't panic here, that would lead to ICEs. And we can't return an error, there's no Result.

However, the ABI sanity check could reject this for non-Rust ABIs. It may already do that, I am not sure.

[folkertdev]

You get a bunch of warnings if you try, but no error https://godbolt.org/z/haPra1Ecf

warning: the feature `unsized_fn_params` is internal to the compiler or standard library
 --> <source>:1:12
  |
1 | #![feature(unsized_fn_params)]
  |            ^^^^^^^^^^^^^^^^^
  |
  = note: using it is strongly discouraged
  = note: `#[warn(internal_features)]` on by default

warning: `extern` fn uses type `[u8]`, which is not FFI-safe
  --> <source>:11:22
   |
11 | extern "C" fn foo(x: X) {}
   |                      ^ not FFI-safe
   |
   = help: consider using a raw pointer instead
   = note: slices have no C equivalent
   = note: `#[warn(improper_ctypes_definitions)]` on by default

the ABI that is used is apparently to pass a pointer and a length https://godbolt.org/z/3WWoccvr4

[bjorn3]

We can't panic here, that would lead to ICEs. And we can't return an error, there's no Result.

The check can be in adjust_for_foreign_abi. This function doesn't return a Result, but the caller of the caller of this function does already return Result<&'tcx FnAbi, &'tcx FnAbiError>, so it should be possible to add another variant to FnAbiError for this.

[RalfJung]

Currently, the one ABI check we do have is entirely separate, it's in compiler/rustc_monomorphize/src/mono_checks/abi_check.rs. That has the advantage of being able to point at where the ABI comes up.

It would be odd to have two entirely parallel systems for rejecting types that are invalid for an ABI.