Make struct layout not depend on unsizeable tail

[lukas-code]

fixes (after backport) #112048

Since unsizing Ptr<Foo<T>> -> Ptr<Foo<U> just copies the pointer and adds the metadata, the layout of Foo must not depend on niches in and alignment of the tail T.

Nominating for beta 1.71, because it will have this issue: @rustbot label beta-nominated

[rustbot]

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

[the8472]

These changes are necessary to fix it but I'm not sure if they're sufficient.

If an unsizing coercion could change which niche is considered the largest or how many bytes it spans that could alter the chosen layout in LayoutCalculator::univariant.
I haven't constructed an example yet where that actually happens and I'm not sure if it can happen (changing available niches between coercions seems iffy). But if you want to be conservative then disabling the alt-layout branch for unsized structs would be a safe choice.

[the8472]

I found a case that still fails with this PR:

#![feature(offset_of)]

#[derive(Clone)]
struct WideptrField<T: ?Sized> {
  first: usize,
  second: usize,
  niche: NicheAtEnd,
  tail: T,
}

#[derive(Clone)]
#[repr(C)]
struct NicheAtEnd {
  arr: [u8; 7],
  b: bool,
}

type Tail = [bool; 8];

fn main() {
  let sized = Box::new(WideptrField {
    first: 0xdead,
    second: 0xbeef,
    niche: NicheAtEnd {
      arr: [0; 7],
      b: false,
    },
    tail: [false; 8] as Tail,
  });

  let unsized_value: Box<WideptrField<dyn Send>> = sized.clone();

  println!("offset in niche, sized   = {}", core::mem::offset_of!(WideptrField<Tail>, niche));
  println!("offset in niche, unsized = {}", core::mem::offset_of!(WideptrField<dyn Send>, niche));
}

prints

offset in niche, sized   = 0
offset in niche, unsized = 16

[the8472]

I have pushed a change that fixes this case. Feel free to force-push over my commit if you already have your own.

[the8472]

Do Container<T> layouts for Container<dyn Foo> always make the tail field zero-sized (as far as layout is concerned)? If so we should have a (debug?) assert that checks that any kind == MaybeUnsized type with a non-zero tail has the same layout as the dyn version of itself.

Layout optimizations have lots of edge-cases. Having an assert will cover more than just the UI test. Or at least make finding bugs easier.

[lukas-code]

Do Container<T> layouts for Container<dyn Foo> always make the tail field zero-sized (as far as layout is concerned)?

This seems to be the case. It looks like dyn Trait is treated like a unit type (no fields & no repr), but unsized:

rust/compiler/rustc_ty_utils/src/layout.rs

Lines 253 to 266 in 498553f

	ty::Dynamic(_, _, ty::Dyn) | ty::Foreign(..) => {
	let mut unit = univariant_uninterned(
	cx,
	ty,
	IndexSlice::empty(),
	&ReprOptions::default(),
	StructKind::AlwaysSized,
	)?;
	match unit.abi {
	Abi::Aggregate { ref mut sized } => *sized = false,
	_ => bug!(),
	}
	tcx.mk_layout(unit)
	}

[lukas-code]

I've built a very ugly and slow draft of the assert here and it didn't find any more incorrectly reordered fields in the (ui) test suite.

I might make that ready for merge soonish in a future PR.

[apiraino]

backport decision postponed after lands in nightly (Zulip discussion)

[eholk]

Niche layout decisions seem subtle enough that I don't really feel confident reviewing this, so I'll reroll.

r? compiler

[petrochenkov]

r? @the8472

[the8472]

@petrochenkov I contributed a commit to this. I shouldn't be reviewing my own changes 😅.

r? @wesleywiser since he also reviewed #108106

[wesleywiser]

Thanks @lukas-code and @the8472! I pushed a commit that adds the niche layout test case @the8472 came up with as another regression test.

@bors r+ p=5

Fixes unsound, P-critical issue.

[bors]

📌 Commit b982f3a has been approved by wesleywiser

It is now in the queue for this repository.

[bors]

⌛ Testing commit b982f3a with merge 6330daa...

[bors]

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 6330daa to master...

[rust-timer]

Finished benchmarking commit (6330daa): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.349s -> 648.99s (0.25%)

[apiraino]

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted