Auto merge of #55410 - nagisa:atomic-align, r=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-lang · Nov 5, 2018 · 13dab66 · 13dab66
2 parents af791bb + 99f7dc4
commit 13dab66
Show file tree

Hide file tree

Showing 8 changed files with 169 additions and 20 deletions.
diff --git a/src/libcore/sync/atomic.rs b/src/libcore/sync/atomic.rs
@@ -124,6 +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(C, align(1))]
 pub struct AtomicBool {
     v: UnsafeCell<u8>,
 }
@@ -147,6 +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")]
+#[cfg_attr(target_pointer_width = "16", repr(C, align(2)))]
+#[cfg_attr(target_pointer_width = "32", repr(C, align(4)))]
+#[cfg_attr(target_pointer_width = "64", repr(C, align(8)))]
 pub struct AtomicPtr<T> {
     p: UnsafeCell<*mut T>,
 }
@@ -1088,6 +1092,7 @@ macro_rules! atomic_int {
      $s_int_type:expr, $int_ref:expr,
      $extra_feature:expr,
      $min_fn:ident, $max_fn:ident,
+     $align:expr,
      $int_type:ident $atomic_type:ident $atomic_init:ident) => {
         /// An integer type which can be safely shared between threads.
         ///
@@ -1101,6 +1106,7 @@ macro_rules! atomic_int {
         ///
         /// [module-level documentation]: index.html
         #[$stable]
+        #[repr(C, align($align))]
         pub struct $atomic_type {
             v: UnsafeCell<$int_type>,
         }
@@ -1831,6 +1837,7 @@ atomic_int! {
     "i8", "../../../std/primitive.i8.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_min, atomic_max,
+    1,
     i8 AtomicI8 ATOMIC_I8_INIT
 }
 #[cfg(target_has_atomic = "8")]
@@ -1844,6 +1851,7 @@ atomic_int! {
     "u8", "../../../std/primitive.u8.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_umin, atomic_umax,
+    1,
     u8 AtomicU8 ATOMIC_U8_INIT
 }
 #[cfg(target_has_atomic = "16")]
@@ -1857,6 +1865,7 @@ atomic_int! {
     "i16", "../../../std/primitive.i16.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_min, atomic_max,
+    2,
     i16 AtomicI16 ATOMIC_I16_INIT
 }
 #[cfg(target_has_atomic = "16")]
@@ -1870,6 +1879,7 @@ atomic_int! {
     "u16", "../../../std/primitive.u16.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_umin, atomic_umax,
+    2,
     u16 AtomicU16 ATOMIC_U16_INIT
 }
 #[cfg(target_has_atomic = "32")]
@@ -1883,6 +1893,7 @@ atomic_int! {
     "i32", "../../../std/primitive.i32.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_min, atomic_max,
+    4,
     i32 AtomicI32 ATOMIC_I32_INIT
 }
 #[cfg(target_has_atomic = "32")]
@@ -1896,6 +1907,7 @@ atomic_int! {
     "u32", "../../../std/primitive.u32.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_umin, atomic_umax,
+    4,
     u32 AtomicU32 ATOMIC_U32_INIT
 }
 #[cfg(target_has_atomic = "64")]
@@ -1909,6 +1921,7 @@ atomic_int! {
     "i64", "../../../std/primitive.i64.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_min, atomic_max,
+    8,
     i64 AtomicI64 ATOMIC_I64_INIT
 }
 #[cfg(target_has_atomic = "64")]
@@ -1922,8 +1935,49 @@ atomic_int! {
     "u64", "../../../std/primitive.u64.html",
     "#![feature(integer_atomics)]\n\n",
     atomic_umin, atomic_umax,
+    8,
     u64 AtomicU64 ATOMIC_U64_INIT
 }
+#[cfg(all(not(stage0), target_has_atomic = "128"))]
+atomic_int! {
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    "i128", "../../../std/primitive.i128.html",
+    "#![feature(integer_atomics)]\n\n",
+    atomic_min, atomic_max,
+    16,
+    i128 AtomicI128 ATOMIC_I128_INIT
+}
+#[cfg(all(not(stage0), target_has_atomic = "128"))]
+atomic_int! {
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    unstable(feature = "integer_atomics", issue = "32976"),
+    "u128", "../../../std/primitive.u128.html",
+    "#![feature(integer_atomics)]\n\n",
+    atomic_umin, atomic_umax,
+    16,
+    u128 AtomicU128 ATOMIC_U128_INIT
+}
+#[cfg(target_pointer_width = "16")]
+macro_rules! ptr_width {
+    () => { 2 }
+}
+#[cfg(target_pointer_width = "32")]
+macro_rules! ptr_width {
+    () => { 4 }
+}
+#[cfg(target_pointer_width = "64")]
+macro_rules! ptr_width {
+    () => { 8 }
+}
 #[cfg(target_has_atomic = "ptr")]
 atomic_int!{
     stable(feature = "rust1", since = "1.0.0"),
@@ -1935,6 +1989,7 @@ atomic_int!{
     "isize", "../../../std/primitive.isize.html",
     "",
     atomic_min, atomic_max,
+    ptr_width!(),
     isize AtomicIsize ATOMIC_ISIZE_INIT
 }
 #[cfg(target_has_atomic = "ptr")]
@@ -1948,6 +2003,7 @@ atomic_int!{
     "usize", "../../../std/primitive.usize.html",
     "",
     atomic_umin, atomic_umax,
+    ptr_width!(),
     usize AtomicUsize ATOMIC_USIZE_INIT
 }
 

diff --git a/src/librustc_codegen_llvm/builder.rs b/src/librustc_codegen_llvm/builder.rs
@@ -482,14 +482,12 @@ impl Builder<'a, 'll, 'tcx> {
         }
     }
 
-    pub fn atomic_load(&self, ptr: &'ll Value, order: AtomicOrdering, align: Align) -> &'ll Value {
+    pub fn atomic_load(&self, ptr: &'ll Value, order: AtomicOrdering, size: Size) -> &'ll Value {
         self.count_insn("load.atomic");
         unsafe {
             let load = llvm::LLVMRustBuildAtomicLoad(self.llbuilder, ptr, noname(), order);
-            // FIXME(eddyb) Isn't it UB to use `pref` instead of `abi` here?
-            // However, 64-bit atomic loads on `i686-apple-darwin` appear to
-            // require `___atomic_load` with ABI-alignment, so it's staying.
-            llvm::LLVMSetAlignment(load, align.pref() as c_uint);
+            // LLVM requires the alignment of atomic loads to be at least the size of the type.
+            llvm::LLVMSetAlignment(load, size.bytes() as c_uint);
             load
         }
     }
@@ -564,15 +562,14 @@ impl Builder<'a, 'll, 'tcx> {
     }
 
     pub fn atomic_store(&self, val: &'ll Value, ptr: &'ll Value,
-                        order: AtomicOrdering, align: Align) {
+                        order: AtomicOrdering, size: Size) {
         debug!("Store {:?} -> {:?}", val, ptr);
         self.count_insn("store.atomic");
         let ptr = self.check_store(val, ptr);
         unsafe {
             let store = llvm::LLVMRustBuildAtomicStore(self.llbuilder, val, ptr, order);
-            // FIXME(eddyb) Isn't it UB to use `pref` instead of `abi` here?
-            // Also see `atomic_load` for more context.
-            llvm::LLVMSetAlignment(store, align.pref() as c_uint);
+            // LLVM requires the alignment of atomic stores to be at least the size of the type.
+            llvm::LLVMSetAlignment(store, size.bytes() as c_uint);
         }
     }
 

diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs
@@ -477,8 +477,8 @@ pub fn codegen_intrinsic_call(
                 "load" => {
                     let ty = substs.type_at(0);
                     if int_type_width_signed(ty, cx).is_some() {
-                        let align = cx.align_of(ty);
-                        bx.atomic_load(args[0].immediate(), order, align)
+                        let size = cx.size_of(ty);
+                        bx.atomic_load(args[0].immediate(), order, size)
                     } else {
                         return invalid_monomorphization(ty);
                     }
@@ -487,8 +487,8 @@ pub fn codegen_intrinsic_call(
                 "store" => {
                     let ty = substs.type_at(0);
                     if int_type_width_signed(ty, cx).is_some() {
-                        let align = cx.align_of(ty);
-                        bx.atomic_store(args[1].immediate(), args[0].immediate(), order, align);
+                        let size = cx.size_of(ty);
+                        bx.atomic_store(args[1].immediate(), args[0].immediate(), order, size);
                         return;
                     } else {
                         return invalid_monomorphization(ty);

diff --git a/src/libstd/panic.rs b/src/libstd/panic.rs
@@ -264,6 +264,9 @@ impl RefUnwindSafe for atomic::AtomicI32 {}
 #[cfg(target_has_atomic = "64")]
 #[unstable(feature = "integer_atomics", issue = "32976")]
 impl RefUnwindSafe for atomic::AtomicI64 {}
+#[cfg(all(not(stage0), target_has_atomic = "128"))]
+#[unstable(feature = "integer_atomics", issue = "32976")]
+impl RefUnwindSafe for atomic::AtomicI128 {}
 
 #[cfg(target_has_atomic = "ptr")]
 #[stable(feature = "unwind_safe_atomic_refs", since = "1.14.0")]
@@ -280,6 +283,9 @@ impl RefUnwindSafe for atomic::AtomicU32 {}
 #[cfg(target_has_atomic = "64")]
 #[unstable(feature = "integer_atomics", issue = "32976")]
 impl RefUnwindSafe for atomic::AtomicU64 {}
+#[cfg(all(not(stage0), target_has_atomic = "128"))]
+#[unstable(feature = "integer_atomics", issue = "32976")]
+impl RefUnwindSafe for atomic::AtomicU128 {}
 
 #[cfg(target_has_atomic = "8")]
 #[stable(feature = "unwind_safe_atomic_refs", since = "1.14.0")]

diff --git a/src/test/run-make-fulldeps/atomic-lock-free/atomic_lock_free.rs b/src/test/run-make-fulldeps/atomic-lock-free/atomic_lock_free.rs
@@ -58,6 +58,14 @@ pub unsafe fn atomic_u64(x: *mut u64) {
 pub unsafe fn atomic_i64(x: *mut i64) {
     atomic_xadd(x, 1);
 }
+#[cfg(target_has_atomic = "128")]
+pub unsafe fn atomic_u128(x: *mut u128) {
+    atomic_xadd(x, 1);
+}
+#[cfg(target_has_atomic = "128")]
+pub unsafe fn atomic_i128(x: *mut i128) {
+    atomic_xadd(x, 1);
+}
 #[cfg(target_has_atomic = "ptr")]
 pub unsafe fn atomic_usize(x: *mut usize) {
     atomic_xadd(x, 1);

diff --git a/src/test/run-pass/atomic-alignment.rs b/src/test/run-pass/atomic-alignment.rs
@@ -0,0 +1,46 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(cfg_target_has_atomic)]
+#![feature(integer_atomics)]
+
+use std::mem::{align_of, size_of};
+use std::sync::atomic::*;
+
+fn main() {
+    #[cfg(target_has_atomic = "8")]
+    assert_eq!(align_of::<AtomicBool>(), size_of::<AtomicBool>());
+    #[cfg(target_has_atomic = "ptr")]
+    assert_eq!(align_of::<AtomicPtr<u8>>(), size_of::<AtomicPtr<u8>>());
+    #[cfg(target_has_atomic = "8")]
+    assert_eq!(align_of::<AtomicU8>(), size_of::<AtomicU8>());
+    #[cfg(target_has_atomic = "8")]
+    assert_eq!(align_of::<AtomicI8>(), size_of::<AtomicI8>());
+    #[cfg(target_has_atomic = "16")]
+    assert_eq!(align_of::<AtomicU16>(), size_of::<AtomicU16>());
+    #[cfg(target_has_atomic = "16")]
+    assert_eq!(align_of::<AtomicI16>(), size_of::<AtomicI16>());
+    #[cfg(target_has_atomic = "32")]
+    assert_eq!(align_of::<AtomicU32>(), size_of::<AtomicU32>());
+    #[cfg(target_has_atomic = "32")]
+    assert_eq!(align_of::<AtomicI32>(), size_of::<AtomicI32>());
+    #[cfg(target_has_atomic = "64")]
+    assert_eq!(align_of::<AtomicU64>(), size_of::<AtomicU64>());
+    #[cfg(target_has_atomic = "64")]
+    assert_eq!(align_of::<AtomicI64>(), size_of::<AtomicI64>());
+    #[cfg(target_has_atomic = "128")]
+    assert_eq!(align_of::<AtomicU128>(), size_of::<AtomicU128>());
+    #[cfg(target_has_atomic = "128")]
+    assert_eq!(align_of::<AtomicI128>(), size_of::<AtomicI128>());
+    #[cfg(target_has_atomic = "ptr")]
+    assert_eq!(align_of::<AtomicUsize>(), size_of::<AtomicUsize>());
+    #[cfg(target_has_atomic = "ptr")]
+    assert_eq!(align_of::<AtomicIsize>(), size_of::<AtomicIsize>());
+}
diff --git a/src/test/ui/feature-gates/feature-gate-cfg-target-has-atomic.rs b/src/test/ui/feature-gates/feature-gate-cfg-target-has-atomic.rs
@@ -61,6 +61,16 @@ pub unsafe fn atomic_u64(x: *mut u64) {
 pub unsafe fn atomic_i64(x: *mut i64) {
     atomic_xadd(x, 1);
 }
+#[cfg(target_has_atomic = "128")]
+//~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
+pub unsafe fn atomic_u128(x: *mut u128) {
+    atomic_xadd(x, 1);
+}
+#[cfg(target_has_atomic = "128")]
+//~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
+pub unsafe fn atomic_i128(x: *mut i128) {
+    atomic_xadd(x, 1);
+}
 #[cfg(target_has_atomic = "ptr")]
 //~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
 pub unsafe fn atomic_usize(x: *mut usize) {
@@ -81,6 +91,8 @@ fn main() {
     //~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
     cfg!(target_has_atomic = "64");
     //~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
+    cfg!(target_has_atomic = "128");
+    //~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
     cfg!(target_has_atomic = "ptr");
     //~^ ERROR `cfg(target_has_atomic)` is experimental and subject to change (see issue #32976)
 }