Atomic as_mut_ptr

[pitdicker]

I encountered the following pattern a few times: In Rust we use some atomic type like AtomicI32, and an FFI interface exposes this as *mut i32 (or some similar libc type).

It was not obvious to me if a just transmuting a pointer to the atomic was acceptable, or if this should use a cast that goes through an UnsafeCell. See #66136 (comment)

Transmuting the pointer directly:

let atomic = AtomicI32::new(1);
let ptr = &atomic as *const AtomicI32 as *mut i32;
unsafe {
    ffi(ptr);
}

A dance with UnsafeCell:

let atomic = AtomicI32::new(1);
unsafe {
    let ptr = (&*(&atomic as *const AtomicI32 as *const UnsafeCell<i32>)).get();
    ffi(ptr);
}

Maybe in the end both ways could be valid. But why not expose a direct method to get a pointer from the standard library?

An as_mut_ptr method on atomics can be safe, because only the use of the resulting pointer is where things can get unsafe. I documented its use for FFI, and "Doing non-atomic reads and writes on the resulting integer can be a data race."

The standard library could make use this method in a few places in the WASM module.

cc @RalfJung as you answered my original question.

[rust-highfive]

r? @KodrAus

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

[RalfJung]

👍 for the basic functionality; I don't have time to review the docs I am afraid.

[pitdicker]

I don't have time to review the docs I am afraid.

Thank you and no problem. I find the docs hard to write with all the macro stuff, I had to give it multiple tries to get something ok-ish. Screenshot of AtomicU64::as_mut_ptr() if it helps:

[jfrimmel]

Since you can produce data races using this method it may be good to mark it unsafe. This would most likely not even introduce more unsafe-blocks, since FFI functions (as on of the primary use cases) always require such an unsafe environment.

[pitdicker]

Since you can produce data races using this method it may be good to mark it unsafe. This would most likely not even introduce more unsafe-blocks, since FFI functions (as on of the primary use cases) always require such an unsafe environment.

That was my first intention, and in a way I agree with you. But there is no way to cause data races with this function in safe Rust, and there is precedent in not marking a function unsafe just because it can lead to unsafety later on. Even all the similarly named as_mut_ptr methods on types such as slices and str can lead to unsafety, but are not unsafe by itself.

[jfrimmel]

That was my first intention, and in a way I agree with you. But there is no way to cause data races with this function in safe Rust, and there is precedent in not marking a function unsafe just because it can lead to unsafety later on. Even all the similarly named as_mut_ptr methods types such as slices and str can lead to unsafety, but are not unsafe by itself.

The important difference is, that they all take a &mut self, whereas those new functions only take the &self receiver. I'm not totally sure, whether this fact can be "exploited" though.

[pitdicker]

The important difference is, that they all take a &mut self, whereas those new functions only take the &self receiver. I'm not totally sure, whether this fact can be "exploited" though.

It was maybe good to mention that atomics are a #[repr(transparent)] wrapper around UnsafeCell<some_integer>, and are guaranteed to have the same memory representation as the integer type the wrap. Because of the UnsafeCell returning *mut from &self this is ok.

[pitdicker]

Another point against making this method unsafe: it is doing nothing that can't already be done in safe (but maybe questionable) code.

When should I make a tracking issue? After review?

[hellow554]

Quoting get_mut:

pub fn get_mut(&mut self) -> &mut i64

Returns a mutable reference to the underlying integer.

This is safe because the mutable reference guarantees that no other threads are concurrently accessing the atomic data.

Please note the difference, that get_mut takes &mut self instead. Shouldn't as_mut_ptr do the same?

[RalfJung]

No. The signature here matches UnsafeCell::get.

[pitdicker]

Apparently the documentation should explain why this method is safe. I am no wordsmith, @jfrimmel and @hellow554 can I use you as test subjects 😄? Does this explanation make sense and is it enough?

Returning an *mut pointer from a shared reference to this atomic is safe because the atomic types work with interior mutability. All modifications of an atomic change the value through a shared reference, and can do so safely as long as they use atomic operations. Any use of the returned raw pointer requires an unsafe block and still has to uphold the same restriction: operations done on it must be atomic.

[pitdicker]

Please note the difference, that get_mut takes &mut self instead. Shouldn't as_mut_ptr do the same?

The &mut returned by get_mut has no restrictions on how you use the resulting integer because the signature guarantees there are no other threads in play.

The *mut returned by as_mut_ref can exist while the atomic is still in use by multiple threads, and can only used by unsafe code or an external library and must uphold the restriction to use atomic operations.

[KodrAus]

This looks good to me! I’ll open up a tracking issue that we can reference in here then r=me

[pitdicker]

Thank you!
r+=KodrAus

[pitdicker]

Maybe this one: @bors r=KodrAus

[bors]

@pitdicker: 🔑 Insufficient privileges: Not in reviewers

[RalfJung]

@bors r=KodrAus

[bors]

📌 Commit d34090a has been approved by KodrAus

[bors]

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened