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 1758: Specify `repr(transparent)` #43036

Closed
aturon opened this Issue Jul 3, 2017 · 55 comments

Comments

Projects
None yet
@aturon
Copy link
Member

aturon commented Jul 3, 2017

RFC

This is a tracking issue for the RFC "Specify repr(transparent)" (rust-lang/rfcs#1758).

Steps:

@aturon aturon changed the title Tracking issue for RFC 1758: Specify Tracking issue for RFC 1758: Specify `repr(transparent)` Jul 3, 2017

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Jul 3, 2017

Mentoring instructions unavailable. This is sadly not that easy, as we have too much code that still relies on assumptions that such types do not exist. I have an in-progress branch which requires implementing the necessary support for "newtype unpacking" but no time frame for completion.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jul 3, 2017

Is making types like Cell repr(transparent) (which e.g. rust-lang/rfcs#1789 relies on) part of the implementation of the RFC or would that require its own RFC?

@aturon

This comment has been minimized.

Copy link
Member Author

aturon commented Jul 3, 2017

@RalfJung For a change at that level, a PR approved by the libs team suffices.

emilio added a commit to emilio/clang-sys that referenced this issue Oct 9, 2017

lib: Stop using bitflags to declare FFI definitions.
Using bitflags for FFI is unsound in linux32, and it causes crashes like
https://bugzilla.mozilla.org/show_bug.cgi?id=1406952.

This is pretty similar to
rust-lang/rust-bindgen#439.

For this to be properly type safe we need to wait for
rust-lang/rust#43036.

Hopefully that's not for too long.

KyleMayes added a commit to KyleMayes/clang-sys that referenced this issue Oct 11, 2017

Replace usage of bitflags with constants
Using bitflags for FFI is unsound in linux32, and it causes crashes like
https://bugzilla.mozilla.org/show_bug.cgi?id=1406952.

This is pretty similar to
rust-lang/rust-bindgen#439.

For this to be properly type safe we need to wait for
rust-lang/rust#43036.

Hopefully that's not for too long.

@mbrubeck mbrubeck referenced this issue Oct 23, 2017

Closed

Implement Borrow #1

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Dec 16, 2017

The RFC makes contradictory statements about the interaction with repr(align(N)): The Motivation section says

Given this attribute delegates all representation concerns, no other repr attribute should be present on the type. This means the following definitions are illegal:

#[repr(transparent, align = "128")]
struct BogusAlign(f64);

#[repr(transparent, packed)]
struct BogusPacked(f64);

... while the Detailed Design section says:

This new representation cannot be used with any other representation attribute but alignment, to be able to specify a transparent wrapper with additional alignment constraints:

#[repr(transparent, align = "128")]
struct OverAligned(f64); // Behaves as a bare f64 with 128 bits alignment.

#[repr(C, transparent)]
struct BogusRepr(f64); // Nonsensical, repr cannot be C and transparent.

AFAICT the RFC initially prohibited repr(transparent, align), then in the discussion some people asked for it to be allowed, but later the rationale for that was called into question and there was no consensus or (explicit) decision. So it's hard to see which alternative was intended to be the final state by the RFC author and the lang team.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 16, 2017

I'd disallow it since it significantly complicates the implementation IMO. Is there any motivation for allowing the combination?

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Dec 16, 2017

Discussion is mostly rust-lang/rfcs#1758 (comment) plus the immediate replies. The motivation is not immediately clear to me, just that a nested wrapper type with align(N) and no transparent is not equivalent to a single type with repr(align(N), transparent). Perhaps @briansmith or @nox could elaborate?

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Dec 16, 2017

@nox tells me on IRC they have no use case for it. Let's amend the RFC to consistently reject repr(transparent, align). If someone does find a use case, they can deal with the implementation effort and amend the RFC again.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Dec 16, 2017

Sorry, it's been forever since we had that conversation. Maybe I'm overlooking something, but I think #[repr(transparent, align=16)] would be very common. For example:

#[repr(transparent, align=16)]
struct Aes256Key([u8; 32]);

#[repr(transparent, align=16)]
struct AesNonce([u8; 16]);

// Keep this in sync with the C definition in aes.h
#[repr(C)]
struct Aes256Context {
    key: Aes256Key,
    ...
    nonce: AesNonce,
    ...
}

extern aes256Encrypt(context: &mut Aes256Context, ...);

I'm not sure how one would express this otherwise.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 16, 2017

Is there any ABI where you need #[repr(transparent)] in conjunction with an aggregate type (e.g. a non-transparent struct, or in your case, an array)?
AFAIK, none of the ABIs we support today has such a distinction, so #[repr(transparent)] wouldn't really have to do anything to be correct in those cases, but maybe I'm wrong?

EDIT: Also, are you ever passing or returning Aes256Key or AesNonce by value to/from a function? That's the only time #[repr(transparent)] actually does anything.
The calling convention part of the ABI is orthogonal to the memory layout, and the latter is identical between a struct with one field and the field itself, pretty much everywhere.

@briansmith

This comment has been minimized.

Copy link

briansmith commented Dec 17, 2017

Is there any ABI where you need #[repr(transparent)] in conjunction with an aggregate type (e.g. a non-transparent struct, or in your case, an array)?

Consider:

typedef uint8_t AES256Key[32];
typedef uint8_t AES128Key[16];
typedef uint8_t AESNonce[16];

struct Aes256Context {
    alignas(16) AES256Key key;
    alignas(16) AES256Nonce nonce;
};

Notice in particular that in this C code AES128Key and AES128Nonce are both aliases for uint8_t[16] and so they can be used (unfortunately) interchangeably. In my Rust code, I want to use the newtype pattern to make AES128Key and AES128Nonce non-interchangeable types. AFAICT, this requires using #repr[transparenent)], e.g. #repr[transparenent)] struct AES128Key([u8; 16]). In this codebase it is also required (for performance and/or correctness reasons, depending on platform) that everything be 128-bit aligned because of assumptions made by the hand-coded assembly code that uses these structures.

AFAIK, none of the ABIs we support today has such a distinction, so #[repr(transparent)] wouldn't really have to do anything to be correct in those cases, but maybe I'm wrong?

EDIT: Also, are you ever passing or returning Aes256Key or AesNonce by value to/from a function? That's the only time #[repr(transparent)] actually does anything.

Is that true? Maybe I'm misunderstanding the purpose of #[repr(transparent)], but I think that #[repr(transparent)] is supposed to do more than change how things are passed by value. Consider:

struct Outer1 {
    struct Wrapper value1;
    struct Wrapper value2; 
};

struct Wrapper {
    uint8_t value[12];
};

struct Outer2 {
    uint8_t value1[12];
    uint8_t value2[12];
};

Are these structures guaranteed, by C, to have the same memory layout? I don't remember the details but I think the answer is "no". And that's why #repr(transparent) matters for more than by-value argument passing semantics; it allows us to make a Wrapper type in our Rust bindings to that is transparent w.r.t. C semantics.

The calling convention part of the ABI is orthogonal to the memory layout, and the latter is identical between a struct with one field and the field itself, pretty much everywhere.

I think a struct with one field, like Wrapper above, is allowed to have additional padding that isn't necessarily added when there is no such struct. For example, in my example, I think it's allowed for offsetof(Outer1, value2) == 16 && offsetof(Outer2, value2) == 12.

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 17, 2017

@briansmith That would imply Wrapper has non-byte alignment and we don't currently support any architecture where that is the case, are you aware of any out there?

EDIT: also, it looks like the correct translation of the C code would be #[repr(align(16))] on fields, which we could reasonably add support for.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jun 2, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 6, 2018

Stabilization PR: #51395
Reference PR: rust-lang-nursery/reference#353

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jun 6, 2018

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jun 8, 2018

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jun 8, 2018

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jun 9, 2018

SimonSapin added a commit to SimonSapin/rust that referenced this issue Jun 12, 2018

bors added a commit that referenced this issue Jun 14, 2018

Auto merge of #51562 - SimonSapin:transparent, r=cramertj
Stabilize #[repr(transparent)]

Tracking issue FCP: #43036 (comment)
Reference PR: rust-lang-nursery/reference#353

bors added a commit that referenced this issue Jun 15, 2018

Auto merge of #51562 - SimonSapin:transparent, r=cramertj
Stabilize #[repr(transparent)]

Tracking issue FCP: #43036 (comment)
Reference PR: rust-lang-nursery/reference#353

bors added a commit that referenced this issue Jun 16, 2018

Auto merge of #51562 - SimonSapin:transparent, r=cramertj
Stabilize #[repr(transparent)]

Tracking issue FCP: #43036 (comment)
Reference PR: rust-lang-nursery/reference#353

bors added a commit that referenced this issue Jun 16, 2018

Auto merge of #51562 - SimonSapin:transparent, r=cramertj
Stabilize #[repr(transparent)]

Tracking issue FCP: #43036 (comment)
Reference PR: rust-lang-nursery/reference#353
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jun 26, 2018

Oops, this should have been closed by #51562.

@SimonSapin SimonSapin closed this Jun 26, 2018

bors added a commit that referenced this issue Jul 3, 2018

Auto merge of #51395 - SimonSapin:repr-transparent, r=SimonSapin
Add #[repr(transparent)] to some libcore types

* `UnsafeCell`
* `Cell`
* `NonZero*`
* `NonNull`
* `Unique`

CC #43036

bors added a commit that referenced this issue Jul 4, 2018

Auto merge of #51395 - SimonSapin:repr-transparent, r=SimonSapin
Add #[repr(transparent)] to some libcore types

* `UnsafeCell`
* `Cell`
* `NonZero*`
* `NonNull`
* `Unique`

CC #43036
@RReverser

This comment has been minimized.

Copy link
Contributor

RReverser commented Aug 5, 2018

Could I ask here for some eyes on a suggestion to add #[repr(transparent)] to Box<T> and whether there are any obvious downsides? Thanks! #52976

@GabrielMajeri GabrielMajeri referenced this issue Aug 28, 2018

Merged

WIP: Implement builder-like setters #117

3 of 3 tasks complete

jD91mZM2 added a commit to jD91mZM2/rust that referenced this issue Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.