unsized_locals fails to uphold alignment requirements

[RalfJung]

This is a variant of #68304 that was not fixed by #71170:

#![feature(unsized_locals)]

use std::any::Any;

#[repr(align(256))]
#[allow(dead_code)]
struct A {
    v: u8
}

impl A {
    fn f(&self) -> *const A {
        assert_eq!(self as *const A as usize % 256, 0);
        self
    }
}

fn mk() -> Box<dyn Any> {
    Box::new(A { v: 4 })
}

fn main() {
    let x = *mk();
    let dwncst = x.downcast_ref::<A>().unwrap();
    let addr = dwncst.f();
    assert_eq!(addr as usize % 256, 0);
}

still fails with

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `176`,
 right: `0`', align.rs:13:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[RalfJung]

Given that libcore and liballoc enable this feature, I am rather concerned they could inadvertently use it in a way that is still unsound. Also, unless we can fix this, the feature should probably be added to INCOMPLETE_FEATURES to warn people against using it.

[RalfJung]

I am rather concerned they could inadvertently use it in a way that is still unsound.

rustc bootstraps successfully with #71417 applied, so at least right now that does not seem to be the case (modulo the part of libstd that rustc never monomorphizes). Of course, that could change any time, there is no check for this.

[nikomatsakis]

Yes, thanks for opening this. To be specific, the issue here is centered on this line:

    let x = *mk();

and the fact that we have x: dyn Any, and LLVM alloca's don't respect the alignment requirements, right?

I think there may be a separate issue on that. (Link?)

But: one thing we had discussed was separating out the "sound" part and the "unsound" part into distinct feature gates, so that we can enable only the former in libstd. Perhaps that's a good focus for this issue?

[nikomatsakis]

cc @eddyb, who had discussed the separate feature gate, and @spastorino, who might be interested in doing the follow-up work to create that gate.

[RalfJung]

and the fact that we have x: dyn Any, and LLVM alloca's don't respect the alignment requirements, right?

Yes.

I think there may be a separate issue on that. (Link?)

Oh, I didn't know that. Let's see if someone finds it.^^

But: one thing we had discussed was separating out the "sound" part and the "unsound" part into distinct feature gates, so that we can enable only the former in libstd. Perhaps that's a good focus for this issue?

Unlike the other unsound features, this is one where we don't even have a plan for how to soundly implement it. Not sure how much I like keeping that around.

[nikomatsakis]

Yes, I see that argument.

I'm reminded of #70193, which limits the max alignment to 4K because of limits in dynamic linkers, though there's no real connection. It's just that part of the challenge here is that the alignment of a dyn Value is totally unknown -- it's not like we can conservatively align to 64 bytes or something.

[RalfJung]

If #70193 lands we could conservatively align to 4k here. 🤣 (no I am not serious)

[spastorino]

Assigning P-high as discussed as part of the Prioritization Working Group process, removing I-prioritize and nominating for discussion on our next weekly meeting.

[eddyb]

Unlike the other unsound features, this is one where we don't even have a plan for how to soundly implement it. Not sure how much I like keeping that around.

Can't we do something dynamic where we allocate size + (align - 1) and align it ourselves?
Also, this has been discussed before, hasn't it?

[RalfJung]

Also, this has been discussed before, hasn't it?

I don't know -- in #68304 we mostly talked about how to mitigate the effect on stable Rust.

Can't we do something dynamic where we allocate size + (align - 1) and align it ourselves?

D'oh, I somehow forgot that one can manually implement "aligned" allocation on top of an unaligned allocator. Of course that would work. That's probably better than killing this part of codegen entirely.^^

[spastorino]

We've discussed this in our last weekly meeting, leaving the nomination up for now as requested by @pnkfelix. He wants to open some fresh issues about this.

[pnkfelix]

Now that I've created more specific issues to track the important sub-tasks identified here, I think we can remove this from the nominated tickets.

[jackh726]

Given that this feature has been removed from core/alloc and is unstable, I'm going to downgrade this to P-medium. This should obviously block stabilization of unsized_locals, but doesn't need special attention outside of that.