Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking issue for RFC 2645, "Transparent Unions" (formerly: and Enums) #60405

Open
1 of 3 tasks
Centril opened this issue Apr 30, 2019 · 70 comments
Open
1 of 3 tasks

Tracking issue for RFC 2645, "Transparent Unions" (formerly: and Enums) #60405

Centril opened this issue Apr 30, 2019 · 70 comments
Labels
A-repr Area: the `#[repr(stuff)]` attribute B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-design-concerns Status: There are blocking design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Apr 30, 2019

This is a tracking issue for the RFC "Transparent Unions and Enums" (rust-lang/rfcs#2645).

Steps:

Unresolved questions:

The role of #[repr(transparent)] in nonnull-style optimizations is not entirely clear. Specifically, it is unclear whether the user can rely on these optimizations to be performed when they make a type transparent. Transparent unions somewhat complicate the matter. General concensus seems to be that the compiler is free to decide where and when to perform nonnull-style optimizations on unions (regardless of whether or not the union is transaprent), and no guarantees are made to the user about when and if those optimizations will be applied. It is still an open question exactly what guarantees (if any) Rust makes about transparent structs (and enums) and nonnull-style optimizations.

This RFC doesn't propose any changes to transparent structs, and so does not strictly depend on this question being resolved. But since this RFC is attempting to round out the #[repr(transparent)] feature, it seems reasonable to dedicate some time to attempting to round out the guarantees about #[repr(transparent)] on structs.

Also it is not clear if transparent unions can even be implemented on LLVM without seriously restricting our semantics for unions overall.

@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Apr 30, 2019
@mjbshaw
Copy link
Contributor

mjbshaw commented Apr 30, 2019

I plan on attempting to implement the feature by adapting my previous implementation. I'd appreciate it if someone could take a look at that commit and let me know if it's horribly wrong :)

@Centril
Copy link
Contributor Author

Centril commented Apr 30, 2019

@pnkfelix Up for reviewing ^--- ?

mjbshaw added a commit to mjbshaw/rust that referenced this issue Jun 11, 2019
bors added a commit that referenced this issue Jun 11, 2019
Implement RFC 2645 (transparent enums and unions)

Tracking issue: #60405
@Centril Centril added B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Jun 11, 2019
@Centril
Copy link
Contributor Author

Centril commented Jun 11, 2019

Implementation of both feature gates was done in #60463 which landed on 2019-06-11.
The PR was written by @mjbshaw and reviewed by @varkor and @rkruppe.

@Centril
Copy link
Contributor Author

Centril commented Jun 11, 2019

Should we apply #[repr(transparent)] to MaybeUninit now?

@mjbshaw
Copy link
Contributor

mjbshaw commented Jun 12, 2019

I'll submit a PR to update MaybeUninit.

mjbshaw added a commit to mjbshaw/rust that referenced this issue Jun 18, 2019
Centril added a commit to Centril/rust that referenced this issue Jun 18, 2019
…=cramertj

Make MaybeUninit #[repr(transparent)]

Tracking issue: rust-lang#60405
Centril added a commit to Centril/rust that referenced this issue Jun 18, 2019
…=cramertj

Make MaybeUninit #[repr(transparent)]

Tracking issue: rust-lang#60405
@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 29, 2019

I ran into this today:

#[repr(C)] struct S(u8, u32);

#[repr(transparent)]
union U { _x: S, y: () }

unsafe extern "C" fn foo(u: U) {
    let pad = (&u as *const _ as *const u8).add(1).read();
    assert_eq!(pad, 13); // Should fail when called below
}

pub unsafe fn bar() {
  let mut u: U = U { y: () };
  (&mut u as *mut _ as *mut u8).add(1).write(42);
  foo(u);
}

Because of (), all bit-patterns of union U are valid. The particular bit-pattern being constructed in bar is [undef, 42, undef, undef, undef, undef, undef, undef]. I expected the assert in foo to fail when called from bar: that bit-pattern is a valid bit-pattern for U, foo reads its first byte, and the test always returns false (42 != 13).

However, due to repr(transparent), this is not correct. repr(transparent) gives U the same ABI as S. That is, U will be passed by value "as if" it were an S. The byte of S at offset 1 is padding, and it does not need to be passed when calling foo.

For example, the ABI could state that S.0 is passed in a 1-byte register (e.g. al), and that S.1 is passed in a 32-bit register (e.g. ecx). This is not exactly, but close to what happens when that snippet is compiled for, e.g., i686-pc-windows-msvc (godbolt). The above is compiled in debug mode to:

core::ptr::write:
        ;; writes 42 to the address at ecx
        mov     byte ptr [ecx], 42
        ret
core::ptr::<impl *mut T>::add:
        ;; increments the address at ecx by 1
        lea     eax, [ecx + 1]
        ret
example::bar:
        sub     esp, 24
        ;; our union is at esp + 8, create ptr:
        lea     ecx, [esp + 8]
        ;; increment its address by 1
        call    core::ptr::<impl *mut T>::add
        mov     ecx, eax
        ;; writes 42 to the address (esp + 9)
        call    core::ptr::<impl *mut T>::write
        ;; now passing the union to foo begins
        ;; S.0 is put in `al` - padding be damned
        mov     al, byte ptr [esp + 8]
        ;; S.1 is put in `ecx`
        mov     ecx, dword ptr [esp + 12]
        ;; then these are moved back to the stack (esp + 16) 
        mov     byte ptr [esp + 16], al
        ;; the padding at esp +17/18/19 is uninitialized
        mov     dword ptr [esp + 20], ecx
        ;; and then moved back and forth a couple of times
        movsd   xmm0, qword ptr [esp + 16]
        movsd   qword ptr [esp], xmm0
        ;; and then we call foo, which never sees 42
        call    example::foo
        add     esp, 24
        ret

If this happens, then the bytes at offset 1,2, and 3 of the union U will not be passed to foo, and will become uninitialized. This would allow the compiler to optimize the assert_eq! inside foo away. This is exactly what happens if we crank-up the opt-level to 3 (godbolt):

example::bar:
        ret

If the union U has the same call ABI as S, then the bytes 1, 2, and 3 cannot be part of the "value representation" (cc @ubsan) of the union - they do not need to be memcpy'ed on move, etc. If these bytes need to be part of the "value representation" of the union, then the union cannot have the same call ABI as S - instead, the union would need the same ABI as, e.g., #[repr(C)] struct(i32,i32) so that those bytes get passed by value on copy/move/etc.

AFAICT, these two constraints are incompatible.

cc @eddyb @rust-lang/wg-unsafe-code-guidelines

@RalfJung
Copy link
Member

RalfJung commented Jun 29, 2019

Ouch. Nice catch!

The reason we implemented a "transparent" repr for unions (re-using the ABI of the only non-ZST field if possible) was because of code that used MybeUninit<SimdType> getting a lot slower compared to using mem::uninitialized, where there was no "wrapper". We had to switch MaybeUninit to use the SIMD ABI in that case to get the old performance back. At least that was our interpretation back then, I remember we did more than one thing to fix the perf problems, and I don't remember if we confirmed which one was "it". Here's the relevant PR.

So, a way to keep that while also achieving that "unions preserve all bytes when copied around" would be to only use the ABI of the only non-ZST field if that ABI is an "Integer" or "vector" one, where we know all bytes are preserved.

That would not really be repr(transparent) any more, that flag would have to be removed.

@mjbshaw
Copy link
Contributor

mjbshaw commented Jun 29, 2019

@gnzlbg I think that's UB, with or without repr(transparent). With repr(transparent). Without repr(transparent). The end results are exactly the same (interestingly, repr(C) is different).

I'm not aware of any guarantees or requirements that a wrapper type (whether it's a union or a struct) preserves padding bytes, or that reading padding bytes is valid/safe. If Rust makes any guarantees about that I'd love to know where I can read about them.

Centril added a commit to Centril/rust that referenced this issue Dec 16, 2019
make transparent enums more ordinary

By recognizing that structs & unions have one variant, we can make the treatment of transparent enums less ad-hoc.

cc rust-lang#60405

r? @davidtwco
Centril added a commit to Centril/rust that referenced this issue Dec 16, 2019
make transparent enums more ordinary

By recognizing that structs & unions have one variant, we can make the treatment of transparent enums less ad-hoc.

cc rust-lang#60405

r? @davidtwco
@Centril
Copy link
Contributor Author

Centril commented Jan 10, 2020

We discussed this briefly on the language team meeting today. Attendance was low. However, those present agreed that it would be OK to move towards stabilizing repr(transparent) on univariant enums specifically, while leaving the hint on unions unstable for the time being. I will write up a short report & PR.

@Centril
Copy link
Contributor Author

Centril commented Jan 11, 2020

Stabilization report & PR for enums is up at #68122.

bors added a commit that referenced this issue Jan 27, 2020
…enkov

Stabilize `#[repr(transparent)]` on `enum`s in Rust 1.42.0

# Stabilization report

The following is the stabilization report for `#![feature(transparent_enums)]`.

Tracking issue: #60405
[Version target](https://forge.rust-lang.org/#current-release-versions): 1.42 (2020-01-30 => beta, 2020-03-12 => stable).

## User guide

A `struct` with only a single non-ZST field (let's call it `foo`) can be marked as `#[repr(transparent)]`. Such a `struct` has the same layout and ABI as `foo`. Here, we also extend this ability to `enum`s with only one variant, subject to the same restrictions as for the equivalent `struct`. That is, you can now write:

```rust
#[repr(transparent)]
enum Foo { Bar(u8) }
```

which, in terms of layout and ABI, is equivalent to:

```rust
#[repr(transparent)]
struct Foo(u8);
```

## Motivation

This is not a major feature that will unlock new and important use-cases. The utility of `repr(transparent)` `enum`s is indeed limited. However, there is still some value in it:

1. It provides conceptual simplification of the language in terms of treating univariant `enum`s and `struct`s the same, as both are product types. Indeed, languages like Haskell only have `data` as the only way to construct user-defined ADTs in the language.

2. In rare occasions, it might be that the user started out with a univariant `enum` for whatever reason (e.g. they thought they might extend it later). Now they want to make this `enum` `transparent` without breaking users by turning it into a `struct`. By lifting the restriction here, now they can.

## Technical specification

The reference specifies [`repr(transparent)` on a `struct`](https://doc.rust-lang.org/nightly/reference/type-layout.html#the-transparent-representation) as:

> ### The transparent Representation
>
>  The `transparent` representation can only be used on `struct`s that have:
>  - a single field with non-zero size, and
>  - any number of fields with size 0 and alignment 1 (e.g. `PhantomData<T>`).
>
> Structs with this representation have the same layout and ABI as the single non-zero sized field.
>
> This is different than the `C` representation because a struct with the `C` representation will always have the ABI of a `C` `struct` while, for example, a struct with the `transparent` representation with a primitive field will have the ABI of the primitive field.
>
> Because this representation delegates type layout to another type, it cannot be used with any other representation.

Here, we amend this to include univariant `enum`s as well with the same static restrictions and the same effects on dynamic semantics.

## Tests

All the relevant tests are adjusted in the PR diff but are recounted here:

- `src/test/ui/repr/repr-transparent.rs` checks that `repr(transparent)` on an `enum` must be univariant, rather than having zero or more than one variant. Restrictions on the fields inside the only variants, like for those on `struct`s, are also checked here.

- A number of codegen tests are provided as well:
    - `src/test/codegen/repr-transparent.rs` (the canonical test)
    - `src/test/codegen/repr-transparent-aggregates-1.rs`
    - `src/test/codegen/repr-transparent-aggregates-2.rs`
    - `src/test/codegen/repr-transparent-aggregates-3.rs`

- `src/test/ui/lint/lint-ctypes-enum.rs` tests the interactions with the `improper_ctypes` lint.

## History

- 2019-04-30, RFC rust-lang/rfcs#2645
  Author: @mjbshaw
  Reviewers: The Language Team

  This is the RFC that proposes allowing `#[repr(transparent)]` on `enum`s and `union`.

- 2019-06-11, PR #60463
  Author: @mjbshaw
  Reviewers: @varkor and @rkruppe

  The PR implements the RFC aforementioned in full.

- 2019, PR #67323
  Author: @Centril
  Reviewers: @davidtwco

  The PR reorganizes the static checks taking advantage of the fact that `struct`s and `union`s are internally represented as ADTs with a single variant.

- This PR stabilizes `transparent_enums`.

## Related / possible future work

The remaining work here is to figure out the semantics of `#[repr(transparent)]` on `union`s and stabilize those. This work continues to be tracked in #60405.
@RalfJung RalfJung changed the title Tracking issue for RFC 2645, "Transparent Unions and Enums" Tracking issue for RFC 2645, "Transparent Unions" (formerly: and Enums) May 6, 2020
@RalfJung
Copy link
Member

RalfJung commented May 6, 2020

This is now just tracking transparent unions.

I think I just realized there is a problem with transparent unions if we want to provide "bag of bytes" semantics for unions:

#[repr(transparent)]
union U { f: u32 }

As a union, U should be just a bag of bytes. If I make one of the bytes poison, then after copying the union around, that should properly preserve which byte is poison and which one is not.

However, to my knowledge, LLVM actually makes an i32 either fully poison or not poison at all -- so the moment such a partially poisoned U gets loaded as an i32 in LLVM, the remaining bytes would lose their content and reset to poison as well. Ouch.

@mjbshaw
Copy link
Contributor

mjbshaw commented May 7, 2020

I have two questions:

  1. Why is that not a problem if you omit #[repr(transparent)]?
  2. Why is this so bad? You just poisoned f; I'm not sure what else you'd expect to happen, with or without #[repr(transparent)].

I'm still not sold on the "bag of bits" idea. I've tried to put an alternative forward but instead of receiving a good rebuttal for why my alternative is inferior it instead go sidetracked by "let's decide what padding bits are" when I think we're pretty much already in agreement on what "padding bits" means. Is this something that a chat on Zulip or something could solve? I think some real-time vocal communication could resolve this in minutes, whereas asynchronous text comms will take days (or weeks) (and GitHub is a suboptimal forum due to it hiding replies and whatnot).

@hanna-kruppe
Copy link
Contributor

Why is that not a problem if you omit #[repr(transparent)]?

Since repr(Rust) unions have no ABI commitments, we can just represent them as a literal byte array. That allows each byte to be poison-or-not independently.

@mjbshaw
Copy link
Contributor

mjbshaw commented May 7, 2020

@hanna-kruppe That's true, but I'm not sure it's useful to allow such granularity for poison because you still can't read f. You can only memcpy the non-poison bytes of f, which I'm not sure Rust should guarantee you can do because it's such a big footgun (and I can't think of any real value it provides).

@hanna-kruppe
Copy link
Contributor

That goes more into your second question (why/how this observation matters), which I deliberately didn't go into because it's a more complex topic and my time is currently very limited.

@RalfJung
Copy link
Member

RalfJung commented May 7, 2020

Why is this so bad? You just poisoned f; I'm not sure what else you'd expect to happen, with or without #[repr(transparent)].

#[repr(transparent)]
union U { f: u32 }

let mut u = U { f: 0 };
(&mut u as *mut _ as *mut MaybeUninit<u8>).add(1).write(MaybeUninit::uninit());

let u2 = u;
let v = (&u2 as *const _ as *const u8).read();
println!("{}", v);

The Rust semantics as they exist in my head and as it is drafted here would guarantee that this program prints 0. This is what "unions are just bags of bytes" means.

But it turns out LLVM actually says "nope this is UB as all of u2 is poisoned". So, the desired semantics is not implementable with LLVM while upholding repr(transparent) guarantees.

Of course we could try to adjust our semantics, but (a) that will make the semantics of unions significantly more complicated, and (b) it seems like a shame that LLVM would force us to cripple our semantics like that, for no good reason. If LLVM's type system was not quite so restrictive, we could just tell LLVM to load 4 bytes at once and preserve which byte is poison and which is not. But LLVM conflates "bytes" with "integers that have arithmetic operations", and then a lot of sadness ensues.

@mjbshaw
Copy link
Contributor

mjbshaw commented May 8, 2020

Thanks for the demo, @RalfJung. I personally think this should have the same behavior as #[repr(transparent)] struct S { f: u32 }. I'll take some time to read through the draft you linked to, though.

@RalfJung
Copy link
Member

RalfJung commented May 8, 2020

@mjbshaw do you still think that if we replace U by MaybeUninit<u32>? Since MaybeUninit is repr(transparent), that's basically the same type.

@joshtriplett joshtriplett added the S-tracking-design-concerns Status: There are blocking design concerns. label Jun 15, 2022
@alercah
Copy link
Contributor

alercah commented Jul 13, 2022

Doesn't writing MaybeUninit::uninit() still write undef and not poison? Unless I'm quite mistaken, in this playground, it's undef and not poison that gets passed to write_to_float. And since undef is tracked bitwise, this all works out doesn't it?

Of course, if LLVM does change uninitialized memory to poison, then we would run into this problem, without bitwise semantics.

@RalfJung
Copy link
Member

Indeed, this was written assuming LLVM would switch to poison eventually.

Cc #94428

@alercah
Copy link
Contributor

alercah commented Oct 8, 2022

Is the concern in this comment about poison spreading to entire union fields not equally valid for #[repr(C)] unions that must be passed as integers per platform ABIs?

@RalfJung
Copy link
Member

RalfJung commented Oct 8, 2022

Yeah sure, that's the same situation.

Judging from this discussion, it seems like LLVM will get a "freezing load" operation. Whenever we have a load that would allow partially uninit data, we could use the freezing load and be sure that the data is preserved correctly. This does lose some information, but at least it would resolve the concern about spreading poison to neighboring bytes.

@clarfonthey
Copy link
Contributor

So, poking around the various open issues and PRs and this looks like the best place to mention this.

I mentioned in #101179 that I think that allowing DSTs in MaybeUninit might be a way to improve the API for slices (by allowing MaybeUninit<[T]>), but it appears that DSTs are still not allowed in unions, even #[repr(transparent)] ones like MaybeUninit. The closest this was to being implemented was #47650 which got closed because the author didn't have time to finish it.

Was there ever an explicit reason to disallow this, or was it just not implemented/considered since no one had discussed it much?

@RalfJung
Copy link
Member

RalfJung commented Dec 3, 2022

The problem is custom DST. If CStr becomes a DST, then size_of_val might have to actually read the data behind the reference to determine the length of the value -- but clearly with a MaybeUninit<CStr> that would be bad.

@clarfonthey
Copy link
Contributor

Ah, right -- we haven't fully eliminated the possibility of thin DSTs. I guess that we're still unsure what a proper custom DST RFC would look like, although my guess is that CStr will be a thorn in the side for most of them. C strings really are the worst. (I say this sarcastically, although I'm not surprised to find another reason why they're problematic.)

My gut feeling is to say that any RFC which would permit thin DSTs (which, as demonstrated by CStr, would be something we want) is that we'd also want a mechanism to filter them out, precisely for cases like this. But I guess that this isn't a strong enough argument to be able to simply allow them now and deal with the consequences later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-repr Area: the `#[repr(stuff)]` attribute B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-design-concerns Status: There are blocking design concerns. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests