Distinguish repr(C) ZSTs from others in ABI compatibility rules#157973
Distinguish repr(C) ZSTs from others in ABI compatibility rules#157973Jules-Bertholet wants to merge 8 commits into
repr(C) ZSTs from others in ABI compatibility rules#157973Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| /// - Alignment 1 | ||
| /// - Not `repr(C)` | ||
| /// - Not a `repr(transparent)` wrapper around a type that fails to satisfy these conditions | ||
| /// - Not an array whose element type fails to satisfy these conditions |
There was a problem hiding this comment.
We exempt arrays for 2 reasons:
- A future version of C, or some other language we would like to do FFI with, might allow passing arrays to functions directly by value, in a way that would conflict with these guarantees
- It would be nice to also use "trivial ABI" in the specification of
repr(C), and we need this clause for that. See discussion at repr(ordered_fields) rfcs#3845 (comment)
We could also simplify this clause by saying merely:
| /// - Not an array whose element type fails to satisfy these conditions | |
| /// - Not an array |
The downside would be a larger breaking change.
Note that repr(transparent) will need to be adjusted to account for this change (by rejecting non-trivial arrays as "additional" fields).
There was a problem hiding this comment.
I'm fine with saying that "all arrays with trivial-ABI element type have trivial ABI".
However, even that less-breaking version breaks ghost again, right? Similar to dtolnay/ghost#41. ghost uses a zero-length array of *const T, which definitely does not have trivial ABI. I don't know why they do that...
EDIT: Ah, ghost is saved by having a repr(Rust) type around the array, under the rules discussed here.
There was a problem hiding this comment.
Some C ABIs pass and return ZSTs by pointer. But () should never be returned by pointer, as it must match void. To account for this, we have to weaken the present guarantee of "any two types with size 0 and alignment 1 are ABI-compatible" to exclude repr(C).
I think the implication here is that () is repr(C)? Am I reading that right? Where do we make that guarantee? Or is the thinking that the language here would make () and #[repr(C)] struct Foo; not ABI compatible?
One callout is that ZSTs aren't (I think?) standardized -- C and C++ without extensions both require types to be non-ZST if I remember right (e.g., see https://stackoverflow.com/a/2632075). Maybe that has changed since then though?
It seems like at minimum, it would be nice to avoid weakening this guarantee for Rust ABI even if we do so for C ABIs as a result of the weird platforms.
There was a problem hiding this comment.
Or is the thinking that the language here would make
()and#[repr(C)] struct Foo;not ABI compatible?
Yes, this.
One callout is that ZSTs aren't (I think?) standardized
Correct.
| /// - Any two types fulfilling all the following conditions are ABI-compatible; | ||
| /// such types are said to have "trivial ABI": |
There was a problem hiding this comment.
| /// - Any two types fulfilling all the following conditions are ABI-compatible; | |
| /// such types are said to have "trivial ABI": | |
| /// - Any two types with "trivial ABI" are ABI-compatible. | |
| /// A type has trivial ABI if is satisfies all of the following: |
|
@rust-lang/opsem @rust-lang/lang what do you think? I think we do have to take back a bit of what we promised here, since we did promise too much. The current docs say:
This is plain wrong for // This is returned by-ptr.
#[repr(C)]
struct T1 {}
// This matches a C function returning `void`.
type T2 = ();
So the proposal is to restrict the above rule as follows:
Under the old rules, we effectively said "All 1-ZST have trivial ABI". Now we have some further restrictions. I think the only types that used to have trivial ABI but don't any more are Note that we also have to adjust the logic for
I cratered the 2nd part and found 0 regressions. I don't know how to measure the fallout from the 1st breakage. We should implement this in Miri, but even that will just give us a very incomplete partial picture. That said, given that the current docs are wrong, we have to do something. The only actually open question (in my eyes) is how bespoke we want the rules to be with the aim of breaking less code.
|
Co-authored-by: Brian Smith <brian@briansmith.org> Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: Ralf Jung <post@ralfj.de>
|
Does the Reference have any discussion of ABI compatibility currently? Hard to make a PR with a diff to something that's not even in the Reference yet. |
| /// - It is an array, and its element type has trivial ABI. (This requirement applies even to arrays of length 0.) | ||
| /// - It is [the never type `!`][prim_never]. | ||
| /// - It is a function item type or closure type. |
There was a problem hiding this comment.
I think this change is reasonable. We have to change these rules because they're just wrong and this is unfixable. The rules as stated seem like a reasonable way to fix it.
My main question would be: Why include arrays? I get a bit worried about that one because array arguments have weird properties in C. We don't want to run into another case in the future where Rust cannot represent certain kinds of C ABIs involving arrays.
There was a problem hiding this comment.
See discussion thread at #157973 (comment). The "and its element type has trivial ABI" requirement should exclude array types with C equivalents.
There was a problem hiding this comment.
Hrm, I guess you are right that the element type having a trivial ABI requirement is probably sufficient.
|
Let's just ask opsem officially. :) |
This comment was marked as outdated.
This comment was marked as outdated.
|
Let's poll both in parallel. @rfcbot cancel |
|
@traviscross proposal cancelled. |
|
@rfcbot fcp merge lang,opsem |
|
@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
|
I still personally think that, given a time machine, using anything in That said, I do recognize practicality. Lacking a time machine, I agree that this is the correct thing to do for Rust in practice. @rustbot reviewed |
The Reference actually delegates explicitly to these stdlib docs: https://doc.rust-lang.org/reference/type-layout.html#r-layout.guarantees After this PR is merged, we'll need to do the follow-up work of adjusting |
Isn't this PR introducing the concept of "trivial ABI"? It's worded like it does. I would like to read the rendered documentation for this file on the website but I can't find what page |
As many pages, but the relevant one for this PR is https://doc.rust-lang.org/core/primitive.fn.html#abi-compatibility I think that by "current rules", Ralf was referring to the current state of this PR. |
Yes, sorry for the confusion. The PR was changed a bit in the beginning. I have clarified my comment. |
|
Is it correct that the only 1-ZST that don't have trivial ABI under this are things like (I think our rules should be stated the way they are, positively, but for understanding what this changes compared to the status quo it is useful to also think of what the exceptions are.) |
|
I can't think of anything else. |
| /// - It has size 0. | ||
| /// - It has alignment 1. | ||
| /// - One of the following apply: | ||
| /// - It is a `repr(Rust)` (implicitly or explicitly, possibly with additional flags such as `packed`) `struct`, `enum`, `union` (regardless of its fields). |
There was a problem hiding this comment.
I tried to read this while asking myself why repr(packed) doesn't matter here. I know, and you know, but does every reader know? Why is it just an "additional flag"? I am inclined to think this is currently confusingly worded.
To start, it uses the term "flag", which feels a bit out-of-left field, like you're thinking about how the compiler reduces this to bitflags, but elsewhere on this page the only use of "flags" is in reference to compiler flags. We instead should talk, here, about the source syntax alone, attributes.
I think something like this would be a wording that makes it clearer that some attributes do conflict with repr(Rust), and some do not, the latter of which you referred to as "additional". If this seems like a bit much then maybe the text should just say "implicitly or explicitly" and move on, possibly with a reference to an explanation elsewhere or pushing the warning text into a later example.
| /// - It is a `repr(Rust)` (implicitly or explicitly, possibly with additional flags such as `packed`) `struct`, `enum`, `union` (regardless of its fields). | |
| /// - It is a `repr(Rust)` `struct`, `enum`, or `union`, regardless of its fields. This includes when a type is implicitly assigned `repr(Rust)` because no explicit `repr` attribute would conflict, such as when `repr(packed)` is treated as `repr(Rust, packed)`, as opposed to `repr(C, packed)`. |
There was a problem hiding this comment.
Also, the parentheses are unnecessary if we use a sentence structure that doesn't need them.
There was a problem hiding this comment.
To start, it uses the term "flag", which feels a bit out-of-left field
Good point. Changed to "modifier", which is what the Reference uses.
Also, the parentheses are unnecessary if we use a sentence structure that doesn't need them.
I use parentheses to enclose clarifications that don't change the meaning (as opposed to normative statements). That's a valuable distinction, and parentheses are the punctuation best suited to express it.
There was a problem hiding this comment.
Hmm. I honestly don't really assign any such extremely specific intent to parentheses because they are used to provide "additional explanation" in various contexts and whether or not that is meaning-changing is ambiguous.
Footnotes are something that one might imagine to be non-normative and merely clarifying, yes? But the entire C programming ecosystem rests on a footnote's "clarification" being treated as normative.
I will give it another read to see if it makes sense with that, though.
There was a problem hiding this comment.
So, regarding parens: Mostly, it feels confusing to read something that is constantly interrupting itself. I actually prefer the next line because it finishes a thought in one sentence, then has another sentence in parentheses. While I felt that made the parentheses slightly redundant, if you think they're a useful stylistic qualifier between normative/clarifying, then that's fine. But if you can find a variant of this line that reads more like that one, it would be appreciated.
| /// - It has alignment 1. | ||
| /// - One of the following apply: | ||
| /// - It is a `repr(Rust)` (implicitly or explicitly, possibly with additional flags such as `packed`) `struct`, `enum`, `union` (regardless of its fields). | ||
| /// - It is a [tuple][prim_tuple] (regardless of its fields, and including [`()`][prim_unit]). |
There was a problem hiding this comment.
Less parens, please.
| /// - It is a [tuple][prim_tuple] (regardless of its fields, and including [`()`][prim_unit]). | |
| /// - It is a [tuple][prim_tuple], including [`()`][prim_unit], regardless of its fields. |
There was a problem hiding this comment.
Rephrasing without the specific note about parens: I think reminding people that () is technically considered a tuple here is more important, thus should come first.
I initially considered suggesting just removing every "regardless of..." case and letting the absence of qualifier speak for itself. There are good reasons to not, however.
| /// `packed` representation modifiers. | ||
| /// - The enum `E` has exactly two variants. | ||
| /// - One variant has exactly one field, of type `T`. | ||
| /// - All fields of the other variant are zero-sized with 1-byte alignment. |
There was a problem hiding this comment.
Here as well, "have trivial ABI" instead of size/align
There was a problem hiding this comment.
In don't think so. This is about a repr(Rust) enum, we fully make the rules there, and we can totally say that [u8; 0] will be ignored without having to worry about C compatibility.
Co-authored-by: Ralf Jung <post@ralfj.de>
|
View all comments
(Split out from compiler implementation in #156112)
Some C ABIs pass and return ZSTs by pointer. But
()should never be returned by pointer, as it must matchvoid. To account for this, we have to weaken the present guarantee of "any two types with size 0 and alignment 1 are ABI-compatible" to excluderepr(C).t-lang nomination summary comment
Fixes rust-lang/unsafe-code-guidelines#552; see also #78586, #155299.
Also related to #155984.
@rustbot label T-lang A-ABI needs-fcp