Correct alignment of atomic types and (re)add Atomic{I,U}128 #53514

ollie27 · 2018-08-20T00:47:16Z

LLVM requires that atomic loads and stores be aligned to at least the size of the type. https://llvm.org/docs/LangRef.html#id193

Increasing the alignment is incompatible with #[repr(transparent)] but fortunately that was only recently added but either this PR or a revert of #52149 would have to be backported to 1.29 beta to avoid breakage from that.

Using #[repr(align)] is a breaking change though because for some reason you can't put a #[repr(align)] struct inside a #[repr(packed)] struct. I don't know if there's a way to increase the alignment without that issue.

cc. #32976

Fixes #39590

rust-highfive · 2018-08-20T00:47:19Z

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2018-08-20T02:34:55Z

I've nominated for beta since a cursory look suggests that's necessary.

kennytm · 2018-08-20T02:49:02Z

r? @cramertj

cramertj · 2018-08-21T00:42:25Z

src/libcore/sync/atomic.rs

@@ -124,7 +124,7 @@ pub fn spin_loop_hint() {
 /// [`bool`]: ../../../std/primitive.bool.html
 #[cfg(target_has_atomic = "8")]
 #[stable(feature = "rust1", since = "1.0.0")]
-#[repr(transparent)]
+#[repr(align(1))]


align(1) doesn't do anything here, but repr(transparent) does, so I'd opt to use that here.

Yeah, this was only added for consistency. I don't know if #[repr(transparent)] is really appropriate for AtomicBool though because the internal type is actually u8 and if we wanted to support platforms that don't have byte level atomics it would have to be something bigger.

cramertj · 2018-08-21T00:58:26Z

src/libcore/sync/atomic.rs

@@ -148,7 +148,9 @@ unsafe impl Sync for AtomicBool {}
 /// This type has the same in-memory representation as a `*mut T`.
 #[cfg(target_has_atomic = "ptr")]
 #[stable(feature = "rust1", since = "1.0.0")]
-#[repr(transparent)]
+#[cfg_attr(target_pointer_width = "16", repr(align(2)))]


Do we support any platforms where size isn't already equal to alignment for *mut? This seems redundant to me, and it sacrifices the #[repr(transparent)].

I'm not aware of any but I don't know what platforms Rust might want to support in the future.

cramertj · 2018-08-21T01:00:47Z

src/libcore/sync/atomic.rs

@@ -1101,7 +1104,7 @@ macro_rules! atomic_int {
        ///
        /// [module-level documentation]: index.html
        #[$stable]
-        #[repr(transparent)]
+        #[repr(align($align))]


As above, I think that this is already always true with the exception of u/i128. I'd prefer a solution which kept repr(transparent) intact for other primitives and special-cased the 128-bit types to receive higher-alignment attributes rather than #[repr(transparent)].

u64 is only aligned to 4 on 32-bit linux systems.

@retep998 Thanks! I think I'd still prefer to special-case and allow repr(transparent) on more platforms, but that makes me unsure.

Yeah, that's the issue, it's not clear which types can be repr(transparent). That's why I just added repr(align) to all types to avoid the issue. We presumably definitely don't want the same type to have repr(transparent) on some platforms but not others so I don't really know what else to do here.

I'm not sure either. Ping @rust-lang/libs for your thoughts? This is a breaking (but necessary) change to the representation guarantees of several atomics-- it'd be nice to keep the repr(transparent) where possible, but I agree that having it be platform-dependent is pretty dodgy.

@sfackler I'd think you can still construct a *const AtomicU64 at the right address without repr(transparent)?

As long as the layout is the same, then *const AtomicU64 to arbitrary memory is still fine. Where repr(transparent) is needed is specifically for compatibility in function calling convention ABIs because two types having the same layout may not be passed the same.

My understanding is that repr(transparent) is the only thing that guarantees that the layout is the same - do we have a separate guarantee that single field structs have the same layout as their element?

When not talking about passing things around in functions by value, wouldn't that be repr(C)?

Yes, repr(C) would be one way to achieve layout compatibility. However, since this is in libstd, we can also just state that it's layout compatible and rely on the fact that rustc de facto makes the layout identical. Furthermore, I am convinced we should guarantee layout compatibility of repr(Rust) newtypes anyway.

nikomatsakis · 2018-08-30T15:39:59Z

I am not clear on whether this is @rust-lang/compiler, @rust-lang/libs, or even @rust-lang/lang, but I do agree we ought to backport if we decide to accept this PR. =) I'm going to cc @rust-lang/wg-unsafe-code-guidelines as well well I'm going crazy because it seems like the sort of thing that is related to unsafe code.

It seems like the real question at play here is:

Do we guarantee #[repr(transparent)] for atomics or do we instead guarantee some kind of alignment requirements?

We sort of can't do both because they may be in conflict? (Is that correct? That was the gist I was getting from the comments, but I don't know if there are concrete examples of this)

Is there a conservative decision we can make in the meantime which we can backport?

nikomatsakis · 2018-08-30T15:41:14Z

My feeling btw is that the ultimate decision here probably requires at least an FCP and might benefit from an RFC (since there may be subtle constraints at play we are not fully considering) — but if we can take some immediate steps (e.g., removing #[repr(transparent)]) that leave room for us to make the decision later, that would be good.

joshtriplett · 2018-08-30T16:01:13Z

For at least u16/u32/u64, it's not clear to me why we can't have both #[repr(transparent)] and #[repr(align(size-of-the-type))].

RalfJung · 2018-08-30T16:10:13Z

Removing repr(transparent) would be a good step I think. I would expect it to be rare anyway that this is required. This only makes a difference when passing things by value to FFI, right? And the point of atomics is usually to be shared and thus passed by reference.

Do we guarantee #[repr(transparent)] for atomics or do we instead guarantee some kind of alignment requirements?

Given that LLVM mandates alignment requirements, our hands are bound.

hanna-kruppe · 2018-08-30T16:11:04Z

Do we guarantee #[repr(transparent)] for atomics or do we instead guarantee some kind of alignment requirements?

repr(transparent) is a bit of a red herring here, since people only need memory layout compatibility, i.e., the ability to cast *const u32 to *const AtomicU32 or the like, not the calling conventions stuff that repr(transparent) actually adds. As I see it, the alternatives are actually:

force natural alignment of atomic types (placing some extra burden on people who type-pun them in unsafe code)
allow atomic types that are not naturally aligned, possibly ruling out some implementation strategies

The latter seems more important to me, but are there any platforms where u64 is not naturally aligned in the ABI, yet 64 bit atomics are supported and require natural alignment? Normally, 64 bit CPUs make u64 naturally aligned while 32 bit CPUs emulate in a library 64 bit atomics in software without corresponding alignment requirements. I suppose a 32 bit platform with double-word CAS might be relevant, but is that even a thing and do we care about it?

hanna-kruppe · 2018-08-30T16:16:43Z

Oh, I missed that LLVM mandates this. Then I guess it's settled. Note that this has more repercussions than just removing repr(transparent) – with or without the attribute, the heightened alignment requirement means that one can't safely cast any &u64 to &AtomicU64 and operate on it, unless align_of::<u64>() >= 8. Well, it's really only a problem on platforms where u64 is not naturally aligned yet AtomicU64 exists, which as I wrote in an earlier comment think is very uncommon.

jethrogb · 2018-08-30T16:25:09Z

I suppose a 32 bit platform with double-word CAS might be relevant, but is that even a thing and do we care about it?

The semantics of this on x86 are... unclear. See e.g. http://joeduffyblog.com/2006/12/06/clr-data-alignment-and-cmpxchg8b/ https://reviews.llvm.org/D27653

pietroalbini · 2018-09-05T22:10:04Z

Ping! This PR is beta-nominated, and if we want it to reach 1.29 stable we need to merge this in less than a week. What's the progress?

Added in rust-lang#52149 the discussion in rust-lang#53514 is showing how we may not want to actually add this attribute to the atomic types. While we continue to debate rust-lang#53514 this commit reverts the addition of the `transparent` attribute before it hits stable.

alexcrichton · 2018-09-05T22:40:50Z

Thanks for the ping @pietroalbini, I've submitted to #53978 to temporarily revert #52149 on beta-only as it should give us some more time to discuss this PR

Added in rust-lang#52149 the discussion in rust-lang#53514 is showing how we may not want to actually add this attribute to the atomic types. While we continue to debate rust-lang#53514 this commit reverts the addition of the `transparent` attribute. This should be a more conservative route which leaves us the ability to tweak this in the future but in the meantime allows us to continue discussion as well.

alexcrichton · 2018-09-05T22:42:50Z

I've also sent #53979 for the master branch

…r=nikomatsakis [beta]: Remove `#[repr(transparent)]` from atomics Added in #52149 the discussion in #53514 is showing how we may not want to actually add this attribute to the atomic types. While we continue to debate #53514 this commit reverts the addition of the `transparent` attribute before it hits stable.

cramertj · 2018-10-01T18:27:31Z

@Aaronepower #53979 already landed removing the #[repr(transparent)]. I think the discussion now is just about whether or not we should provide #[repr(transparent)] for these types on architectures that support it.

pnkfelix · 2018-10-11T14:56:08Z

discussed briefly at T-compiler meeting. We cannot make a spot decision on this without more careful review. Re-assigning to @nagisa for now to make a summary comment (potentially linking to other relevant bugs) for the rest of the compiler team to read.

nikomatsakis · 2018-10-11T14:56:35Z

I'm a bit confused about what the points of decision are here. Can someone maybe crystallize into the key questions at hand?

My impression from @rkruppe's comment:

Oh, I missed that LLVM mandates this. Then I guess it's settled. Note that this has more repercussions than just removing repr(transparent) – with or without the attribute, the heightened alignment requirement means that one can't safely cast any &u64 to &AtomicU64 and operate on it, unless align_of::() >= 8. Well, it's really only a problem on platforms where u64 is not naturally aligned yet AtomicU64 exists, which as I wrote in an earlier comment think is very uncommon.

Is that (a) we really just have to do whatever LLVM mandates and (b) this may mean that &mut T -> &AtomicT casts are not generally safe across platforms. If that's the case, can we just change the alignment and try to document the alignment requirements (and which platforms meet those restrictions by default)?

Also note that &T -> &AtomicT is just...never safe to do, despite the many comments writing it. I presume it is some kind of shorthand? An e.g. &u64 is guaranteed not to change, or am I missing something.

cramertj · 2018-10-11T17:41:15Z

@nikomatsakis

Also note that &T -> &AtomicT is just...never safe to do, despite the many comments writing it. I presume it is some kind of shorthand? An e.g. &u64 is guaranteed not to change, or am I missing something.

I think you can s/&T -> &AtomicT/&mut T -> &AtomicT/g for the purposes of this conversation and the discussion all still applies.

nagisa · 2018-10-11T18:06:55Z

So, I was tasked with summarizing this. Here it goes (tis gonna be a long one).

First, there is one critical requirement that we cannot avoid to keep this correct:

For atomic operations to be valid on x: *mut T, x.align_offset(size_of::<T>) must be equal to 0. In other words, the pointer on which atomic operations are done must be aligned to at least the size of T.

There’s no way around it, even if alignment of the type is less than its size (e.g. i128 in Rust currently is aligned to 8 bytes, on some targets i64 may be aligned to 4 bytes etc). It is user’s responsibility to ensure the correct alignment.

In C/C++ land this is solved by having types atomic_T_t and std::atomic<T> which have proper alignments and otherwise satisfy all platform-specific constraints. Since Rust:

Exposes a safe UB-less API for this functionality; and
Does not expose a way for the user to ensure these requirements are met…

The safe API must by all means ensure that these requirements (and all other machine-specific requirements) are met by itself. With that out of the way… What follows are some of the options:

We could simply "just" specify repr(align) and not guarantee any sort of representational equivalency between Rust’s atomics and C atomics. This would make atomic operations in FFI context roughly impossible.

This is the approach taken by this PR.

Make AtomicT have both repr(transparent) and repr(align(correct value))

Only if we include alignment into list of type “equivalency” requirements. On one hand, that would mean that u8/uint8_t and #[repr(transparent, align(2)] struct U8(u8) would no longer be types with equivalent representation. On the other hand, C (gcc) provides ability to create a type equivalent to U8 via typedef uint8_t more_aligned_uint8_t __attribute__ ((aligned (2))); and Rust cannot represent such type at all.

Making this (dual repr attributes) possible would solve all the problems with atomics, AFAIK.

Make a way to convert from *mut atomic_T_t to &AtomicT

In order to use atomics in an FFI context it is critical to make it possible to execute atomic operations on pointers obtained over the FFI boundary. After all the whole purpose of the atomic operations is to work on areas of memory, not on values specifically.

The question then is, how do we expose atomic operations for pointers obtained via FFI? One option is to allow (the fairly unsafe) equivalence between (i.e. the proposal above):

/* in C */ extern void my_atomic_operation(atomic_T_t *);
/* in Rust */ extern "C" fn my_atomic_operation(&AtomicT);

Another option is to have simply:

impl AtomicT { 
    // UB if pointer is not appropriately aligned or does not satisfy any other machine-specific requirements. Doing this with `T = atomic_T_t` should always be valid.
    unsafe fn from_ptr(x: *mut T) -> &AtomicT { ... }
}

Either way, some sort of representational equivalency between &AtomicT and C’s atomic_T_t is necessary.

It is important to remember that the alignment for the atomic instruction itself (in generated LLVM code) must be specified and must satisfy the requirements mentioned above. This means that the way we call the atomic intrinsics (such as this) must change as well (i.e. instead of being called with e.g. T = u8, they should be called with T = atomic_u8_t or T = AtomicT).

This made me think of another option of which I haven’t thought of before, which is changing the implementation of the Atomic itself (for a concrete example using AtomicU8) from:

/* #[ ... ] */
struct AtomicU8 {
    v: UnsafeCell<u8>
}

to

#[repr(align(correct))]
newtype atomic_u8_t = /*???*/;

#[repr(transparent)]
struct AtomicU8 {
    v: UnsafeCell<atomic_u8_t>
}

EDIT: removed "natural" from "natural alignment" as per comment below.

hanna-kruppe · 2018-10-11T19:25:58Z

There’s no way around it, even if natural alignment of the type is less than its size (e.g. i128 in Rust currently is naturally aligned to 8 bytes, on some targets i64 may be naturally aligned to 4 bytes etc). It is user’s responsibility to ensure the correct alignment.

Terminology nit: You mean ABI alignment here. "Natural alignment" means alignment equal to the data type size.

Make AtomicT have both repr(transparent) and repr(align(correct value))

repr(transparent) is only relevant when passing AtomicT by value, but that is not useful because (as also noted elsewhere in @nagisa's post), in FFI one passes pointers to a shared memory location that should be accessed atomically. All that's needed for FFI is equal alignment and memory layout, which is trivially achieved with repr(C), side stepping the need to decide anything about repr(transparent, align(N)) (which has been suggested multiple times during the RFC and stabilization process and been rejected each time).

For the record, for the definition in libcore we could also merge this PR as-is. We don't currently guarantee that a struct AtomicT { x: T } is laid out exactly as T, but it's always been true in rustc (and we may also decide to guarantee it to users cc rust-lang/unsafe-code-guidelines#34).

It is important to remember that the alignment for the atomic instruction itself (in generated LLVM code) must be specified and must satisfy the requirements mentioned above. This means that the way we call the atomic intrinsics (such as this) must change as well (i.e. instead of being called with e.g. T = u8, they should be called with T = atomic_u8_t or T = AtomicT).

We argued about this before (in this thread and on IRC too, I believe) but I still believe the second half of this, particularly the choice of "must", is wrong. We can simply amend the definition of these intrinsics to require suitable alignment with no change to the signature.

RalfJung · 2018-10-12T07:05:54Z

We could simply "just" specify repr(align) and not guarantee any sort of representational equivalency between Rust’s atomics and C atomics. This would make atomic operations in FFI context roughly impossible.

Is that true? It's still a newtype, just without repr(transparent). So pointers to atomics are compatible with pointers to equally-aligned C atomics (if we assume the newtype guarantees that are already relied upon in several places). Just passing them by value across FFI is not okay, but that would be a rather strange thing to do for an atomic (you want to share it by reference, not value).

Basically, I agree with @rkruppe.

nagisa · 2018-10-12T10:38:22Z

Is that true? It's still a newtype, just without repr(transparent).

I do not trust transmute::<&Y>(&x) and &*x.y.get() to be equivalent, given #[repr(Rust, align(A))] struct X { y: UnsafeCell<Y> } and let x; X;. This property is necessary to make conversion from *mut T to &AtomicT possible, which by extension makes operations on FFI stuff possible at all.

I’m not sure of what newtype guarantees you’re speaking about.

RalfJung · 2018-10-12T11:05:17Z

I’m not sure of what newtype guarantees you’re speaking about.

Basically, newtypes having the same layout (size and field offsets) as the type they wrap. Like, your X having the same layout as UnsafeCell<Y>. We don't guarantee this for repr(Rust), but we might. If you use repr(C), I think we have to guarantee it -- I assume it's what the C ABI mandates?

So just using #[repr(C, align(A))] should be sufficient.

pnkfelix · 2018-10-18T14:59:14Z

Discussed at compiler team meeting.

After reviewing @nagisa 's summary comment and the follow-on comments, we are inclined to try to experiment with using #[repr(C, align(X))] to resolve this.

@nagisa has agreed to take point on ushering in that change to this PR.

nagisa · 2018-10-18T15:00:25Z

This is the link to the discussion.

nikomatsakis · 2018-10-18T15:00:30Z

@nagisa

I do not trust transmute::<&Y>(&x) and &*x.y.get() to be equivalent, given #[repr(Rust, align(A))] struct X { y: UnsafeCell<Y> } and let x; X;.

Why not? What are the steps required to make that true:

UnsafeCell<Y> has to have the same size and alignment as Y (seems like that should obviously be something we guarantee)
same with single-field structs like struct X { y: UnsafeCell<Y> } (I also this is pretty clearly a good idea)

Is there some other subtlety involved?

nagisa · 2018-10-18T15:04:05Z

@nikomatsakis my distrust with that comes specifically from repr(Rust) not being specified and therefore assert_eq!(&x as *const _, &x.y as *const _) not necessarily being true.

While I agree that “newtypes” having equivalent memory layout as the types they contain seem like a sensible idea, it is not something that we ever explicitly specified and making unsafe code to rely on such unwritten rule is scary.

nikomatsakis · 2018-10-18T18:28:34Z

OK. I feel like it is within our power, however, to reach this decision. =) (Not on this PR per se.)

TimNN · 2018-10-30T17:25:35Z

Ping from triage! It looks like this PR has been superseded by #55410, so I'm closing it. If my understanding is incorrect, please re-open.

@pnkfelix

Correct alignment of atomic types and (re)add Atomic{I,U}128 This is a updated #53514 to also make atomic types `repr(C)` as per comment in #53514 (comment). Fixes #39590 Closes #53514 r? @pnkfelix

rust-highfive assigned kennytm Aug 20, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2018

Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 20, 2018

rust-highfive assigned cramertj and unassigned kennytm Aug 20, 2018

cramertj reviewed Aug 21, 2018

View reviewed changes

pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 25, 2018

alexcrichton mentioned this pull request Sep 5, 2018

[beta]: Remove #[repr(transparent)] from atomics #53978

Merged

pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 5, 2018

alexcrichton mentioned this pull request Sep 5, 2018

Remove #[repr(transparent)] from atomics #53979

Merged

pnkfelix assigned nagisa Oct 11, 2018

RalfJung mentioned this pull request Oct 12, 2018

Check atomic access alignment requirements rust-lang/miri#475

Closed

pnkfelix removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 18, 2018

nikomatsakis mentioned this pull request Oct 18, 2018

Layout of single-field structs rust-lang/unsafe-code-guidelines#34

Closed

TimNN added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 23, 2018

nagisa mentioned this pull request Oct 27, 2018

Correct alignment of atomic types and (re)add Atomic{I,U}128 #55410

Merged

TimNN closed this Oct 30, 2018

ollie27 deleted the atomic branch November 6, 2018 14:06

Correct alignment of atomic types and (re)add Atomic{I,U}128 #53514

Correct alignment of atomic types and (re)add Atomic{I,U}128 #53514

Conversation

ollie27 commented Aug 20, 2018

rust-highfive commented Aug 20, 2018

Mark-Simulacrum commented Aug 20, 2018

kennytm commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cramertj Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanna-kruppe Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Aug 30, 2018 • edited Loading

nikomatsakis commented Aug 30, 2018

joshtriplett commented Aug 30, 2018

RalfJung commented Aug 30, 2018 • edited Loading

hanna-kruppe commented Aug 30, 2018

hanna-kruppe commented Aug 30, 2018

jethrogb commented Aug 30, 2018 • edited Loading

pietroalbini commented Sep 5, 2018

alexcrichton commented Sep 5, 2018

alexcrichton commented Sep 5, 2018

cramertj commented Oct 1, 2018

pnkfelix commented Oct 11, 2018

nikomatsakis commented Oct 11, 2018

cramertj commented Oct 11, 2018

nagisa commented Oct 11, 2018 • edited Loading

hanna-kruppe commented Oct 11, 2018

RalfJung commented Oct 12, 2018

nagisa commented Oct 12, 2018 • edited Loading

RalfJung commented Oct 12, 2018

pnkfelix commented Oct 18, 2018

nagisa commented Oct 18, 2018

nikomatsakis commented Oct 18, 2018

nagisa commented Oct 18, 2018

nikomatsakis commented Oct 18, 2018

TimNN commented Oct 30, 2018

cramertj Aug 21, 2018 •

edited

Loading

hanna-kruppe Aug 30, 2018 •

edited

Loading

nikomatsakis commented Aug 30, 2018 •

edited

Loading

RalfJung commented Aug 30, 2018 •

edited

Loading

jethrogb commented Aug 30, 2018 •

edited

Loading

nagisa commented Oct 11, 2018 •

edited

Loading

nagisa commented Oct 12, 2018 •

edited

Loading