Repository navigation
Use of volatile read is unsound #2
Description
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.
Activity
Amanieu commentedon Mar 17, 2019
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:
In practice, using volatile to read concurrently modified data is fine, and the Linux kernel even relies on this for their
READ_ONCEmacro (which just does a volatile read). It is extremely unlikely that LLVM would break existing code that relies on these semantics.joshlf commentedon Mar 17, 2019
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!