CopyCell::set has undefined behavior

[SimonSapin]

toolshed/src/cell.rs

Lines 10 to 12 in e42cd8c

	pub struct CopyCell<T> {
	/// Internal value
	value: T,

toolshed/src/cell.rs

Lines 61 to 68 in e42cd8c

	pub fn set(&self, value: T) {
	use std::ptr::write_volatile;
	// Regular write produces abnormal behavior when running tests in
	// `--release` mode. Reordering writes when the compiler assumes
	// things are immutable is dangerous.
	unsafe { write_volatile(self.mut_ptr(), value) };
	}

This is writing through &T without std::cell::UnsafeCell being involved at all. Unfortunately UnsafeCell is currently not Copy, so as far as I understand CopyCell cannot be made sound until something like rust-lang/rust#55207 is implemented.

[maciejhirsz]

Yes, I'd need Copy on either Cell or UnsafeCell. I'm unsure I follow where the UB is though (can be just my ignorance), write_volatile should prevent any optimizations from moving the operation around.

The only use case I have for this is swapping references around in a tree. Would having a *mut T instead of T wrapper be sounder here?

[maciejhirsz]

As to previous comment, using *mut T wouldn't work since I actually want to be able to swap the pointers, not mutate the values behind them (except for such places that contain CopyCell themselves). I'm constructing a tree in which I want aliasing, but I also want to be able to swap references. Alternative would be to forgo aliasing altogether and use &mut references, which the Arena can provide already in a sound manner.

Aliasing would have been possible with a regular Cell, since references are Copy, but only if the data structure itself is not nested. The only reason I could have ever found for Cell not implementing Copy is ergonomics, not soundness. Likewise, the discussion around UnsafeCell linked above around implementing Copy would be sound, except for breaking soundness on RefCell<T> producing a &UnsafeCell<T> that assumes !Copy.

The soundness of CopyCell therefore hinges on:

1. The fact that the PhantomData marker disables the Sync marker, thus making it unusable in multi-threaded scenarios, where mutation and aliasing are at odds for obvious reasons.
2. That write_volatile, while not being a guarantee of soundness (mostly due to being rather nebulous in its lack of a definition of what volatile itself actually means), is the absolute best I can do to make sure the code does not exhibit UB in single-threaded scenario.
3. A mutable pointer is never exposed except for a scenario where it is obtained from a &mut CopyCell.

In practice, I've been using this crate in a couple of projects, in no cases, running with --release and with lto on for tests and benchmarks, has the behavior of CopyCell been anything but expected. This, again, is not a guarantee of it being UB free, or that future versions of Rust won't introduce UB to it, but it is the most pragmatic thing I can do for the time being.

I'll keep this issue open until either Cell, UnsafeCell, or another construct by which I can obtain soundness guarantees that also implements Copy is available in the language.