Repository navigation
Add infrastructure to "compute the ABI of a Rust type, described as a C type" #672
Description
Proposal
Platform ABIs are typically defined in terms of C types. When adding support for a platform in Rust, the ABI adjustment code needs to figure out how to map Rust types into such a description. For many ABIs this is fairly simple, but some ABIs do complicated type-directed traversals, and that easily leads to issues:
extern "C" fnon sparc64 targets does not respect repr(transparent) rust#115336extern "C" fnon mips64 targets does not respect repr(transparent) rust#115404- riscv64gc-unknown-linux-gnu
extern "C"ABI violates repr(transparent) on unions rust#115481 - loongarch64-unknown-linux-gnu
extern "C"ABI violates repr(transparent) on unions rust#115509 - The sparc64, riscv64, loongarch64
extern "C" fnABIs are all wrong when aligned/packed structs are involved rust#115609 extern "C" fnABI on aarch64 violates repr(transparent) rust#115664
Literally every single ABI that we have implemented and that does something non-trivial got it wrong! This is clearly a systematic issue. I don't have high confidence that after fixing the above bugs, those targets are "good" -- these ABIs tend to special-case specific arcane conditions and it is really hard to be sure that tests like rust-lang/rust#115372 cover all of them.
So I propose that we systematically attack this issue, by introducing a type for "C ABI descriptions" that roughly looks like this (this is a first draft, this will almost certainly need to be extended/adjusted):
/// The C type that describes the ABI of a Rust type.
enum CAbiType {
Primitive(abi::Primitive),
Struct(Vec<CAbiType>),
PackedStruct(Vec<CAbiType>),
Union(Vec<CAbiType>),
Array { elem: Box<CAbiType>, len: usize },
/// A kind of type that doesn't exist in C such as a zero-sized type, an unsized type,
/// or some enums. This can never exist as the field of a struct/union/array,
/// it is used only as a top-level ABI type.
/// (This is the most likely place for changes to be needed.)
Opaque,
}We could then have a single shared function that computes the CAbiType for a Rust TyAndLayout, and all targets that need to make subtle adjustments should be ported to use that shared function instead of traversing the type/layout themselves. This shared function would, in particular, know to skip repr(transparent) wrappers.
Alternative
Instead of making this a completely new type that is derived from TyAndLayout, we could also alter the existing Abi type to be able to represent C structs, unions, and arrays. The intent of that Abi type was to capture everything relevant for the ABI, but unfortunately it turns out that the current Abi::Aggregate case just loses too much information.
Mentors or Reviewers
I won't have time to implement this or to serve as the sole mentor, but I'd be happy to co-mentor with someone else.
Process
The main points of the Major Change Process are as follows:
- File an issue describing the proposal.
- A compiler team member or contributor who is knowledgeable in the area can second by writing
@rustbot second.- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a
-C flag, then full team check-off is required. - Compiler team members can initiate a check-off via
@rfcbot fcp mergeon either the MCP or the PR.
- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a
- Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.
You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
Activity
rustbot commentedon Sep 6, 2023
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
cc @rust-lang/compiler @rust-lang/compiler-contributors
[-]Add infrastructure to "compute the ABI of a Rust type, describes as a C type"[/-][+]Add infrastructure to "compute the ABI of a Rust type, described as a C type"[/+]RalfJung commentedon Sep 25, 2023
I think the entire ABI/
PassModesystem and in particularCastTywill actually need a larger refactoring, otherwise I don't see how we can fix issues such as rust-lang/rust#115609.oli-obk commentedon Sep 26, 2023
@rustbot second
RalfJung commentedon Sep 26, 2023
Which of the various variants we discussed does this second? In particular we discussed that (a variant of)
CAbiTypeabove might entirely replace the currentAbitype.Or do we leave that to experimentation during the implementation?
oli-obk commentedon Sep 26, 2023
Incremental progress towards (and including) replacing
Abi. As long as a compiler team member is alright with all the steps, this MCP is the overarching plan, but may be halted before actually replacingAbiif new insights during the implementation speak against itRalfJung commentedon Oct 6, 2023
@eddyb mentions that another shape this could take is to have a method on
TyAndLayoutthat produces the "list of ABI fields" of this type -- i.e., the list of fields that this type has for the purpose of call ABI (which is separate from the list of declared fields, found inFieldsShape, due to things like newtypes disappearing in the ABI).ABI adjustments could then even be given a wrapper around
TyAndLayoutthat doesn't give access to anything but such ABI-stable information.RalfJung commentedon Oct 7, 2023
After more discussion with @eddyb we came to no final conclusion but exchanged some promising ideas:
layout.abifield is poorly named. It wasn't even intended to be about the function call ABI, it was intended to be about how such values are treated by codegen backends internally (as SSA values vs in-memory) -- entirely an optimization. Proposed new name:storage: Storage, orStorageKindorStorageClass; explicitly document that it is merely an optimization; maybe rename theAggregatevariant toMemory. Maybe have aZstvariant for "no storage needed"? Maybe that could be the same variant asUninhabited? (thoughis_uninhabitedhas to still work, but I don't see why that would be a "storage"/"ABI" property)storage. In particular, make one-field#[repr(C)]useStorage::Scalar! Instead ABI is provided via two functions (or an equivalent API):get_abi_kindwhich can returnScalar,Vector,Aggregate(maybeAggregate { kind: Array | Struct | PackedStruct | Union }?);get_abi_fieldswhich only works onAggregates and returns the fields of this struct as far as ABI is concerned (different fromFieldsShapesince it will skip newtypes and maybe some ZST).RalfJung commentedon Nov 21, 2023
When we are refactoring the ABI adjustments anyway, it might make sense to make the entire thing properly declarative: currently we are setting a "default"
PassModefor all types initially, and then we have various "adjustment" passes that patch things up. That's very fragile (causing issues like rust-lang/rust#115666 and rust-lang/rust#118124) and it becomes hard to tell what the effective ABI of a given function even is. It is very easy to just forget to adjust some types, leading to code that seemingly works but has the wrong ABI. On wasm this led to us now having a de-facto ABI that nobody ever wanted but that is hard to change. It'd be better if the ABI "adjustments" had to just declare explicitly for each type how it should be passed.apiraino commentedon Dec 19, 2023
@rustbot label -final-comment-period +major-change-accepted
RalfJung commentedon Dec 21, 2023
Opened an issue to track this: rust-lang/rust#119183