Use of volatile read is unsound

[joshlf]

The volatile read at https://github.com/Amanieu/seqlock/blob/master/src/lib.rs#L147 is unsound - according to the LLVM memory model, volatile reads on data which is being concurrently modified are UB:

Note that NotAtomic volatile loads and stores are not properly atomic; do not try to use them as a substitute.

- https://llvm.org/docs/Atomics.html

This should be a relaxed atomic load instead.

[Amanieu]

That's not what your quote says. It just says that the operations aren't properly atomic, which means that they may be split into multiple operations, which would not be atomic (i.e. you could observe a mix of the new and old value). Also volatile operations don't have any of the memory ordering properties that atomic operations have.

We don't actually need either of these properties in seqlock:

• We discard the data if it is concurrently modified, so we don't care about split reads/writes.
• We enforce memory ordering with other atomic operations and fences.

In practice, using volatile to read concurrently modified data is fine, and the Linux kernel even relies on this for their READ_ONCE macro (which just does a volatile read). It is extremely unlikely that LLVM would break existing code that relies on these semantics.

[joshlf]

So I get where you're coming from on that interpretation. I'm honestly a bit surprised because when I was looking at the spec to find the relevant quote, I could've sworn seeing a stronger wording in the past (explicitly calling out using volatile on concurrently-modified memory as UB). It's possible I'm just making that up. In any case, it strikes me that a) the spec definitely isn't precisely worded enough to guarantee that this isn't technically UB, but also that b) your point about the fact that people rely on this behavior is a fair one.

In any case, I came here from rust-lang/rust#59155 (comment), so hopefully that will obsolete this anyway.

Cheers!