Distinguish repr(C) ZSTs from others in ABI computation#156112
Distinguish repr(C) ZSTs from others in ABI computation#156112Jules-Bertholet wants to merge 1 commit intorust-lang:mainfrom
repr(C) ZSTs from others in ABI computation#156112Conversation
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
I don't think this fixes that issue. We need #155299 for that (and a wording change to the ABI docs). This new logic can be entirely removed again once we compute the right layout for zero-element Let's see if this new field shows up in perf. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Distinguish `repr(C)` ZSTs from others in ABI computation
f72b6fa to
9a2f4c8
Compare
| // CHECK: define win64cc void @pass_zst_win64(ptr {{[^,]*}}) | ||
| // CHECK: define win64cc void @pass_rust_zst_win64() | ||
| #[no_mangle] | ||
| extern "win64" fn pass_rust_zst_win64(_: ()) {} |
There was a problem hiding this comment.
This will be interesting... I am not sure how to make sense of this ABI on non-MSVC targets. Given that the type layout itself is "wrong", how could one possibly actually call such a function...?
There was a problem hiding this comment.
You could call such a function from C with a signature of void pass_rust_zst_win64(). However, the Rust signature trips the improper_ctypes lint, so that's not something we are committing to.
There was a problem hiding this comment.
Currently, you could not. We don't allow any arguments to be omitted at the moment.
There was a problem hiding this comment.
Also note that I am talking about the "win64" ABI in general, not just this particular function -- I should have been more clear about that.
A "win64" function call on a windows-gnu or Linux target that involves a zero-field struct (or struct whose only field is a 0-element array) might just produce nonsense, right? We'd need repr(win64) as well.
9a2f4c8 to
8a01401
Compare
| size, | ||
| max_repr_align: None, | ||
| unadjusted_abi_align: element.align.abi, | ||
| repr_c: false, |
There was a problem hiding this comment.
This could be:
| repr_c: false, | |
| repr_c: element.repr_c, |
Or even:
| repr_c: false, | |
| repr_c: true, |
But I don't want this PR to get bogged down in controversy, so let's leave it alone for now (unless there is lang team consensus).
If we do change this, the documentation in library/core/src/primitive_docs.rs will need to change as well.
There was a problem hiding this comment.
It's unclear to me what the consequence of either of the tree possible options here is.
There was a problem hiding this comment.
false: all ZST arrays have trivial ABI for all element typeselement.repr_c: ZST arrays are passed like arepr(C)ZST if the element type isrepr(C)true: ZST arrays are passed like arepr(C)ZST for all element types
There was a problem hiding this comment.
This codepath affects all arrays, not just ZST, right? I guess your point is that for non-ZST repr_c (currently) doesn't make a difference?
ZST arrays are passed like a repr(C) ZST if the element type is repr(C)
That's what I expected based on the description.
... now I can't find it any more, did you change the doc comment for this field? Propagating this through arrays made sense to me at first sight but I did not think deeply about it.
Also, is it correct to say "passed like a repr(C) ZST"? For all I know, C might have different zero-sized types that are passed differently.
There was a problem hiding this comment.
did you change the doc comment for this field?
Yes.
Also, is it correct to say "passed like a repr(C) ZST"
"Passed like a repr(C) ZST with the same alignment as the element type".
For all I know, C might have different zero-sized types that are passed differently.
Standard C doesn't have zero-sized types at all. But yes, widespread compiler extensions do make this happen: https://godbolt.org/z/1xWqqj3qv
Currently, Rust can't match the flexible-array-member ABI. This PR leaves that unchanged.
There was a problem hiding this comment.
Ah, it looks like flexible array members having a different ABI is just a quirk of Clang, not replicated by GCC: https://godbolt.org/z/eWKazGxxW
Filed an issue with LLVM: llvm/llvm-project#195572
There was a problem hiding this comment.
This makes me question again whether we should support any kind of ZST in our C interop, given that those types do not exist in standard C, and C compilers cannot even agree among themselves...
That's part of this PR.
No, because these structs should remain zero-sized on many targets, including ones where they are passed by pointer. |
a74ed3a to
e2ee4b8
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (167ff94): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.1%, secondary 2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 494.817s -> 493.602s (-0.25%) |
If you are putting non-trivial implementation changes into the same PR as insta-stable docs updates (that will require t-lang FCP), you're making this PR a lot more difficult to work with. It'd be a good idea to split this up. |
View all comments
Some C ABIs pass and return ZSTs by pointer. But
()should never be returned by pointer, as it must matchvoid. To fix this, distinguishrepr(C)ZSTs (andrepr(transparent)wrappers around them) from other ZSTs in ABI computation.Changes documented ABI guarantees (
library/core/src/primitive_docs.rs), therefore:@rustbot label T-lang needs-fcp
Non-Windows ABIs that pass ZSTs indirectly (e.g. Linux
powerpc,sparc64,s390x) need more investigation and tests to make sure we always do the right thing. (This PR shouldn't make them any more broken than they already are, but it might not fully fix them.)Fixes rust-lang/unsafe-code-guidelines#552; see also #78586, #155299.
cc @RalfJung