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

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions src/libcore/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(align(1))]
pub struct AtomicBool {
v: UnsafeCell<u8>,
}
Expand All @@ -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(align(2)))]
#[cfg_attr(target_pointer_width = "32", repr(align(4)))]
#[cfg_attr(target_pointer_width = "64", repr(align(8)))]
pub struct AtomicPtr<T> {
p: UnsafeCell<*mut T>,
}
Expand Down Expand Up @@ -1086,6 +1090,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.
///
Expand All @@ -1099,6 +1104,7 @@ macro_rules! atomic_int {
///
/// [module-level documentation]: index.html
#[$stable]
#[repr(align($align))]
pub struct $atomic_type {
v: UnsafeCell<$int_type>,
}
Expand Down Expand Up @@ -1819,6 +1825,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")]
Expand All @@ -1832,6 +1839,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")]
Expand All @@ -1845,6 +1853,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")]
Expand All @@ -1858,6 +1867,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")]
Expand All @@ -1871,6 +1881,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")]
Expand All @@ -1884,6 +1895,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")]
Expand All @@ -1897,6 +1909,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")]
Expand All @@ -1910,8 +1923,49 @@ atomic_int! {
"u64", "../../../std/primitive.u64.html",
"#![feature(integer_atomics)]\n\n",
atomic_umin, atomic_umax,
8,
u64 AtomicU64 ATOMIC_U64_INIT
}
#[cfg(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(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"),
Expand All @@ -1923,6 +1977,7 @@ atomic_int!{
"isize", "../../../std/primitive.isize.html",
"",
atomic_min, atomic_max,
ptr_width!(),
isize AtomicIsize ATOMIC_ISIZE_INIT
}
#[cfg(target_has_atomic = "ptr")]
Expand All @@ -1936,6 +1991,7 @@ atomic_int!{
"usize", "../../../std/primitive.usize.html",
"",
atomic_umin, atomic_umax,
ptr_width!(),
usize AtomicUsize ATOMIC_USIZE_INIT
}

Expand Down
15 changes: 6 additions & 9 deletions src/librustc_codegen_llvm/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -556,15 +554,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);
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/librustc_codegen_llvm/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,8 +478,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);
}
Expand All @@ -488,8 +488,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);
Expand Down
6 changes: 6 additions & 0 deletions src/libstd/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(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")]
Expand All @@ -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(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")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
46 changes: 46 additions & 0 deletions src/test/run-pass/atomic-alignment.rs
Original file line number Diff line number Diff line change
@@ -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>());
}
12 changes: 12 additions & 0 deletions src/test/ui/feature-gates/feature-gate-cfg-target-has-atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
Loading