repr(transparent): don't consider most length-0 arrays trivial#155984
repr(transparent): don't consider most length-0 arrays trivial#155984Jules-Bertholet wants to merge 3 commits into
repr(transparent): don't consider most length-0 arrays trivial#155984Conversation
|
rustbot has assigned @dingxiangfei2009. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
| if elem_trivial { | ||
| check_unsuited(tcx, typing_env, *elem_ty) |
There was a problem hiding this comment.
Why do you allow any arrays here? Seems easier to just reject them all?
I'd be surprised if there is much/any use of arrays as "trivial" types in repr(transparent).
There was a problem hiding this comment.
I think it's conceptually nice to treat "array with element type T" the same as "struct with field of type T". And it turns out that we need some non-trivial complexity anyway, to support the ghost crate.
There was a problem hiding this comment.
to support the ghost crate.
That seems like a different problem. I'm not even sure we should.
There was a problem hiding this comment.
To clarify: this PR doesn't fix using types from ghost in repr(transparent). As of https://github.com/dtolnay/ghost/releases/tag/0.1.21, ghost is already fixed. The packed check is so that we don't break it again in a different and harder-to-fix way.
There was a problem hiding this comment.
Ah, do they also use arrays? That is kind of annoying...
FWIW based on the previous crater run I think there are no uses of ghost on crates.io that require it to be "trivial" for repr(transparent).
That said, I think I could live with a more principled solution to this: stop checking anything ABI-related (repr(C), array) once we are inside a repr(Rust) type. Your PR is checking for "repr(Rust) packed(1)" and only suppresses the error check, which seems to be overfitting to ghost specifically.
There was a problem hiding this comment.
We still won't learn how relevant it actually is. So that run is IMO an entire waste of resources. OTOH if we reject all arrays we'll see how often the pattern occurs and then we can still adjust the check later.
There was a problem hiding this comment.
I don't think crater will necessarily catch all cases of this breakage. ghost has nearly 17 million lifetime downloads on crates.io, but its most popular dependent has ~60k. That suggests there is a lot of usage outside crates.io.
More broadly, I just don't think this sort of breakage is acceptable given Rust's stability policies. Breaking changes that are fundamentally difficult or even impossible for crates to work around should require more justification than "it's slightly easier".
There was a problem hiding this comment.
Worth asking: @dtolnay what do you think about this?
There was a problem hiding this comment.
We are not discussing which change should actually land. We are discussing which change to run with crater. I am arguing that we get better data by cratering this PR in a version that rejects all arrays.
- Either I am right and crater finds no array uses. That's good to know. I would argue that's good enough to make this an FCW; if there's tons of private use of this that relies on arrays in repr(transparent) we'll hear about it during the warning period. Ultimately its up to the lang team to weigh this though, our task is to give them the data they need for a decision.
- Or crater finds some (many?) array uses, giving us clear evidence that yes we do need more targeted checks.
You have so far not at all engaged with that argument.
There was a problem hiding this comment.
I have queued up a crater run in #156114 so that we can get that data.
For that we need a version of this that emits a hard error. |
72a9d17 to
d4c13b1
Compare
This comment has been minimized.
This comment has been minimized.
With this PR, an array type is considered trivial for the purpose of `repr(transparent)` only if its element type is—we emit the `repr_transparent_non_zst_fields` FCW otherwise. This has two benefits: ## Forbid non-portable definitions Some types have alignment 1 only on certain platforms. Prior to this PR, the following snippet would compile on AVR, and *only* on AVR: ```rust #[repr(transparent)] struct Foo(i32, [u16; 0]); ``` After this PR, the above now fails to compile on any target. ## FFI and CFI compatibility We want to add support for Control Flow Integrity to Rust at some point. There are some good reasons to want CFI to consider `*const [u8; 0]` and `*const [u8; 1]` compatible with one another. But that means we must consider `*const [u8; 0]` and `*const ()` to be CFI-incompatible. Declaring `[u8; 0]` non-trivial for `repr(transparent)` makes that easier to achieve. See discussion on Zulip: <https://rust-lang.zulipchat.com/#narrow/channel/136281-t-opsem/topic/ABI-compatibility.20rules.20of.20ZST.20types/near/591412488>
Needed to support the `ghost` crate.
c5c22f8 to
2af89b0
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
|
The PR should now be ready for crater @rustbot ready |
|
I don't think this is the check we even want, that's too many ad-hoc hacks. Let's just check the version where all arrays get rejected, to get a baseline that will tell us whether there's any evidence in the ecosystem that these hacks are needed. |
| typing_env, | ||
| t, | ||
| inside_repr_rust_packed_1 | ||
| || def.repr().pack.is_some_and(|a| a.bytes() == 1), |
There was a problem hiding this comment.
This will also be true for repr(Swift, packed) if we get that in the future... I guess there's no good way to check for repr(Rust, packed)?
There was a problem hiding this comment.
There isn't, sadly
There was a problem hiding this comment.
One verbose way to achieve this would be an exhaustive field match on ReprOptions similar to #155418.
|
@bors try |
|
⌛ Trying commit fe0c86e with merge 47f5a12… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/25234662750 |
`repr(transparent)`: don't consider most length-0 arrays trivial
| return ControlFlow::Continue(()); | ||
| } | ||
|
|
||
| let elem_layout = tcx.layout_of(typing_env.as_query_input(*elem_ty)); |
There was a problem hiding this comment.
| let elem_layout = tcx.layout_of(typing_env.as_query_input(*elem_ty)); | |
| // Arrays only have trivial ABI if their element type has trivial ABI, | |
| // so we need to recurse. However, the element type might be non-zero-sized | |
| // even if the array is zero-sized, so we have to check the layout before | |
| // we can recurse. | |
| let elem_layout = tcx.layout_of(typing_env.as_query_input(*elem_ty)); |
There was a problem hiding this comment.
Please add tests at least for
- newtype around
[u16; 0] - newtype around
[u8; 0](with a comment explaining that rejecting this is unnecessary but it's not worth the extra logic to accept it) - the type generated by
ghost
| #[rustc_abi(assert_eq)] | ||
| type TestC = (extern "C" fn($t1) -> $t1, extern "C" fn($t2) -> $t2); | ||
| #[rustc_abi(assert_eq)] | ||
| type TestSystem = (extern "system" fn($t1) -> $t1, extern "system" fn($t2) -> $t2); |
There was a problem hiding this comment.
This has nothing to do with repr(transparent), does it?
There was a problem hiding this comment.
With the whole business of the Windows ABI passing some empty structs by pointer, I figured that an extra check in this area can't hurt. I don't think this PR would break anything there, but that's what tests are for
There was a problem hiding this comment.
Maybe make this a separate PR then so we don't mix up the discussion.
We run this test on lots of targets, and for almost all of them "system" is the same as "C". So I think this is mostly a waste of time. However you make a good point -- it would make sense to also test some of the target-specific ABIs here. We can just add those under cfg(...) then I think?
| for field in field_infos { | ||
| if field.trivial | ||
| && let Some(unsuited) = check_unsuited(tcx, typing_env, field.ty).break_value() | ||
| && let Some(unsuited) = check_unsuited(tcx, typing_env, field.ty, false).break_value() |
There was a problem hiding this comment.
| && let Some(unsuited) = check_unsuited(tcx, typing_env, field.ty, false).break_value() | |
| && let Some(unsuited) = check_unsuited(tcx, typing_env, field.ty, /* inside_repr_rust_packed_1 */ false).break_value() |
| inside_repr_rust_packed_1 | ||
| || def.repr().pack.is_some_and(|a| a.bytes() == 1), |
There was a problem hiding this comment.
This ... || ... expression seems worth let-binding before the map.
| ty::Array(elem_ty, len) => { | ||
| // If we are inside a `#[repr(Rust, packed(1))]` ADT, | ||
| // the alignment is guaranteed to be 1 and Rust has full control over the ABI. | ||
| // Therefore, we can allow any length-0 array. |
There was a problem hiding this comment.
Why only 0-length arrays?
There was a problem hiding this comment.
#[repr(Rust, packed(1))] guarantees alignment 1, but not size 0. If the element type contains e.g. repr(C), it might not be portably 0-sized. The if elem_trivial { check_unsuited(...) } check below allows the non-0-length arrays that we can safely allow.
I suppose we could allow types like #[repr(Rust, packed(1))] struct Foo([[u128; 0]; 42]), which this PR currently rejects. But again, that would conflict with your preference for less complexity.
There was a problem hiding this comment.
We only ever call check_trivial on types that we already know to be zero-sized. So I think you can remove the length check and everything will remain correct. Or am I missing something?
There was a problem hiding this comment.
We only ever call
check_trivialon types that we already know to be zero-sized
… on the current target. The element type could be an empty repr(C) struct. We have to reject this:
#[repr(C)]
struct MaybeZst;
#[repr(packed)]
struct MaybeZstWrap([MaybeZst; 42]);
#[repr(transparent)]
struct ThisShouldntCompile(u32, MaybeZstWrap);There was a problem hiding this comment.
I see. That warrants a comment in the code and a test case.
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
|
☔ The latest upstream changes (presumably #156217) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I cratered a stricter version of this in #156114, and found no regressions. There's still a bunch of review feedback above though -- the logic here is quite subtle and there should be more comments explaining it. |
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
| // Even some 1-ZST fields are not allowed though, if they have `non_exhaustive` or private | ||
| // fields or `repr(C)`. We call those fields "unsuited". |
There was a problem hiding this comment.
This comment is outdated now.
| // Even some 1-ZST fields are not allowed though. Some fields are "unsuited", which can be the case for various reasons: | |
| // - semver: `non_exhaustive` types or types with private fields may become non-1-ZST in the future. | |
| // - ABI: not all 1-ZST actually have trivial ABI. | |
| // - Portability: the type might not actually be a 1-ZST on other targets. |
One thing I am wondering is if we really want to add that last point. The repr(C) point was justified with portability, but under #157973 that falls under the ABI category.
Either way, please write up a high-level human-readable specification of the entire algorithm, i.e. something that fully defines which types are considered trivial by repr(transparent) and which not, but in a way that is optimized for human consumption, not for being expressed in reasonably efficient code. We'll need that for the t-lang FCP and we'll need to put it in the Reference later. (That's why I think we should maybe drop the "Portability" point: this wasn't a concern so far, and it's a bunch of extra complexity that has to permanently become part of the language spec.)
There was a problem hiding this comment.
Note: my current plan is to hold off on pursuing this PR until #157973 is past FCP.
There was a problem hiding this comment.
Yeah that makes sense, as those rules will impact what exactly the logic has to be here.
| Array, | ||
| } | ||
|
|
||
| fn check_unsuited<'tcx>( |
There was a problem hiding this comment.
| fn check_unsuited<'tcx>( | |
| /// It is a precondition to this function that the type has size 0. | |
| /// Note that non-trivial alignments are permitted! That can happen when | |
| /// we recurse into the fields of a packed struct. | |
| fn check_unsuited<'tcx>( |
View all comments
With this PR, an array type is considered trivial for the purpose of
repr(transparent)only if its element type is—we emit therepr_transparent_non_zst_fieldsFCW (#78586) otherwise. To support a pattern used by theghostcrate, we also permit all array types with length 0 when they are contained within arepr(Rust, packed(1))ADT.This has two benefits:
Forbid non-portable definitions
Some types have alignment 1 only on certain platforms. Prior to this PR, the following snippet would compile on AVR, and only on AVR:
After this PR, the above now fails to compile on any target.
FFI and CFI compatibility
We want to add support for Control Flow Integrity to Rust at some point. There are some good reasons to want CFI to consider
*const [u8; 0]and*const [u8; 1]compatible with one another. But that means we must consider*const [u8; 0]and*const ()to be CFI-incompatible. Declaring[u8; 0]non-trivial forrepr(transparent)makes that easier to achieve. See discussion on Zulip:https://rust-lang.zulipchat.com/#narrow/channel/136281-t-opsem/topic/ABI-compatibility.20rules.20of.20ZST.20types/near/591412488
@rustbot label T-lang needs-fcp A-repr
Also needs a crater run.