Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clarify atomic bit validity #121943

Merged
merged 6 commits into from
Mar 28, 2024
Merged

Clarify atomic bit validity #121943

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
59a3a5d
Clarify atomic bit validity
joshlf Mar 3, 2024
db34b08
Clarify bit validity for AtomicBool
joshlf Mar 3, 2024
00d21c9
Document AtomicPtr bit validity
joshlf Mar 3, 2024
fba87f6
Use "size and alignment" rather than layout
joshlf Mar 3, 2024
c50804c
Update library/core/src/sync/atomic.rs
joshlf Mar 3, 2024
a6e3e02
Update library/core/src/sync/atomic.rs
joshlf Mar 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions library/core/src/sync/atomic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@taiki-e taiki-e Mar 3, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Member

@taiki-e taiki-e Mar 3, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// **Note**: This type is only available on platforms that support atomic
/// loads and stores of `u8`.
Expand Down Expand Up @@ -272,7 +272,7 @@ unsafe impl Sync for AtomicBool {}

/// A raw pointer type which can be safely shared between threads.
///
/// This type has the same in-memory representation as a `*mut T`.
/// This type has the same size and bit validity as a `*mut T`.
///
/// **Note**: This type is only available on platforms that support atomic
/// loads and stores of pointers. Its size depends on the target pointer's size.
Expand Down Expand Up @@ -2121,7 +2121,7 @@ macro_rules! atomic_int {
$int_type:ident $atomic_type:ident) => {
/// An integer type which can be safely shared between threads.
///
/// This type has the same in-memory representation as the underlying
/// This type has the same size and bit validity as the underlying
/// integer type, [`
#[doc = $s_int_type]
/// `].
Expand Down