Add Atomic*::from_mut. #74532

m-ou-se · 2020-07-19T19:00:51Z

The atomic equivalent of Cell::from_mut.

rust-highfive · 2020-07-19T19:00:54Z

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

m-ou-se · 2020-07-19T19:03:47Z

This assumes the integral types have the same alignment requirement as their atomic equivalent on all platforms that support those. ~~I think that is the case, but not sure how to verify or (statically) check.~~ (Edit: See below.)

(This is definitely the case for AtomicBool, AtomicU8 and AtomicI8, as those are align(1).)

m-ou-se · 2020-07-19T19:40:27Z

The only platforms where the integer sizes don't match their alignment seem to be:

msp430-none-elf
avr-unknown-unknown

(Found by grep -P 'i(8|16|32|64|128):(?!\1)' src/librustc_target/spec/*.rs.)

The MSP430 supports atomics up to 16 bits (although not supported by llvm nor Rust), and aligns u16 to 16 bits.

The AVR target doesn't specify a max_atomic_width, which means it falls back to the pointer width (16), even though u16s align to 8 bits on that target. This seems incorrect, see avr-rust#172. (Also, --target=avr-unknown-unknown doesn't build on HEAD.)

Edit: 32-bit x86 is also a problem for 64-bit atomics. See below.

sfackler · 2020-07-19T20:32:30Z

On i686-unknown-linux-gnu, u64 is 4-byte aligned while AtomicU64 is 8-byte aligned: https://rust.godbolt.org/z/6n4dsr

m-ou-se · 2020-07-20T08:43:40Z

Thanks! Looks like my grep command didn't give what I was looking for ^^'

This works better:

fn main() {
    for triple in rustc_target::spec::get_targets() {
        let target_triple = rustc_target::spec::TargetTriple::from_triple(&triple);
        let target = rustc_target::spec::Target::search(&target_triple).unwrap();
        let layout = rustc_target::abi::TargetDataLayout::parse(&target).unwrap();
        if
            layout.i8_align.abi.bits() != 8 ||
            layout.i16_align.abi.bits() != 16 ||
            layout.i32_align.abi.bits() != 32 ||
            layout.i64_align.abi.bits() != 64
        {
            print!("{:<30}", triple);
            print!("{:>4}", layout.i8_align.abi.bits());
            print!("{:>4}", layout.i16_align.abi.bits());
            print!("{:>4}", layout.i32_align.abi.bits());
            print!("{:>4}", layout.i64_align.abi.bits());
            println!("  ({})", target.max_atomic_width());
        }
    }
}

i686-unknown-linux-gnu           8  16  32  32  (64)
i586-unknown-linux-gnu           8  16  32  32  (64)
i686-unknown-linux-musl          8  16  32  32  (64)
i586-unknown-linux-musl          8  16  32  32  (64)
i686-linux-android               8  16  32  32  (64)
i686-unknown-freebsd             8  16  32  32  (64)
i686-unknown-openbsd             8  16  32  32  (64)
i686-unknown-netbsd              8  16  32  32  (64)
i686-unknown-haiku               8  16  32  32  (64)
i686-apple-darwin                8  16  32  32  (64)
avr-unknown-unknown              8   8   8   8  (16)
msp430-none-elf                  8  16  16  16  (0)
i686-unknown-cloudabi            8  16  32  32  (64)
i686-wrs-vxworks                 8  16  32  32  (64)

Apart from the two I already mentioned, looks like I missed only intel x86, which is only a problem for 64-bit atomics.

I see a lot of discussion on llvm/gcc lists/trackers about whether i64 should've been 64-bit aligned or whether a 64-bit atomic should maybe be 32-bit aligned, but unfortunately this is how things are now. :)

Which path forward makes most sense?

Don't add from_mut to any atomic type. :(
Add from_mut only for 8-bit atomics (AtomicBool, AtomicU8, AtomicI8), which are always 1-byte aligned (that's hardcoded in src/libcore/sync/atomic.rs).
Add from_mut only for 8, 16, and 32 bit atomics. No AtomicU64::from_mut on any platform.
Add from_mut to all atomic types, but gate them behind some cfg just like like #[cfg(target_has_atomic = "64")].

3 would work and still be useful, but would become a problem if a platform gets added with 32-bit atomics and 16-bit aligned u32s. (Not sure how likely that is to happen.) 2 would be a safe fallback, but less useful. 4 might make sense, considering that atomic stuff is already gated behind specific cfg features.

m-ou-se · 2020-07-20T08:59:11Z

I guess a motivating example for this change makes sense at this point.

I want to work towards not only a from_mut, but also a from_mut_slice (and to_mut_slice, while we're at it), to allow a function that writes things to, say, a &mut [u32] to split the work up over threads without resorting to unsafe code:

fn calculate_things(buffer: &mut [u32]) {
    let buffer: &[AtomicU32] = AtomicU32::from_mut_slice(buffer);
    crossbeam_utils::thread::scope(|s| {
        for i in 0..n_threads {
            s.spawn(|_| {
                // do work and add stuff to `buffer`.
            });
        }
    }).unwrap();
}

Whether threads and atomics are used should be transparent to the caller. All it should know is that it needs to give mutable access to a buffer of u32s, and this function's implementation can decide itself if it wants to access those as atomics from multiple threads for its calculations.

C++20 also has something similar in the form of atomic_ref (as originally proposed in wg21's p0019r7).

JohnCSimon · 2020-08-11T06:12:41Z

@m-ou-se Ping from triage: can you please address the merge conflict?

m-ou-se · 2020-08-13T18:56:16Z

Add from_mut to all atomic types, but gate them behind some cfg just like like #[cfg(target_has_atomic = "64")].

Another option is to expect users to use #[cfg(accessible(AtomicU64::from_mut))] (from #64797), to avoid having to add another cfg name.

Dylan-DPC-zz · 2020-09-02T00:50:13Z

r? @KodrAus

KodrAus

This seems worthwhile to me, because it's probably not really obvious that integers might have different alignments to their atomic equivalents. I've created a tracking issue we can use: #76314

Should we also consider AtomicIsize and AtomicUsize where possible?

KodrAus · 2020-09-04T05:06:11Z

library/core/src/sync/atomic.rs

+    /// ```
+    #[inline]
+    #[unstable(feature = "atomic_from_mut", issue = "none")]
+    pub fn from_mut(v: &mut *mut T) -> &Self {


This one seems a little strange to me, since *mut T is already a pointer.

Just like AtomicU16::from_mut will take a &mut u16, AtomicPtr<T>::from_mut should take a &mut to the non-atomic type: &mut *mut T. The already existing AtomicPtr<T>::get_mut() also returns a &mut *mut T: https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicPtr.html#method.get_mut

@KodrAus The pointer itself is what atomic here, not what it points to, so when using is you actually use a pointer to the atomic pointer (otherwise you'll be copying the atomic pointer)

m-ou-se · 2020-09-05T10:17:17Z

Should we also consider AtomicIsize and AtomicUsize where possible?

This PR already implements AtomicIsize::from_mut and AtomicUsize::from_mut. They're implemented using the same atomic_int macro that's used for u8, u16, etc.

KodrAus · 2020-09-06T13:50:13Z

Thanks @m-ou-se!

@bors r+ rollup

bors · 2020-09-06T13:50:15Z

📌 Commit 7cc2569 has been approved by KodrAus

…m-mut, r=KodrAus Add Atomic*::from_mut. The atomic equivalent of [`Cell::from_mut`](https://doc.rust-lang.org/stable/std/cell/struct.Cell.html#method.from_mut).

bors · 2020-09-15T02:09:39Z

⌛ Testing commit 9914c3b with merge 715e934...

bors · 2020-09-15T03:58:19Z

☀️ Test successful - checks-actions, checks-azure
Approved by: KodrAus
Pushing 715e934 to master...

JohnTitor · 2020-09-20T01:01:44Z

Seems like this breaks libc CI, see https://dev.azure.com/rust-lang2/libc/_build/results?buildId=3161&view=logs&j=c7464085-50dc-55b0-d49a-ca2fc4e4d69e&t=7417e154-0538-5077-3a89-29444dec6e1c&l=430.
To reproduce the build error, run cargo +nightly build -Z build-std=core,alloc -vv --no-default-features --target armv7-apple-ios.
I'm not sure if we could follow-up something here though.

m-ou-se · 2020-09-20T01:10:18Z

@JohnTitor The error you linked is for 64 bit atomics. Is your target file correct? Does armv7 have 64 bit atomics?

Edit: I'll take a closer look tomorrow.

JohnTitor · 2020-09-20T01:17:04Z

@JohnTitor The error you linked is for 64 bit atomics. Is your target file correct? Does armv7 have 64 bit atomics?

Given this, it has 64-bit atomics, right?

https://github.com/rust-lang/rust/blob/85fbf49ce0e2274d0acf798f6e703747674feec3/compiler/rustc_target/src/spec/armv7_apple_ios.rs

RalfJung · 2020-09-20T07:31:26Z

library/core/src/sync/atomic.rs

+    #[inline]
+    #[unstable(feature = "atomic_from_mut", issue = "76314")]
+    pub fn from_mut(v: &mut *mut T) -> &Self {
+        let [] = [(); align_of::<AtomicPtr<()>>() - align_of::<*mut ()>()];


This will assert that their alignment is equal right? But shouldn't we rather test that Self alignment is no stricter than that of the pointer? You can use the static_assert macro to write any kind of boolean test.

Btw, if you know of any other place that uses the such an array length hack to encode a static assertion, please let me know -- we should add a FIXME at least.

get_mut already assumes that the alignment of the non-atomic type is ≤ the alignment of the atomic variant. from_mut technically only needs ≥, but things would be quite broken if it was > instead of just =.

You can use the static_assert macro to write any kind of boolean test.

Can I depend on that in core? I don't think core has any dependencies right now.

Oh right, you cannot directly use it.

But personally I feel like it'd be worth copying it to libcore and then using it there as an internal macro. That said, it seems you will have to rework this entire thing anyway so let's see where that goes.

RalfJung · 2020-09-20T07:34:59Z

The test failure is caused by the alignment check. And while I think the check can be relaxed (and written in a more readable way than an array length hack), I am not sure this is sufficient... there are platforms where u64 has 4-byte alignment, but AtomucU64 always requires 8-byte alignment on all platforms (LLVM has a special clause for alignment when it comes to atomic accesses). On those platforms, it is impossible to convert a &mut u64 to &mut AtomucU64 in general.

m-ou-se · 2020-09-20T09:26:45Z

On those platforms, it is impossible to convert a &mut u64 to &mut AtomicU64 in general.

Yes. AtomicU64::from_mut should simply not be available on those platforms. This is one of the reasons why it exists, to make sure users won't to do their own &mut u64 to &mut AtomicU64 transmutes which are easy to get wrong.

This PR disables some conversions that shouldn't exist, but does so using cfg(target_arch = ..). It'd be better if this information actually came from the build target spec, but that requires adding another cfg(atomic_...), because cfg(align_of(..) == n) is not a thing. I'll implement that ASAP, because apparently this is breaking some things right now.

RalfJung · 2020-09-20T10:11:51Z

Ah I see. That seems rather subtle, both in terms of accidentally writing non-portable code and (if you happen to work on a platform where this is not available) not realizing why the function is missing.

But that's a discussion for the tracking issue, 'll raise my points there.

RalfJung · 2020-09-20T10:15:32Z

This PR disables some conversions that shouldn't exist, but does so using cfg(target_arch = ..). It'd be better if this information actually came from the build target spec, but that requires adding another cfg(atomic_...), because cfg(align_of(..) == n) is not a thing. I'll implement that ASAP, because apparently this is breaking some things right now.

It seems appropriate to me to revert this PR, both to take time pressure off of you and to get broken users fixed ASAP. Developing an alternative approach looks like it could take some time. "Revert" doesn't mean "we'll never land this", it means "this PR had some unforeseen problems so let's take it back and then re-considering landing with what we learned".

m-ou-se · 2020-09-20T10:22:07Z

It seems appropriate to me to revert this PR, both to take time pressure off of you and to get broken users fixed ASAP.

I have the PR that fixes this properly ready! Just running the tests locally first :)

m-ou-se · 2020-09-20T10:24:09Z

Seems to work 🎉 #76965

m-ou-se · 2020-09-20T10:36:15Z

what we learned

Notes to self:

Don't assume that rustc_target::spec::get_targets() gives a complete overview of all platforms people will use rustc for.
Alignment, supported atomic sizes, etc. don't just depend on target_arch, but also on the vendor/os. (E.g. armv7-linux worked fine, but armv7-apple-ios broke.)
Adding a cfg for target spec info is pretty easy, and much preferred over hardcoding specific target triples/archs. 🙃

RalfJung · 2020-09-20T10:44:19Z

Okay, implementing it went quicker than I expected, but since this is now a language addition, there might be extra process here -- IIRC we've had RFCs for new cfg flags in the past. I am not sure if this one warrants a special exception.

Maybe open a thread in the t-lang stream on Zulip to get some lang team people to comment on that.

m-ou-se · 2020-09-20T10:48:22Z

Okay, implementing it went quicker than I expected, but since this is now a language addition, there might be extra process here -- IIRC we've had RFCs for new cfg flags in the past. I am not sure if this one warrants a special exception.

All the cfg() flags for atomics are unstable (gated behind feature(cfg_target_has_atomic)), including target_has_atomic and target_has_atomic_load_store. The latter was also added without much process iirc.

If this PR doesn't go through quickly, I'll send a PR to revert this one first.

RalfJung · 2020-09-20T10:49:56Z

The latter was also added without much process iirc.

Ah, if there's precedent for "ad-hoc" new atomic cfg flags then please link to that in the other PR.

KodrAus · 2020-09-20T11:51:19Z

Oh... I missed all of this going on here 😟 Sorry everyone!

@ghost

…mic-from-mut, r=kodrAus Revert adding Atomic::from_mut. This reverts rust-lang#74532, which made too many assumptions about platforms, breaking some things. Will need to be added later with a better way of gating on proper alignment, without hardcoding cfg(target_arch)s. --- To be merged if fixing from_mut (rust-lang#76965) takes too long. r? @ghost

@ghost

…mic-from-mut, r=kodrAus Revert adding Atomic::from_mut. This reverts rust-lang#74532, which made too many assumptions about platforms, breaking some things. Will need to be added later with a better way of gating on proper alignment, without hardcoding cfg(target_arch)s. --- To be merged if fixing from_mut (rust-lang#76965) takes too long. r? @ghost

joshtriplett · 2020-09-21T14:38:35Z

As mentioned on the PR for it, adding this new feature-gated cfg looks like a good solution to me from a language perspective.

@RalfJung

…-from-mut, r=Amanieu Add cfg(target_has_atomic_equal_alignment) and use it for Atomic::from_mut. Fixes some platform-specific problems with rust-lang#74532 by using the actual alignment of the types instead of hardcoding a few `target_arch`s. r? @RalfJung

@RalfJung

…-from-mut, r=Amanieu Add cfg(target_has_atomic_equal_alignment) and use it for Atomic::from_mut. Fixes some platform-specific problems with rust-lang#74532 by using the actual alignment of the types instead of hardcoding a few `target_arch`s. r? @RalfJung

rust-highfive assigned cramertj Jul 19, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2020

m-ou-se force-pushed the atomic-from-mut branch from 01b2b7b to e52cd6d Compare July 19, 2020 19:06

This comment has been minimized.

Sign in to view

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 11, 2020

m-ou-se force-pushed the atomic-from-mut branch from e52cd6d to 42fa4cf Compare August 13, 2020 18:54

m-ou-se force-pushed the atomic-from-mut branch from 42fa4cf to 09fff5f Compare August 13, 2020 19:23

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 1, 2020

rust-highfive assigned KodrAus and unassigned cramertj Sep 2, 2020

KodrAus mentioned this pull request Sep 4, 2020

Tracking Issue for atomic_from_mut #76314

Open

6 tasks

KodrAus reviewed Sep 4, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 6, 2020

Dylan-DPC-zz mentioned this pull request Sep 6, 2020

Rollup of 18 pull requests #76414

Closed

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 15, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 15, 2020

bors merged commit 715e934 into rust-lang:master Sep 15, 2020

rustbot added this to the 1.48.0 milestone Sep 15, 2020

JohnTitor mentioned this pull request Sep 20, 2020

Add UIO_MAXIOV constant to uclibc rust-lang/libc#1895

Merged

RalfJung reviewed Sep 20, 2020

View reviewed changes

m-ou-se mentioned this pull request Sep 20, 2020

Add cfg(target_has_atomic_equal_alignment) and use it for Atomic::from_mut. #76965

Merged

m-ou-se mentioned this pull request Sep 20, 2020

Revert adding Atomic::from_mut. #76967

Merged

m-ou-se deleted the atomic-from-mut branch December 30, 2020 21:26

Add Atomic*::from_mut. #74532

Add Atomic*::from_mut. #74532

Conversation

m-ou-se commented Jul 19, 2020

rust-highfive commented Jul 19, 2020

m-ou-se commented Jul 19, 2020 • edited Loading

m-ou-se commented Jul 19, 2020 • edited Loading

sfackler commented Jul 19, 2020

m-ou-se commented Jul 20, 2020

m-ou-se commented Jul 20, 2020

This comment has been minimized.

JohnCSimon commented Aug 11, 2020

m-ou-se commented Aug 13, 2020

Dylan-DPC-zz commented Sep 2, 2020

KodrAus left a comment

Choose a reason for hiding this comment

KodrAus Sep 4, 2020

Choose a reason for hiding this comment

m-ou-se Sep 5, 2020

Choose a reason for hiding this comment

elichai Sep 13, 2020

Choose a reason for hiding this comment

m-ou-se commented Sep 5, 2020

KodrAus commented Sep 6, 2020

bors commented Sep 6, 2020

bors commented Sep 15, 2020

bors commented Sep 15, 2020

JohnTitor commented Sep 20, 2020

m-ou-se commented Sep 20, 2020 • edited Loading

JohnTitor commented Sep 20, 2020

RalfJung Sep 20, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Sep 20, 2020

Choose a reason for hiding this comment

m-ou-se Sep 20, 2020

Choose a reason for hiding this comment

m-ou-se Sep 20, 2020

Choose a reason for hiding this comment

RalfJung Sep 20, 2020

Choose a reason for hiding this comment

RalfJung commented Sep 20, 2020 • edited Loading

m-ou-se commented Sep 20, 2020

RalfJung commented Sep 20, 2020

RalfJung commented Sep 20, 2020

m-ou-se commented Sep 20, 2020

m-ou-se commented Sep 20, 2020

m-ou-se commented Sep 20, 2020

RalfJung commented Sep 20, 2020

m-ou-se commented Sep 20, 2020

RalfJung commented Sep 20, 2020 • edited Loading

KodrAus commented Sep 20, 2020

joshtriplett commented Sep 21, 2020

m-ou-se commented Jul 19, 2020 •

edited

Loading

m-ou-se commented Jul 19, 2020 •

edited

Loading

m-ou-se commented Sep 20, 2020 •

edited

Loading

RalfJung Sep 20, 2020 •

edited

Loading

RalfJung commented Sep 20, 2020 •

edited

Loading

RalfJung commented Sep 20, 2020 •

edited

Loading