repr(transparent) wrapper with uninhabited ZST has different return ABI than wrapped field.
#135802
Comments
zachs18
added
C-bug
rustbot
added
needs-triage
This comment has been minimized.
This comment has been minimized.
|
That would be a breaking change though since we currently accept code like this on stable: enum Void {}
#[rept(transparent)]
struct Wrap<T>(Void, T);@rust-lang/lang turns out accepting this code is incoherent; we cannot make this a transparent wrapper around Cc @rust-lang/opsem |
RalfJung
changed the title
repr(transparent) wrapper with uninhabited ZST, transmuted to function pointer returning wrapped field.repr(transparent) wrapper with uninhabited ZST has different return ABI than wrapped field.
RalfJung
added
the
I-lang-nominated
|
I’m not sure it’s incoherent from a language perspective. Clearly it clashes with the current lowering chosen by the compiler, but this may just be a compiler bug. There’s no problem when the 1-ZST field is inhabited from the compiler’s perspective, even if it has a custom safety invariant that makes it effectively impossible to construct, right? It’s just that the compiler currently applies the “return type is known to be uninhabited -> function can’t return at all” reasoning to functions where this contradicts our general function ABI compatibility rules. Similarly, if there was a target where Changing this would be a slight loss of optimization power in theory but it doesn’t seem so bad since any call site that calls the function as |
This comment has been minimized.
This comment has been minimized.
|
It's the fault of (This is not a fictional example, I've written such code before except that I did not yet put the witness ZST in a field of a transparent wrapper.) |
I am not sure what alternative fix you are suggesting though. We would have to entirely remove To me this seems like a clear oversight -- we intended to only allow fields that are irrelevant to the language (ignoring safety invariants, the user is responsible for that), but forgot about uninhabited types. |
This comment has been minimized.
This comment has been minimized.
As I said, that specific code doesn't interact with
Note that transparent wrappers have always been able to add extra safety invariants to the non-ZST field (e.g., |
This comment has been minimized.
This comment has been minimized.
I don't know the relevant compiler internals, but would it be possible to only stop using |
Yes, something like that. Specifically, I think it would be sufficient if functions that return a transparent wrapper around an inhabited type were compiled as if their return type was inhabited, even if the transparent wrapper has uninhabited 1-ZST types. I realize that such a fine distinction is difficult to fit into rustc's inner workings, but e.g. I don't think there's any need to change the ABI of e.g.
Rust has historically missed several nuances w.r.t. |
It's possible to draw a distinction between
So it's as basically close to making that part of its public ABI as you can get within the current limitations of rustdoc. Transmuting from it to the underlying integer type is allowed and the other direction is also allowed with the exception of 0. |
|
Also, the aforementioned rustdoc heuristic means that a type like I mentioned previously:
indeed shows the attribute in its documentation: use core::arch::x86_64::__m256i;
struct HasAvx2 {
_feature_detected: (),
}
#[repr(transparent)]
pub struct U32x8(pub __m256i, HasAvx2);
|
Yes, I consider this part of the public API. It's just there by definition rather than by name, which removes any possible ambiguity on the interpretation of the name (i.e. it's just about layout and ABI, and not about the set of valid representations). Thanks for all the clarifications! From my point of view, I got convinced that the current definition of |
We need to compile That said, I don't actually know which effect |
Because "Any two types with size 0 and alignment 1 are ABI-compatible." you can at least reproduce the miri crash without any transparent wrapper: enum Void {}
fn foo() -> Void {
panic!()
}
fn main() {
let f: fn() -> Void = foo;
let f: fn() = unsafe { std::mem::transmute(f) };
f();
}This is less likely to lead to an actual miscompilation because both return types being ZST should erase any differences in the generated machine code. But I feel like it points towards the root of the problem being ABI compatibility vs. exploiting uninhabited return types. Transparent wrappers are just an easy way to exercise it with the relatively small set of current ABI compatibility guarantees. |
We treat it mostly as a ZST I think, though if you return an uninhabited type, the return terminator gets replaced by unreachable: rust/compiler/rustc_codegen_ssa/src/mir/block.rs Lines 452 to 462 in 99768c8 rust/compiler/rustc_codegen_cranelift/src/base.rs Lines 293 to 302 in 99768c8 |
jieyouxu
added
A-repr
RalfJung
removed
the
I-lang-nominated
Ah, good point. The Miri ICE can probably be fixed by adjusting this check, but it doesn't seem right to permit |
|
Vibe-wise it feels like As for Footnotes
|
That's not even well-defined though with the current rustc architecture; the |
|
I have two branches for possible resolutions of this isssue:
Personally, I would prefer option 1, but IIUC this needs a T-lang decision? |
|
Since both branches target |
|
The first doesn't specifically target It does appear to fix the ICE from this snippet as well, which makes sense, it makes `#[rustc_layout(debug)]`s of `enum Void {}` and unit under option 1error: layout_of(()) = Layout {
size: Size(0 bytes),
align: AbiAndPrefAlign {
abi: Align(1 bytes),
pref: Align(8 bytes),
},
abi: Memory {
sized: true,
},
fields: Arbitrary {
offsets: [],
memory_index: [],
},
largest_niche: None,
uninhabited: false,
variants: Single {
index: 0,
},
max_repr_align: None,
unadjusted_abi_align: Align(1 bytes),
randomization_seed: 0,
}
--> src/main.rs:4:1
|
4 | pub type A = ();
| ^^^^^^^^^^
error: layout_of(Void) = Layout {
size: Size(0 bytes),
align: AbiAndPrefAlign {
abi: Align(1 bytes),
pref: Align(1 bytes),
},
abi: Memory {
sized: true,
},
fields: Primitive,
largest_niche: None,
uninhabited: true,
variants: Empty,
max_repr_align: None,
unadjusted_abi_align: Align(1 bytes),
randomization_seed: 0,
}
--> src/main.rs:6:1
|
6 | enum Void {}
| ^^^^^^^^^$ cargo +dev-compiler-2-stage2 miri run
Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
Compiling miri-thing v0.1.0 (/tmp/miri-thing)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
Running `/home/zachary/Programming/rust/rust-worktree/rust-compiler-2/build/x86_64-unknown-linux-gnu/stage2/bin/cargo-miri runner /home/zachary/opt_mount/zachary/cargo-target/miri/x86_64-unknown-linux-gnu/debug/miri-thing`
thread 'main' panicked at src/main.rs:3:3:
explicit panic
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effectFootnotes |
|
Option 1 only changes compiler-internal implementation details, doesn't it? So an MCP should be sufficient for that. I do like that option. :) |
…bited, r=workingjubilee Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc `@RalfJung`
…bited, r=workingjubilee Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc ``@RalfJung``
…bited, r=workingjubilee Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc `@RalfJung`
…bited, r=workingjubilee Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc ``@RalfJung``
…ted, r=<try> Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc `@RalfJung` try-job: x86_64-gnu-nopt
…bited, r=workingjubilee Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc `@RalfJung`
Rollup merge of rust-lang#136985 - zachs18:backend-repr-remove-uninhabited, r=workingjubilee Do not ignore uninhabited types for function-call ABI purposes. (Remove BackendRepr::Uninhabited) Accepted MCP: rust-lang/compiler-team#832 Fixes rust-lang#135802 Do not consider the inhabitedness of a type for function call ABI purposes. * Remove the [`rustc_abi::BackendRepr::Uninhabited`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/enum.BackendRepr.html) variant * Instead calculate the `BackendRepr` of uninhabited types "normally" (as though they were not uninhabited "at the top level", but still considering inhabitedness of variants to determine enum layout, etc) * Add an `uninhabited: bool` field to [`rustc_abi::LayoutData`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_abi/struct.LayoutData.html) so inhabitedness of a `LayoutData` can still be queried when necessary (e.g. when determining if an enum variant needs a tag value allocated to it). This should not affect type layouts (size/align/field offset); this should only affect function call ABI, and only of uninhabited types. cc ``@RalfJung``
This code ICEs Miri, the normal compiler compiles this without complaint, but might be miscompiling it1, though a
constversion using-Zunleash-the-miri-inside-of-youdoes ICE the compiler.Code
modified version using `-Zunleash-the-miri-inside-of-you` that ICEs rustc
RUSTFLAGS="-Zunleash-the-miri-inside-of-you" cargo +nightly runBacktrace:
Meta
rustc --version --verbose:Error output
Error output
ICE file
@rustbot label +A-miri +A-ABI
This shouldn't ICE regardless, but whether there is a miscompilation depends on if the original code is sound:
CC rust-lang/unsafe-code-guidelines#485
Possible Miscompilation
Does
#[repr(transparent)]s ABI-compatility guarantee apply in the prescence of uninhabited 1-ZST fields?A
#[repr(transparent)]wrapper that contains an uninhabited 1-ZST field getsUninhabitedABI IIUC, which may contradict the guarantees about#[repr(transparent)]wrappers being ABI-compatible.Consider
Foofrom above:If
#[repr(transparent)]'s ABI-compatility guarantees do apply, then the above code should be sound:Foo, it must diverge (e.g. by panicking).Foois ABI-compatible withField, then it should be sound to transmute a function pointer to a divergingfn() -> Foointo afn() -> Fieldthat also diverges.rustc_layout(debug)
Using
#![feature(rustc_attrs)]and#[rustc_layout(debug)]we see that the ABI it has iswhereas
FieldhasPossible Miscompilation
Assuming this is supposed to be sound, then there is an observable miscompilation here too. Where
Fieldis returned by invisible reference,fn(u32) -> Foodoes not returnFooby invisible reference, so transmutingfn(u32) -> Foointofn(u32) -> Fieldwill pass a different number of arguments.Specifically on
x86_64-unknown-linux-gnu: IfFieldis returned on the stack, thenfn(..) -> Foodoes not reserve stack space or pass a hidden reference parameter, butfn(..) -> Fielddoes. This can be made into an observable miscompilation if the function takes parameters: (playground link, IIUC: the invisible reference is being passed, but the callee isn't expecting it, so it thinks it's the first "real" parameter instead (caller: ret by-ref inrdi,xinrsi; callee: ret uninhabited,xinrdi)).Footnotes
see "Possible Miscompilation" section ↩
The text was updated successfully, but these errors were encountered: