Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 #1091

josephlr · 2024-04-04T05:01:16Z

Note: this makes the Atomic* impls require Rust 1.60 or later.

The atomic impls are moved to their own atomics module, and now traits are only implemented on types like AtomicU32 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:

impl_for_transparent_wrapper!(FromBytes for AtomicBool);

would not emit a UnsafeCell<bool>: FromBytes trait bound. This resulted in #1028 being unsound. This is fixed by:

Simplifying the macro (no more duplicated code)
adding a $ty::Inner: $trait bound

josephlr · 2024-04-04T05:06:43Z

@joshlf I can split out the impl_for_transparent_wrapper! fix into a separate PR if you want.

josephlr · 2024-04-04T07:56:38Z

@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:

TRYBUILD=overwrite ./cargo.sh +nightly test --workspace --all-features
TRYBUILD=overwrite ./cargo.sh +stable test --workspace --features=__internal_use_only_features_that_work_on_stable
TRYBUILD=overwrite ./cargo.sh +msrv test --workspace --features=__internal_use_only_features_that_work_on_stable

(not TRYBUILD=overwrite ./cargo.sh +all test --workspace like some of the documentation suggests).

It might be worth noting the correct procedure in CONTRIBUTING.md or INTERNAL.md, or adding an easier way to run the "correct" UI tests.

josephlr · 2024-04-30T02:11:13Z

@joshlf merge conflict has been resolved, let me know if you want me to change anything here or split the PR.

joshlf

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!

src/atomics.rs

joshlf · 2024-05-03T09:49:48Z

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 main (otherwise it's a GitHub-generated amalgam of different commit messages smushed together).

joshlf · 2024-05-03T18:00:01Z

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?

josephlr · 2024-05-08T12:02:00Z

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 main (otherwise it's a GitHub-generated amalgam of different commit messages smushed together).

Should now be a single commit

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?

Code moved back to lib.rs and util.rs.

@joshlf this should be ready for another look. Note that I changed how the atomic types implement TransparentWrapper. Now the atomic types implement a much simpler util::Atomic, and then use a generic impl<A: Atomic> TransparentWrapper. This allows us to have simpler macros and less generated code.

josephlr · 2024-05-08T12:20:52Z

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

joshlf

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.

joshlf · 2024-05-09T15:21:37Z

Cargo.toml

@@ -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


Suggested change

# 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

joshlf · 2024-05-09T15:28:12Z

src/lib.rs

+/// 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].


Suggested change

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

joshlf · 2024-05-09T15:32:27Z

src/lib.rs

+///
+/// # Safety
+///
+/// `$atomic` must be an atomic type with corresponding native type `$native`.


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 by UnsafeCells

joshlf · 2024-05-09T15:35:28Z

src/lib.rs

+        /// 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].


Suggested change

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

joshlf · 2024-05-09T15:36:28Z

src/lib.rs

+#[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]);


Safety comment, and put inside a safety_comment! { ... } block?

joshlf · 2024-05-09T15:38:56Z

src/lib.rs

+#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "32")))]
+mod atomic_32 {
+    use super::*;
+    unsafe_impl_traits_for_atomics!(AtomicU32[u32], AtomicI32[i32]);


Safety comment, and put inside a safety_comment! { ... } block?

joshlf · 2024-05-09T15:39:00Z

src/lib.rs

+#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "64")))]
+mod atomic_64 {
+    use super::*;
+    unsafe_impl_traits_for_atomics!(AtomicU64[u64], AtomicI64[i64]);


Safety comment, and put inside a safety_comment! { ... } block?

joshlf · 2024-05-09T15:39:04Z

src/lib.rs

+#[cfg_attr(doc_cfg, doc(cfg(target_has_atomic = "ptr")))]
+mod atomic_ptr {
+    use super::*;
+    unsafe_impl_traits_for_atomics!(AtomicUsize[usize], AtomicIsize[isize]);


Safety comment, and put inside a safety_comment! { ... } block?

joshlf · 2024-05-09T15:40:17Z

src/lib.rs

+    /// `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.


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.

joshlf · 2024-05-09T15:43:26Z

src/util.rs

-            ptr.cast::<$atomic>()
-        }
-    };
+/// `Self` must be an atomic type whose native equivalent is `Self::Native`.


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 <joerichey@google.com>

codecov-commenter · 2024-05-26T11:52:15Z

Codecov Report

Attention: Patch coverage is 17.64706% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 87.71%. Comparing base (76915d0) to head (f366da5).

Files	Patch %	Lines
src/util.rs	0.00%	14 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

josephlr force-pushed the atomic_gate branch from 9473ad4 to ec0051e Compare April 4, 2024 05:02

This was referenced Apr 4, 2024

Implement traits for atomic types #1028

Merged

Build failure in v0.8.0-alpha.7 on Cortex M #1086

Open

josephlr force-pushed the atomic_gate branch 2 times, most recently from a2c9b30 to a2db8f9 Compare April 4, 2024 05:32

josephlr changed the title ~~Gate implementations for Atomic* types behind #[cfg(target_has_atomic)]~~ Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 Apr 4, 2024

josephlr force-pushed the atomic_gate branch 2 times, most recently from 61cb559 to 2d66234 Compare April 4, 2024 07:14

josephlr force-pushed the atomic_gate branch 2 times, most recently from 423ad6a to 715b592 Compare April 4, 2024 23:25

josephlr force-pushed the atomic_gate branch from 715b592 to 97ecc14 Compare April 29, 2024 23:59

josephlr mentioned this pull request Apr 30, 2024

Update zerocopy to 0.8.0 alpha.8 ... rust-random/rand#1441

Closed

joshlf requested changes May 3, 2024

View reviewed changes

src/atomics.rs Outdated Show resolved Hide resolved

src/atomics.rs Outdated Show resolved Hide resolved

src/atomics.rs Outdated Show resolved Hide resolved

joshlf mentioned this pull request May 3, 2024

Document process for updating UI tests #1170

Closed

josephlr force-pushed the atomic_gate branch from 97ecc14 to 7376e9e Compare May 8, 2024 11:55

josephlr force-pushed the atomic_gate branch 2 times, most recently from cd818a1 to ffc2d84 Compare May 8, 2024 12:05

josephlr requested a review from joshlf May 8, 2024 12:07

josephlr force-pushed the atomic_gate branch from ffc2d84 to b736ce5 Compare May 9, 2024 06:46

joshlf requested changes May 9, 2024

View reviewed changes

Gate Atomic type behind the corresponding #[cfg(target_has_atomic)]

f366da5

Signed-off-by: Joe Richey <joerichey@google.com>

josephlr force-pushed the atomic_gate branch from b736ce5 to f366da5 Compare May 26, 2024 11:44

djkoloski self-assigned this May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 #1091

Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 #1091

josephlr commented Apr 4, 2024 •

edited

josephlr commented Apr 4, 2024

josephlr commented Apr 4, 2024 •

edited

josephlr commented Apr 30, 2024

joshlf left a comment

joshlf commented May 3, 2024

joshlf commented May 3, 2024

josephlr commented May 8, 2024

josephlr commented May 8, 2024

joshlf left a comment

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

joshlf May 9, 2024

codecov-commenter commented May 26, 2024

	# 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

Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 #1091

Are you sure you want to change the base?

Gate implementations for Atomic* types behind #[cfg(target_has_atomic)] and Rust 1.60 #1091

Conversation

josephlr commented Apr 4, 2024 • edited

josephlr commented Apr 4, 2024

josephlr commented Apr 4, 2024 • edited

josephlr commented Apr 30, 2024

joshlf left a comment

Choose a reason for hiding this comment

joshlf commented May 3, 2024

joshlf commented May 3, 2024

josephlr commented May 8, 2024

josephlr commented May 8, 2024

joshlf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 26, 2024

Codecov Report

josephlr commented Apr 4, 2024 •

edited

josephlr commented Apr 4, 2024 •

edited