5

Atomics must be mutable by RalfJung · Pull Request #2464 · rust-lang/miri · GitH...

 2 years ago
source link: https://github.com/rust-lang/miri/pull/2464
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Conversation

Member

RalfJung commented 14 days ago

taiki-e reacted with heart emoji All reactions

RalfJung

added the S-blocked-on-rust Status: Blocked on landing a Rust PR label

14 days ago

RalfJung

changed the title Atomic must be mutable

Atomics must be mutable

14 days ago

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");

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?)

All reactions

Member

@taiki-e taiki-e 13 days ago

edited

AFAIK, the following are cases where atomic loads perform operations that could be problematic on read-only memory. 1

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

  1. 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.

  2. 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.

All reactions

Member

Author

@RalfJung RalfJung 13 days ago

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.

All reactions

Member

Author

@RalfJung RalfJung 13 days ago

Also let's discuss the Miri implementation here; the spec is being discussed at rust-lang/unsafe-code-guidelines#355.

taiki-e and thomcc reacted with thumbs up emoji All reactions

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK