-
Notifications
You must be signed in to change notification settings - Fork 102
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
Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 #1091
Conversation
@joshlf I can split out the |
a2c9b30
to
a2db8f9
Compare
61cb559
to
2d66234
Compare
@joshlf I was eventually able to get the UI tests to pass, but it was a little annoying figuring out which command to run to regenerate them. For reference, it's:
(not It might be worth noting the correct procedure in |
423ad6a
to
715b592
Compare
@joshlf merge conflict has been resolved, let me know if you want me to change anything here or split the PR. |
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.
Apologies for taking so long to review this. I've left a few basic comments from a late-night skim, but I should have more comments once I get a chance to take a look at this with more brain cells.
Really impressed by how well you picked up on all of our idiosyncratic design patterns! This PR feels like it was written by one of the maintainers!
Oh one other thing: Can you squash all of your commits into one? That way you'll get to control the commit message that ends up in |
Taking another look at this: I think it'd be easier to review (and also to reason about after the fact if need be) if we split out the change that moves code into a new module, since that makes it so there's no clear diff for the code that moved. Would you be able to move the code back to its old location and then we can follow up with the code movement in a follow-on PR? |
Should now be a single commit
Code moved back to @joshlf this should be ready for another look. Note that I changed how the atomic types implement |
cd818a1
to
ffc2d84
Compare
Failures (https://github.com/google/zerocopy/actions/runs/9001494361/job/24728104608?pr=1091) seem unrelated. Given the number of checks being run (237!) it seems like some of the tests which need to download entire toolchains spuriously time-out |
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.
Awesome, thank you for reworking this! In general this looks awesome. I'm going to try to push hard on how we express and propagate our safety invariants, but the rest of the PR looks great. This is definitely nicer than what we have currently.
@@ -35,6 +35,10 @@ exclude = [".*"] | |||
# From 1.61.0, Rust supports generic types with trait bounds in `const fn`. | |||
zerocopy-generic-bounds-in-const-fn = "1.61.0" | |||
|
|||
# From 1.60.0, Rust supports detecting if a target supports atomics, so we can |
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.
# From 1.60.0, Rust supports detecting if a target supports atomics, so we can | |
# From 1.60.0, Rust supports detecting whether a target supports atomics, so we can |
src/lib.rs
Outdated
/// Note: On targets like `thumbv6m-none-eabi` (which has an [`AtomicU32`] | ||
/// type but `#[cfg(target_has_atomic = "32")]` is false), we don't implement | ||
/// the zerocopy traits. We will be able to handle this when Rust stabilizes | ||
/// [`#[cfg(target_has_atomic_load_store)]`][target_has_atomic_load_store]. |
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.
/// Note: On targets like `thumbv6m-none-eabi` (which has an [`AtomicU32`] | |
/// type but `#[cfg(target_has_atomic = "32")]` is false), we don't implement | |
/// the zerocopy traits. We will be able to handle this when Rust stabilizes | |
/// [`#[cfg(target_has_atomic_load_store)]`][target_has_atomic_load_store]. | |
/// TODO(#1009): On targets like `thumbv6m-none-eabi` (which has an [`AtomicU32`] | |
/// type but `#[cfg(target_has_atomic = "32")]` is false), we don't implement | |
/// the zerocopy traits. We will be able to handle this when Rust stabilizes | |
/// [`#[cfg(target_has_atomic_load_store)]`][target_has_atomic_load_store]. |
I've updated #1009 to track this. Feel free to suggest edits to that issue if there's more detail I should include.
src/lib.rs
Outdated
/// | ||
/// # Safety | ||
/// | ||
/// `$atomic` must be an atomic type with corresponding native type `$native`. |
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.
Can this be more precise? "Atomic type" here is obviously alluding to the fact that all of the atomics in core::sync::atomic
have similar semantics, and in fact their doc comments are macro-generated under the hood.
Maybe we could say something like:
$atomic
has the same size and bit validity as$native
- All of the bytes of
$atomic
are covered byUnsafeCell
s
src/lib.rs
Outdated
/// SAFETY: | ||
/// Per [1], `AtomicBool`, `AtomicU8`, and `AtomicI8` have an alignment of 1. | ||
/// | ||
/// [1] Per https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicBool.html: | ||
/// | ||
/// This type has the same size, alignment, and bit validity as [the native type]. |
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.
/// SAFETY: | |
/// Per [1], `AtomicBool`, `AtomicU8`, and `AtomicI8` have an alignment of 1. | |
/// | |
/// [1] Per https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicBool.html: | |
/// | |
/// This type has the same size, alignment, and bit validity as [the native type]. | |
/// SAFETY: | |
/// Per [1], `AtomicBool`, `AtomicU8`, and `AtomicI8` have an alignment of 1. | |
/// | |
/// [1] Per [2], [3], and [4]: | |
/// | |
/// This type has the same size, alignment, and bit validity as [the native type]. | |
/// | |
/// [2] https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicBool.html | |
/// [3] https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicU8.html | |
/// [4] https://doc.rust-lang.org/nightly/core/sync/atomic/struct.AtomicI8.html |
I know we've been fast-and-loose with this in the past, but I'm trying to move us towards being more precise with our safety comments.
src/lib.rs
Outdated
#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "8")))] | ||
mod atomic_8 { | ||
use super::*; | ||
unsafe_impl_traits_for_atomics!(AtomicBool[bool], AtomicU8[u8], AtomicI8[i8]); |
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.
Safety comment, and put inside a safety_comment! { ... }
block?
src/lib.rs
Outdated
#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "32")))] | ||
mod atomic_32 { | ||
use super::*; | ||
unsafe_impl_traits_for_atomics!(AtomicU32[u32], AtomicI32[i32]); |
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.
Safety comment, and put inside a safety_comment! { ... }
block?
src/lib.rs
Outdated
#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "64")))] | ||
mod atomic_64 { | ||
use super::*; | ||
unsafe_impl_traits_for_atomics!(AtomicU64[u64], AtomicI64[i64]); |
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.
Safety comment, and put inside a safety_comment! { ... }
block?
src/lib.rs
Outdated
#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "ptr")))] | ||
mod atomic_ptr { | ||
use super::*; | ||
unsafe_impl_traits_for_atomics!(AtomicUsize[usize], AtomicIsize[isize]); |
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.
Safety comment, and put inside a safety_comment! { ... }
block?
src/lib.rs
Outdated
/// `AtomicPtr<T>`` is garunteed to wrap a `*mut T`. Just like *mut T, we | ||
/// don't implement FromBytes/IntoBytes. | ||
/// | ||
/// [1] TODO(#896), TODO(https://github.com/rust-lang/rust/pull/121943): | ||
/// Cite docs once they've landed. | ||
/// | ||
/// [2] Per https://doc.rust-lang.org/reference/type-layout.html#size-and-alignment: | ||
/// | ||
/// Alignment is measured in bytes, and must be at least 1. | ||
/// | ||
/// [3] Per https://doc.rust-lang.org/reference/type-layout.html#size-and-alignment: | ||
/// | ||
/// The size of a value is always a multiple of its alignment. | ||
unsafe_impl!(AtomicBool: Unaligned); | ||
unsafe_impl!(AtomicU8: Unaligned); | ||
unsafe_impl!(AtomicI8: Unaligned); | ||
assert_unaligned!(AtomicBool, AtomicU8, AtomicI8); | ||
/// See the comments on the `*mut T` impls for more information. |
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.
This safety comment smells to me. We shouldn't have to talk about FromBytes
/IntoBytes
in order to justify this impl - we should only need to reference the safety invariants on util::Atomic
.
ptr.cast::<$atomic>() | ||
} | ||
}; | ||
/// `Self` must be an atomic type whose native equivalent is `Self::Native`. |
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.
Same here as in lib.rs
regarding the safety invariant. Ideally this safety invariant should be sufficient to prove that the following impl (unsafe impl<A: Atomic, I: Invariants> TransparentWrapper<I> for A
) is sound without reference to the stdlib documentation.
I know that I'm asking for a higher quality bar than we have on main
already 🙂 This refactor is making it so that the safety reasoning is more "at a distance", so I'm a bit more worried about making the safety reasoning precise and watertight.
Signed-off-by: Joe Richey <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1091 +/- ##
=======================================
Coverage 87.71% 87.71%
=======================================
Files 15 15
Lines 5138 5138
=======================================
Hits 4507 4507
Misses 631 631 ☔ View full report in Codecov by Sentry. |
In the process of rebasing and updating this PR, I discovered that some unsoundness in the implementation. The |
Previously, `impl_for_transparent_wrapper!` permitted the following unsound code to compile: impl_for_transparent_wrapper!(FromBytes for AtomicBool); `impl_for_transparent_wrapper!` attempted to ensure that `AtomicBool`'s inner type implemented `FromBytes`, but failed to properly enforce this bound (it should have failed thanks to `bool: !FromBytes`). This commit simplifies `impl_for_transparent_wrapper!` and fixes this soundness hole. This fix is adapted from @josephlr's similar fix in #1091. Credit to @josephlr for discovering this soundness hole. Co-authored-by: Joe Richey <[email protected]>
Previously, `impl_for_transparent_wrapper!` permitted the following unsound code to compile: impl_for_transparent_wrapper!(FromBytes for AtomicBool); `impl_for_transparent_wrapper!` attempted to ensure that `AtomicBool`'s inner type implemented `FromBytes`, but failed to properly enforce this bound (it should have failed thanks to `bool: !FromBytes`). This commit simplifies `impl_for_transparent_wrapper!` and fixes this soundness hole. This fix is adapted from @josephlr's similar fix in #1091. Credit to @josephlr for discovering this soundness hole. Co-authored-by: Joe Richey <[email protected]>
I've carved the soundness fix for |
Previously, `impl_for_transparent_wrapper!` permitted the following unsound code to compile: impl_for_transparent_wrapper!(FromBytes for AtomicBool); `impl_for_transparent_wrapper!` attempted to ensure that `AtomicBool`'s inner type implemented `FromBytes`, but failed to properly enforce this bound (it should have failed thanks to `bool: !FromBytes`). This commit simplifies `impl_for_transparent_wrapper!` and fixes this soundness hole. This fix is adapted from @josephlr's similar fix in #1091. Credit to @josephlr for discovering this soundness hole. Co-authored-by: Joe Richey <[email protected]>
Note that `cfg(target_has_atomic = ...)` was only added in Rust 1.60. However, we do not add a version detection feature for this since, prior to 1.60, these cfgs are not present, so none of the atomics will be supported. This is identical to the behavior if we were to add version detection, so there is no point in doing so. This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
I've carved out the |
Note that `cfg(target_has_atomic = ...)` was only added in Rust 1.60. However, we do not add a version detection feature for this since, prior to 1.60, these cfgs are not present, so none of the atomics will be supported. This is identical to the behavior if we were to add version detection, so there is no point in doing so. This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
IIUC, this isn't quite true - |
Previously, `impl_for_transparent_wrapper!` permitted the following unsound code to compile: impl_for_transparent_wrapper!(FromBytes for AtomicBool); `impl_for_transparent_wrapper!` attempted to ensure that `AtomicBool`'s inner type implemented `FromBytes`, but failed to properly enforce this bound (it should have failed thanks to `bool: !FromBytes`). This commit simplifies `impl_for_transparent_wrapper!` and fixes this soundness hole. This fix is adapted from @josephlr's similar fix in #1091. Credit to @josephlr for discovering this soundness hole. Co-authored-by: Joe Richey <[email protected]>
This is adapted from @josephlr's similar implementation in #1091. Fixes #1086 Co-authored-by: Joe Richey <[email protected]>
Fixes #1086
Note: this makes the
Atomic*
impl
s require Rust 1.60 or later.The atomic
impl
s are moved to their ownatomics
module, and now traits are only implemented on types likeAtomicU32
if#[cfg(target_has_atomic = "32")]
.doc_cfg
attributes are also added so that the generated documentation notes this restriction:We also make sure to not implement
AtomicBool: FromBytes
.This also fixes a bug in the
impl_for_transparent_wrapper!
macro where code like:would not emit a
UnsafeCell<bool>: FromBytes
trait bound. This resulted in #1028 being unsound. This is fixed by:$ty::Inner: $trait
bound