-
Notifications
You must be signed in to change notification settings - Fork 348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Atomics must be mutable #2464
Atomics must be mutable #2464
Conversation
src/concurrency/data_race.rs
Outdated
let (alloc_id, _offset, _prov) = | ||
this.ptr_try_get_alloc_id(place.ptr).expect("there are no zero-sized atomic accesses"); | ||
if this.get_alloc_mutability(alloc_id)? == Mutability::Not { | ||
throw_ub_format!("atomic operations cannot be performed on read-only memory"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth noting that this is target/size-specific for atomic loads? Or maybe there's a way to phrase it less strictly?
The guidance I've always been given for dealing with cross-process shared memory is that this is that atomics must be used if another process may write to it and that raw pointers (since obviously references are out of the question) would be UB (even if volatile were used), as it would still be a data race.
I do sympathize with not writing to add the target-specific logic for when this is broken to miri (it seems fragile and gross), but my concern is that encouraging people to not use atomics for this would lead them to perform worse UB.
(Unless we want to consider it not a data race?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, the following are cases where atomic loads perform operations that could be problematic on read-only memory. 1
- aarch64: 128-bit loads (LL/SC by ld(a)xp/st(l)xp) (if FEAT_LSE2 which makes load pair/store pair single-copy atomic is not available)
- x86_64: 128-bit loads (CAS by cmpxchg16b)
- x86: 64-bit loads (CAS by cmpxchg8b) (if both SSE and X87 are not available)
- arm (pre-v6,
Linux only): any non-relaxed/non-unordered loads (CAS by__kuser_cmpxchg
via__sync_*
builtins)
EDIT: pre-v6 arm issue is not Linux only.
EDIT: see rust-lang/unsafe-code-guidelines#355 (comment) for the emulator's case.
Considering the above, I think it is ok to allow atomic load on read-only memory when all of the following are met. (assuming we don't want to do complex and fragile target-specific processing)
- size is pointer-width or smaller. 2
- ordering is relaxed (or unordered).
And the error message would probably be something like:
atomic load operations with non-relaxed ordering or grater than pointer-width cannot be performed on read-only memory
Also, even if Miri does not allow them, I agree that additional notes would be helpful.
Footnotes
-
armv6+'s 64-bit load is
ldrexd
, powerpc64 and s390x have 128-bit atomic load instructions, and RISC-V and MIPS don't have double-width atomics, so those platforms are fine. I don't know about 32-bit powerpc/sparc's 64-bit atomic load -- IIRC LLVM does not support the former, and the latter is a tier 3 target. ↩ -
However, on 64-bit architecture's ILP32 ABIs, such as x86_64's X32 ABI, 64-bit load is definitely okay, so if Miri or rustc is aware of the architecture's pointer width, we may want to allow those as well. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the above, I think it is ok to allow atomic load on read-only memory when all of the following are met. (assuming we don't want to do complex and fragile target-specific processing)
That's hoping we don't find further exceptions to these rules later? :D
We can possibly relax this rule later if there is consensus that all targets supported by Rust will support relaxed atomic loads of pointer size and smaller on read-only memory, but how sure are we no targets will violate this in the future?
Note that Miri anyway does not support sharing memory with other processes, so read-only memory here refers to static
without interior mutability. Due to rust-lang/unsafe-code-guidelines#303, this PR does not change behavior on any program that Miri currently accepts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also let's discuss the Miri implementation here; the spec is being discussed at rust-lang/unsafe-code-guidelines#355.
c9ff538
to
34f3889
Compare
34f3889
to
a1f5a75
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Fixes #2463
Needs rust-lang/rust#100181