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.

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

Contradiction in documentation about alignment of AtomicBool #126084

Closed
FeldrinH opened this issue Jun 6, 2024 · 2 comments · Fixed by #126213
Closed

Contradiction in documentation about alignment of AtomicBool #126084

FeldrinH opened this issue Jun 6, 2024 · 2 comments · Fixed by #126213
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@FeldrinH
Copy link

FeldrinH commented Jun 6, 2024

Location

https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicBool.html

Summary

The introductory section for AtomicBool states:

This type has the same size, alignment, and bit validity as a bool.

But later the safety requirements for from_ptr state:

ptr must be aligned to align_of::<AtomicBool>() (note that on some platforms this can be bigger than align_of::<bool>()).

This seems to be a contradiction. If AtomicBool has the same alignment as bool then how can align_of::<AtomicBool>() be bigger than align_of::<bool>().

@FeldrinH FeldrinH added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Jun 6, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 6, 2024
@jieyouxu jieyouxu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-bug Category: This is a bug. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 6, 2024
@lolbinarycat
Copy link

the alignment requirement seems to have been copy-pasted across every *::from_ptr method. it could be argued that this alignment requirement could be desirable to allow emulating AtomicBool on platforms that only have word-sized atomic operations in the future, but we probably shouldn't make this into UB when it was arguably not UB before.

@zachs18
Copy link
Contributor

zachs18 commented Jun 10, 2024

Rust guarantees that size % align == 0, so since the size is 1 the only possible alignment for AtomicBool that wouldn't also require changing the size is 1. I agree that from_ptr's docs should be clarified, either by removing that line, or perhaps changing it to something like

ptr must be aligned to align_of::<AtomicBool>() (note that this is always true, because align_of::<AtomicBool>() == 1).

so that it is clear that alignment is being considered, it's just never an issue.

The docs for Atomic{U,I}8 have a similar issue, since they also guarantee their size is the same as {u,i}8 (and therefore have size 1), but have the same copy-pasted note about alignment in their from_ptr's docs. (Though for them, this would be more annoying to fix, since their from_ptr is generated by a macro shared with the other atomic integers, which can be more aligned than their corresponding primitives.)


Tangent about "[...] allow emulating AtomicBool on platforms that only have word-sized atomic operations [...]"

Note that at the codegen level (but not at the language level), this is sort of already being done. E.g. if the platform only has 32-bit atomics, we mask the address to the aligned 4-byte word containing the AtomicBool, and then perform a fetch_update loop masking the value to the approriate byte in the word. This is how Atomic{Bool,{U,I}{8,16}} are implemented "under the hood" by LLVM on such platforms, like RISC-V (example) which have 32-bit (but not 8/16-bit) atomic read-modify-write instructions.

@bors bors closed this as completed in 94b9ea4 Jun 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 24, 2024
Rollup merge of rust-lang#126213 - zachs18:atomicbool-u8-i8-from-ptr-alignment, r=Nilstrieb

Update docs for AtomicBool/U8/I8 with regard to alignment

Fixes rust-lang#126084.

Since `AtomicBool`/`AtomicU8`/`AtomicI8` are guaranteed to have size == 1, and Rust guarantees that `size % align == 0`, they also must have alignment equal to 1, so some current docs are contradictory/confusing when describing their alignment requirements.

Specifically:

* Fix `AtomicBool::from_ptr` claiming that `align_of::<AtomicBool>() > align_of::<bool>()` on some platforms. (same for `AtomicU8::from_ptr`/`AtomicI8::from_ptr`)
* Explicitly state that `AtomicU8`/`AtomicI8` have the same alignment as `u8`/`i8` (in addition to size and bit validity)
* (internal) Change the `if_not_8_bit` macro to be `if_8_bit` and to allow an "if-else"-like structure, instead of just "if"-like.

---

I opted to leave the "`ptr` must be aligned" wording in `from_ptr`'s docs and just clarify that it is always satsified, instead of just removing the wording entirely. If that is instead preferred I can do that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants