Ignore ZST offsets when deciding whether to use Scalar/ScalarPair layout

[erikdesjardins]

This is important because Scalar/ScalarPair layout previously would not be used if any ZST had nonzero offset.
For example, before this change, only ((), u128) would be laid out like u128, not (u128, ()).

Fixes #63244

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[petrochenkov]

AFAIR, this restriction was introduced to be able to do DST coercions, but I'm not sure whether this applies to tuples.

r? @RalfJung

[erikdesjardins]

Yep, this PR miscompiles the following code :(

#![feature(unsized_tuple_coercion)]

pub type Siz = (u8, [u32; 2]);
pub type Un = (u8, [u32]);

pub fn coercion(x: &Siz) -> &Un {
    x
}

LLVM IR

define { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } @_ZN4test8coercion17h76ca7bfe6ae8dcf2E({ [0 x i32], [2 x i32], [0 x i8], i8, [3 x i8] }* noalias readonly align 4 dereferenceable(12) %x) unnamed_addr #0 {
start:
  %0 = bitcast { [0 x i32], [2 x i32], [0 x i8], i8, [3 x i8] }* %x to { [0 x i8], i8, [3 x i8], [0 x i32] }*
  %1 = insertvalue { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } undef, { [0 x i8], i8, [3 x i8], [0 x i32] }* %0, 0
  %2 = insertvalue { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } %1, i64 2, 1
  ret { { [0 x i8], i8, [3 x i8], [0 x i32] }*, i64 } %2
}

Should I: limit this to types that don't implement Unsize<T> (if that's possible and reasonable to do for layout); or limit this to ZSTs in the last field; or give up on making this change and just add the above code as a test?

[RalfJung]

This is way outside my league I'm afraid.^^ But AFAIK it is indeed impossible to reorder the last field due to unsizing coercions.

just add the above code as a test?

That would definitely be a good idea. I cannot give any advice on the other alternatives, sorry.

Cc @eddyb (but they are very busy recently)... not sure whom else to ping? Cc @nikomatsakis

[nikomatsakis]

r? @nikomatsakis -- I'll take a look and try to give intelligent feedback :)

[erikdesjardins]

Changed to instead allow reordering of the last field of a MaybeUnsized struct iff it's a ZST, which I think is most likely to be viable (if anything is).

[erikdesjardins]

I'm not sure if this is actually okay.

[nikomatsakis]

Well, the logic makes some sense to me, at least, but I'd like to hear what @eddyb thinks, as they definitely know this code best (I'm not all that familiar with it, to be honest).

I guess the "other way around" here would be not to reorder fields but to make the checks and code that are failing with the current ordering more capable?

I guess in any case that the logic is that we can 'permute' the offset of ZST fields arbitrarily, since there is no memory to be accessed? The offset might be visible in other ways (e.g., computing raw pointers and converting them to integers), not sure if that's a concern. I guess the layout of tuples is largely undefined...

Also, just as a point of "curiosity", I think that -- in general -- it's actually plausible for us to reorder DST fields even if they are not last, so long as we can recover not only the alignment of the field but also its size, but it would then require us to do the arithmetic at runtime to compute the offsets of all other fields, something we've traditionally avoided for its complexity.

[erikdesjardins]

Yeah, it's possible that the following check could be removed instead:

rust/src/librustc_middle/ty/layout.rs

Lines 393 to 395 in b7856f6

	// All other fields must be ZSTs, and we need them to all start at 0.
	let mut zst_offsets = offsets.iter().enumerate().filter(|&(i, _)| fields[i].is_zst());
	if zst_offsets.all(|(_, o)| o.bytes() == 0) {

but I suspect that other code relies on all fields of a scalar layout having 0 offset so it might be a more complex change.

The relevant parts of layout haven't changed significantly since eddyb's original #45225 so I eagerly await their comments :)

[Muirrum]

@eddyb This is a triage bump.

[nikomatsakis]

r? @eddyb

Make it official we're waiting for their feedback here=)

[eddyb]

Ugh I'm really sorry, I only now got a triage PM about this, I don't recall seeing this PR before (I guess I haven't gotten around to checking the PRs assigned to me in the last 18 days - in general, feel free to PM me on Zulip).

This looks fine, I guess, I'm only worried about the perf loss from sorting twice (which I'm not sure can be avoided).

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 7336e5f48c72e3606f8e496daa21890f9415d881 with merge 9ebc0face0731b2ce4ff7382be873c6f734a0c50...

[eddyb]

Sorry about forgetting about Abi::Vector until the last moment, but it's also important (it also has #[repr(transparent)] support etc.).

You could/should also add tests for it but I'm not sure what type to use (maybe you can find a #[repr(transparent)] test that also tests Abi::Vector using #[repr(simd)] or similar?).

[eddyb]

@bors r-

[eddyb]

Also, I should say, this shouldn't cause more codegen than today, since any ZSTs that were reordered to the start, still are (that part of the code wasn't changed at all), so they will still get no offset applied.

The new byte-based offset will pretty much only replace an identical offset being applied as a "struct field", but both should result in the same machine code (barring any LLVM optimizations but since this is about ZSTs, I'm not worried about that).

[RalfJung]

@eddyb this is ready for another round of review.

@erikdesjardins please write a note in the PR when this is ready for review again; GitHub does not (reliably) send email notifications for force pushes.

[erikdesjardins]

I messaged @eddyb on Zulip as they mentioned above, but yeah I should probably still comment so other people know what's going on.

[erikdesjardins]

@rustbot modify labels: -S-waiting-on-author, +S-waiting-on-review

[eddyb]

I got the Zulip PM but then because of how Zulip forces you to always see messages when navigating, it marked it as read, I'm really sorry. I'm still generally months behind most things on GitHub (and even waiting-on-review PRs) but trying to help where I can (don't mind repeated PMs, it helps with various platforms being all over the place).

@bors r+

[bors]

📌 Commit 24e0913 has been approved by eddyb

[bors]

⌛ Testing commit 24e0913 with merge 5b9e886...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 5b9e886 to master...