Stabilize atomic_from_ptr

[tgross35]

This stabilizes atomic_from_ptr and moves the const gate to const_atomic_from_ptr. Const stability is blocked on const_mut_refs.

Tracking issue: #108652

Newly stable API:

// core::atomic

impl AtomicBool { pub unsafe fn from_ptr<'a>(ptr: *mut bool) -> &'a AtomicBool; }

impl<T> AtomicPtr<T> { pub unsafe fn from_ptr<'a>(ptr: *mut *mut T) -> &'a AtomicPtr<T>; }

impl AtomicU8    { pub unsafe fn from_ptr<'a>(ptr: *mut u8)    -> &'a AtomicU8;    }
impl AtomicU16   { pub unsafe fn from_ptr<'a>(ptr: *mut u16)   -> &'a AtomicU16;   }
impl AtomicU32   { pub unsafe fn from_ptr<'a>(ptr: *mut u32)   -> &'a AtomicU32;   }
impl AtomicU64   { pub unsafe fn from_ptr<'a>(ptr: *mut u64)   -> &'a AtomicU64;   }
impl AtomicUsize { pub unsafe fn from_ptr<'a>(ptr: *mut usize) -> &'a AtomicUsize; }

impl AtomicI8    { pub unsafe fn from_ptr<'a>(ptr: *mut i8)    -> &'a AtomicI8;    }
impl AtomicI16   { pub unsafe fn from_ptr<'a>(ptr: *mut i16)   -> &'a AtomicI16;   }
impl AtomicI32   { pub unsafe fn from_ptr<'a>(ptr: *mut i32)   -> &'a AtomicI32;   }
impl AtomicI64   { pub unsafe fn from_ptr<'a>(ptr: *mut i64)   -> &'a AtomicI64;   }
impl AtomicIsize { pub unsafe fn from_ptr<'a>(ptr: *mut isize) -> &'a AtomicIsize; }

[rustbot]

r? @Mark-Simulacrum

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

[tgross35]

This seems like a pretty uncontroversial method that mirrors AtomicX::as_ptr. Requres FCP

r? libs-api
@rustbot label +t-libs-api -t-libs

[rustbot]

Error: Label t-libs-api can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[dtolnay]

@rust-lang/libs-api:
@rfcbot fcp merge

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[WaffleLapkin]

Hm, looking at the docs again,

• The value behind ptr must not be accessed through non-atomic operations for the whole lifetime 'a.

This doesn't seem correct to me (or, rather, this is sufficient, but we could, and probably should, be looser). I think in practice, we only need a guarantee that the periods of using atomic and non-atomic accesses are synchronized. I.e. that for any two atomic and non-atomic accesses, there is a happens before relationship between them. (I could be wrong though, I'm not very knowledgeable here...)

[tgross35]

Do you think it would be better to say something about needing to use atomic accesses specifically whenever you cross Sync contexts?

I don’t think it is too bad as-is because users of the Atomic will probably assume that you can safely use it everywhere, send it across threads etc. If the caller assumes that there is a time when it doesn’t need to do atomic accesses (one thread, no race conditions) and then something down the line gets refactored to share the Atomic between threads, that could be an easy quiet race condition. Less chance for error if the caller always has to assume that it always has to use atomics.

Maybe changing from must to should would be enough?

[WaffleLapkin]

Well, I'm just saying that the current documentation doesn't seem to reflect the "actual" safety invariants. I agree that the stuff about atomic/non-atomic accesses being synchronized is tricky to explain and easy to mess up, but if you are using atomics, chances are, you already doing things that are hard to explain and easy to mess up 🙃

Either way, I think that leaving what is currently written as something like "the «actual» conditions are trivially satisfied if ..." may be a good idea. Like we could have both the easy to understand, but a bit too restrictive and the actually correct, but complex rules in the docs.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rustbot]

Some changes occurred in GUI tests.

cc @GuillaumeGomez

[tgross35]

@WaffleLapkin I updated the docs to say the following:

+    /// * The value behind `ptr` must not be accessed through non-atomic operations whenever
+    ///   the `AtomicBool` is used in a `Sync` context (e.g. read or modified in a different
+    ///   thread).

does that sound better?

Sorry @GuillaumeGomez I have absolutely no clue how your commit wound up in this PR, it should be gone now

[bors]

📌 Commit 227c844 has been approved by dtolnay

It is now in the queue for this repository.

[bors]

⌛ Testing commit 227c844 with merge 2a7c2df...

[bors]

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing 2a7c2df to master...

[rust-timer]

Finished benchmarking commit (2a7c2df): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 628.136s -> 627.414s (-0.11%)
Artifact size: 271.27 MiB -> 271.28 MiB (0.00%)

[RalfJung]

I think the docs for these functions should explicitly say that mixed-sized atomic accesses are forbidden.

I don't think this has been done? @tgross35 could you file a follow-up PR fixing the docs? This is important, we don't want people to assume that these are implicitly allowed.

[RalfJung]

Specifically I was looking for the overlapping atomics part here and I am not seeing it.

[RalfJung]

This should say "must not".

Looks like some methods got this note and some didn't; it should be present on all of these methods.

[tgross35]

@RalfJung I will follow up in #116762

[RalfJung]

@taiki-e points out an interaction between the caveats we are adding here to stay within the bounds of the C++ model, and this comment by @dtolnay:

As I understand it, the new guarantee introduced here would make the following sound: we want a value that can be atomically loaded but the backing data might either be the read-write memory where interior mutable values ordinarily go, or could be readonly memory on certain architectures that have this new guarantee.

Under the constraints imposed by this PR, I think that code would not be sound. And indeed there is no way to write such code soundly in C++, so we should be extremely careful with allowing it in Rust.