When DST struct's last field is misaligned, memory access generated are for the wrong address

[mukilan]

Example

struct Foo<T: ?Sized + Bar> {
    a: u8,
    b: T
}

trait Bar {
    fn get(&self) -> usize;
}

struct BarImpl {
    i: usize
}

impl Bar for BarImpl {
    fn get(&self) -> usize {
        self.i
    }
}


fn main() {
    let f = Foo { a:1, b: BarImpl { i: 5 } };
    assert!(f.b.get() == 5); // succeeds
    let f = &f as &Foo<Bar>;
    assert!(f.b.get() == 5); // assert fails
}

in playpen

[eddyb]

cc @rust-lang/compiler

[Aatch]

Hmm, this seems tricky to solve properly, I see three options.

1.Force word alignment on unsized fields. This is the easiest, but won't work for types with higer alignment due to SIMD or similar.
2. Force maybe-unsized types to be packed, but that doesn't seem very appealing.
3. Calculate the appropriate offset based on the alignment information in the vtable. This is the most robust method, but makes extracting the field in an unsized type require a runtime calculation.

[eddyb]

@Aatch the preferred solution involves right-aligning fields before the unsized one (as opposed to left-aligning them) and making pointers to the structure point to the field directly.
But it's more work and may not play nicely with LLVM.

That and your 3. are the only two correct solutions I know of.

[Gankra]

Worth noting that tuples have their last field ?Sized, so any decision here may affect them too.

[eddyb]

@gankro you sure? I'd be very surprised if that were the case, as I've never seen any compiler code supporting unsized tuples (or enums, for that matter).

[Aatch]

@gankro @eddyb seems to be right: https://play.rust-lang.org/?gist=38f03e1c1ed02dafffa9&version=nightly

[Gankra]

https://github.com/rust-lang/rfcs/blob/master/text/0401-coercions.md#coercions specified this (last bullet of "coerce_inner").

https://github.com/rust-lang/rfcs/blob/master/text/0982-dst-coercion.md does not appear to overturn this decision.

If it doesn't work, it's a bug.

[eddyb]

@gankro 401 is obsolete wrt unsizing. 982 specifies struct only, and that's it. Nothing else is specified atm. If there is a bug, it's that 982 doesn't make it clear that it obsoleted 401's unsizing, which was overreaching and remained unimplemented.

[Gankra]

Works for me 👍

[pnkfelix]

@Aatch wrote (the third of three options):

1. [...]
2. [...]
3. Calculate the appropriate offset based on the alignment information in the vtable. This is the most robust method, but makes extracting the field in an unsized type require a runtime calculation.

Just to be clear: am I correct that the runtime calculation is only necessary when the type T (for the field of type T: ?Sized) has been instantiated with a trait type? I.e. if we instantiated T with the concrete types S or [S; k] (where S: Sized), then there is no need for runtime calculation to determine the field offset, right?

I ask because that scenario does not sound so dire to me.

[eddyb]

@pnkfelix Yes, it's only a performance regression for P<Trait> with P being Rc or Arc, but not Box, &_ or any other prefix-less pointer.
Also P<[T]> is not affected, as it has a constant alignment (that of T).

The cheaper solution involves pointing, e.g. Rc<Trait> directly at the Trait and having the ref-counts right-aligned so they are at constant negative offsets from the Trait lvalue.

But since struct layout is not specified (and #[repr(C)] structs must always be Sized, although I'm not sure we enforce it), switching from runtime calculations to the pointer-to-unsized-field scheme should be backwards-compatible.

That said, even if an implementation of @Aatch's 3rd option is likely to be merged, it's still somewhat low-priority, with Rc<Trait> and Arc<Trait> behaving correctly for types of pointer alignment or less.

I expect the 16-byte SIMD alignments to mess things up, but on x64 that only can happen if the allocation itself is not aligned to 16 bytes, as the RcBox prefix is also 16 bytes there.

[pnkfelix]

@eddyb yes I would normally be inclined to agree that it is low-priority; I came and looked at this based on @eefriedman 's linking #27023 here ... but I'm not really sure I agree these are fundamentally the same bug. (That is, it seems like a fix for #27023 can happen independently, and sooner, than the fix for #26403 ...)