Clarify atomic bit validity #121943
Clarify atomic bit validity #121943
Conversation
The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. The new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type.
rustbot
added
S-waiting-on-review
Co-authored-by: Taiki Endo <te316e89@gmail.com>
Co-authored-by: Taiki Endo <te316e89@gmail.com>
| @@ -243,7 +243,7 @@ const EMULATE_ATOMIC_BOOL: bool = | |||
|
|
|||
| /// A boolean type which can be safely shared between threads. | |||
| /// | |||
| /// This type has the same in-memory representation as a [`bool`]. | |||
| /// This type has the same size, alignment, and bit validity as a [`bool`]. | |||
There was a problem hiding this comment.
Hmm, it's a bit odd to see "alignment" here when it's not in the others.
I guess it's a consequence of the other things, though: bool is size 1 per https://doc.rust-lang.org/std/mem/fn.size_of.html, which means the alignment can't be more than 1, and alignment is strictly positive so it's squeezed to have only one possible value.
One thing I'm unsure about: this clearly has a safety invariant that it's zero or one, but is that strictly a validity invariant? How do those interact with atomic ops? Or is the API restricted enough that it never hits a scenario where the question would come up?
There was a problem hiding this comment.
Hmm, it's a bit odd to see "alignment" here when it's not in the others.
Ideally AtomicI8&AtomicU8 docs should mention that as well, but I don't know how easy it would be to do that due to macro.
One thing I'm unsure about: this clearly has a safety invariant that it's zero or one, but is that strictly a validity invariant? How do those interact with atomic ops? Or is the API restricted enough that it never hits a scenario where the question would come up?
In my understanding, the stabilization of from_ptr (done in 1.75) establishes a requirement that AtomicBool must have strictly the same validity invariant as bool.
https://doc.rust-lang.org/stable/std/sync/atomic/struct.AtomicBool.html#method.from_ptr
There was a problem hiding this comment.
Hmm, it's a bit odd to see "alignment" here when it's not in the others.
I guess it's a consequence of the other things, though:
boolis size 1 per https://doc.rust-lang.org/std/mem/fn.size_of.html, which means the alignment can't be more than 1, and alignment is strictly positive so it's squeezed to have only one possible value.
I could also just explicitly document that the alignment is 1 if you'd prefer.
One thing I'm unsure about: this clearly has a safety invariant that it's zero or one, but is that strictly a validity invariant? How do those interact with atomic ops? Or is the API restricted enough that it never hits a scenario where the question would come up?
Does @taiki-e's argument convince you? If not, I'd be happy to omit this for AtomicBool in particular (which is the only type here with a non-trivial bit validity requirement). Alternatively, I could say something like "it's guaranteed that all bit-valid bools can be soundly transmuted to AtomicBool, but the inverse is not currently guaranteed."
There was a problem hiding this comment.
Alternatively, I could say something like "it's guaranteed that all bit-valid
bools can be soundly transmuted toAtomicBool, but the inverse is not currently guaranteed."
I only mentioned from_ptr (bool -> AtomicBool) because as_ptr (AtomicBool -> bool) was already stable at the time, but this should be guaranteed in both directions.
https://doc.rust-lang.org/stable/std/sync/atomic/struct.AtomicBool.html#method.as_ptr
There was a problem hiding this comment.
Yeah, I agree. I just meant that the proposed wording could work as an alternative if @scottmcm isn't convinced by your argument that we can rely on bit validity being equal between the two types. In particular, it sounds like their concern is about whether atomic operations could introduce the possibility of an AtomicBool temporarily being in some in-between state. Unless I'm misunderstanding, that concern does not imply that 0x00 or 0x01 would be invalid AtomicBool instances, but rather that there might be more valid AtomicBool instances than just those two.
There was a problem hiding this comment.
whether atomic operations could introduce the possibility of an AtomicBool temporarily being in some in-between state.
Hmm, if an atomic operation has an observable in-between state, it is not "atomic", right?
There was a problem hiding this comment.
Yeah, I agree; I'm just trying to steel-man @scottmcm's argument. I am convinced by your argument that bit validity is completely equivalent, but I want to make sure to leave the door open for other language changes on this PR if they aren't convinced.
There was a problem hiding this comment.
I think you've convinced me here. If someone's doing a typed move of AtomicBool, it better be 0/1.
Things are complicated behind a reference, but this isn't actually guaranteeing anything there, so I think it's fine as-is.
|
I definitely like having more specific words here. @bors r+ rollup |
bors
added
S-waiting-on-bors
Clarify atomic bit validity The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. For integer types and for `AtomicPtr<T>`, the new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type/`*mut T`. For `AtomicBool`, the new wording clarifies that size, alignment, and bit validity are guaranteed to match `bool`. Note that we use the phrase "size and alignment" rather than "layout" since the latter term also implies that the field types are the same. This isn't true - `AtomicXxx` doesn't store an `xxx`, but rather an `UnsafeCell<xxx>`. This distinction is important for some `unsafe` code, which needs to reason about the presence or absence of interior mutability in order to ensure that their code is sound (see e.g. google/zerocopy#251).
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#121943 (Clarify atomic bit validity) - rust-lang#123089 (Add invariant to VecDeque::pop_* that len < cap if pop successful) - rust-lang#123101 (Delegation: fix ICE on wrong `Self` instantiation) - rust-lang#123130 (Load missing type of impl associated constant from trait definition) - rust-lang#123133 (chore: fix some comments) - rust-lang#123136 (Some wording improvement) - rust-lang#123139 (`num::NonZero::get` can be 1 transmute instead of 2) - rust-lang#123142 (Let nils know about changes to target docs) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#121943 (Clarify atomic bit validity) - rust-lang#123075 (CFI: Fix drop and drop_in_place) - rust-lang#123101 (Delegation: fix ICE on wrong `Self` instantiation) - rust-lang#123130 (Load missing type of impl associated constant from trait definition) - rust-lang#123133 (chore: fix some comments) - rust-lang#123136 (Some wording improvement) - rust-lang#123139 (`num::NonZero::get` can be 1 transmute instead of 2) - rust-lang#123142 (Let nils know about changes to target docs) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#121943 - joshlf:patch-11, r=scottmcm Clarify atomic bit validity The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. For integer types and for `AtomicPtr<T>`, the new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type/`*mut T`. For `AtomicBool`, the new wording clarifies that size, alignment, and bit validity are guaranteed to match `bool`. Note that we use the phrase "size and alignment" rather than "layout" since the latter term also implies that the field types are the same. This isn't true - `AtomicXxx` doesn't store an `xxx`, but rather an `UnsafeCell<xxx>`. This distinction is important for some `unsafe` code, which needs to reason about the presence or absence of interior mutability in order to ensure that their code is sound (see e.g. google/zerocopy#251).
The previous definition used the phrase "representation", which is ambiguous given the current state of memory model nomenclature in Rust. For integer types and for
AtomicPtr<T>, the new wording clarifies that size and bit validity are guaranteed to match the corresponding native integer type/*mut T. ForAtomicBool, the new wording clarifies that size, alignment, and bit validity are guaranteed to matchbool.Note that we use the phrase "size and alignment" rather than "layout" since the latter term also implies that the field types are the same. This isn't true -
AtomicXxxdoesn't store anxxx, but rather anUnsafeCell<xxx>. This distinction is important for someunsafecode, which needs to reason about the presence or absence of interior mutability in order to ensure that their code is sound (see e.g. google/zerocopy#251).