-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Distinguish repr(C) ZSTs from others in ABI compatibility rules
#157973
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.
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
31d03e7
34de188
41e5d13
a01211b
a9bed71
3e86bb0
fd2f2de
22d1733
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1841,9 +1841,19 @@ mod prim_ref {} | |||||||||
| /// call will be valid ABI-wise. The callee receives the result of transmuting the function pointer | ||||||||||
| /// from `fn()` to `fn(i32)`; that transmutation is itself a well-defined operation, it's just | ||||||||||
| /// almost certainly UB to later call that function pointer.) | ||||||||||
| /// - Any two types with size 0 and alignment 1 are ABI-compatible. | ||||||||||
| /// - A `repr(transparent)` type `T` is ABI-compatible with its unique non-trivial field, i.e., the | ||||||||||
| /// unique field that doesn't have size 0 and alignment 1 (if there is such a field). | ||||||||||
| /// - Any two types fulfilling all the following conditions are ABI-compatible; | ||||||||||
| /// such types are said to have "trivial ABI": | ||||||||||
|
Comment on lines
+1844
to
+1845
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| /// - It has size 0. | ||||||||||
| /// - It has alignment 1. | ||||||||||
| /// - One of the following apply: | ||||||||||
| /// - It is a `repr(Rust)` (implicitly or explicitly, possibly with additional modifiers such as `packed`) `struct`, `enum`, `union` (regardless of its fields). | ||||||||||
| /// - It is a [tuple][prim_tuple] (regardless of its fields, and including [`()`][prim_unit]). | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Less parens, please.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rephrasing without the specific note about parens: I think reminding people that 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. |
||||||||||
| /// - It is a `repr(transparent)` `struct`, `enum`, or `union`, and all fields have trivial ABI. | ||||||||||
| /// - It is an array, and its element type has trivial ABI. (This requirement applies even to arrays of length 0.) | ||||||||||
|
workingjubilee marked this conversation as resolved.
|
||||||||||
| /// - It is [the never type `!`][prim_never]. | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we completely sure that we want
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. People tell me that that's fine, and I don't know enough about what ABIs do here to be able to point at potential problems. Cc @workingjubilee @chorman0773 @CAD97 Note that if we don't want this, we'll need to change more than just remove this last item in the list. For instance, we'd also have to declare
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware of any problems with doing it ... there's no separate signature for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One could imagine a custom rust ABI that omits the information required to return from the function if we know it anyway doesn't return. OTOH at least in LLVM the concept of an uninhabited type does not exist and
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notably, you can call a _Noreturn/[[noreturn]] function via a pointer to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At worst, if we ever want to do FFI with some hypothetical language whose ABI does make a distinction, we can have a separate |
||||||||||
| /// - It is a function item type or closure type. | ||||||||||
|
Comment on lines
+1852
to
+1854
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See discussion thread at #157973 (comment). The "and its element type has trivial ABI" requirement should exclude array types with C equivalents.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hrm, I guess you are right that the element type having a trivial ABI requirement is probably sufficient. |
||||||||||
| /// - A `repr(transparent)` type is ABI-compatible with its unique field that does not have trivial ABI | ||||||||||
| /// (as defined above), if such a field exists. | ||||||||||
|
Jules-Bertholet marked this conversation as resolved.
Outdated
|
||||||||||
| /// - `i32` is ABI-compatible with `NonZero<i32>`, and similar for all other integer types. | ||||||||||
| /// - If `T` is guaranteed to be subject to the [null pointer | ||||||||||
| /// optimization](option/index.html#representation), and `E` is an enum satisfying the following | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this.
Correct.