added prefix transmute at transmute! #2024

Aditya-PS-05 · 2024-11-05T16:15:27Z

codecov-commenter · 2024-11-05T17:31:21Z

Codecov Report

Attention: Patch coverage is 74.07407% with 21 lines in your changes missing coverage. Please review.

Project coverage is 89.30%. Comparing base (ace5bb0) to head (92f1b6d).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
src/util/macro_util.rs	69.11%	21 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2024      +/-   ##
==========================================
- Coverage   89.46%   89.30%   -0.17%     
==========================================
  Files          16       16              
  Lines        5838     5945     +107     
==========================================
+ Hits         5223     5309      +86     
- Misses        615      636      +21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Aditya-PS-05 · 2024-11-06T05:37:56Z

@joshlf , please review it

jswrenn · 2024-11-06T17:27:11Z

src/macros.rs

+/// Conditionally transmutes a value of one type to a prefix-compatible value of
+/// another type, allowing the destination type to be smaller than the source type.
+///
+/// This macro provides prefix transmutation functionality, where the destination
+/// type's size can be smaller than or equal to the source type's size. The types
+/// `Src` and `Dst` must be explicitly specified in the macro invocation, allowing
+/// it to verify that the prefix of `Src` is compatible with `Dst`.
+///
+/// # Syntax
+/// ```ignore
+/// try_transmute_prefix!(SrcType, DstType, value_expr)
+/// ```
+///
+/// # Parameters
+/// - `SrcType`: The source type of the transmute. Must implement `IntoBytes`.
+/// - `DstType`: The destination type for the transmute. Must implement `FromBytes`.
+/// - `value_expr`: The expression producing the `SrcType` value to be transmuted.
+///
+/// In this prefix transmutation, the `SrcType` value will not be dropped.
+/// Instead, its bits will be copied into a new value of type `DstType`,
+/// allowing for compatibility when `size_of::<SrcType>() >= size_of::<DstType>()`.
+///
+/// # Safety
+/// This macro performs an unsafe operation, using a union to verify size
+/// relationships. It will only compile if `size_of::<SrcType>() >= size_of::<DstType>()`,
+/// ensuring safe prefix compatibility.
+///
+/// # Examples
+///
+/// ```
+/// # use zerocopy::*;
+/// let large_array: [u8; 5] = [0, 1, 2, 3, 4];
+/// let result: Result<[u8; 2], _> = try_transmute_prefix!([u8; 5], [u8; 2], large_array);
+/// assert!(result.is_ok());
+/// assert_eq!(result.unwrap(), [0, 1]);
+///
+/// let large_num: u64 = 0x0102030405060708;
+/// let result: Result<u32, _> = try_transmute_prefix!(u64, u32, large_num);
+/// assert!(result.is_ok());
+/// assert_eq!(result.unwrap(), 0x05060708);
+/// ```
+///
+#[macro_export]
+macro_rules! try_transmute_prefix {
+    ($src_type:ty, $dest_type:ty, $e:expr) => {{
+        let e: $src_type = $e;
+        if false {
+            // Check that the source size is greater than or equal to destination size
+
+            // SAFETY: This code is never executed.
+            Ok(unsafe {
+                let union_val = $crate::util::macro_util::core_reexport::mem::transmute::<$src_type, $crate::util::macro_util::MaxSizesOf<$src_type, $dest_type>>(e);
+                $crate::util::macro_util::core_reexport::mem::transmute_copy(&union_val.source)
+            })
+        } else {
+            $crate::util::macro_util::try_transmute_prefix::<$src_type, $dest_type>(e)
+        }
+    }};
+}
+


I don't think we want to introduce another distinct macro here; rather, we want try_transmute to support the @prefix notation.

I added this just to test the changes and I thought it would be better to keep this.

jswrenn · 2024-11-06T17:28:03Z

src/macros.rs

+            // Create a union to verify size relationships at compile time
+            #[repr(C)]
+            union MaxSizesOf<T, U> {
+                source: T,
+                _dest: U,
+            }


You define MaxSizesOf this in macro_util.rs, so you don't need to redefine it here.

jswrenn · 2024-11-06T17:38:53Z

src/macros.rs

+                // First transmute to union to verify size relationships
+                let union_val = $crate::util::macro_util::core_reexport::mem::transmute::<_, MaxSizesOf<_, _>>(e);


Can size verification go in a else if false branch so we don't pay the runtime cost of a double transmutation?

Okay, I will resolve these changes.

jswrenn · 2024-11-06T17:40:19Z

src/macros.rs

+// #[macro_export]
+// macro_rules! transmute {
+//     ($e:expr) => {{
+//         // NOTE: This must be a macro (rather than a function with trait bounds)
+//         // because there's no way, in a generic context, to enforce that two
+//         // types have the same size. `core::mem::transmute` uses compiler magic
+//         // to enforce this so long as the types are concrete.
+//         let e = $e;
+//         if false {
+//             // This branch, though never taken, ensures that the type of `e` is
+//             // `IntoBytes` and that the type of this macro invocation expression
+//             // is `FromBytes`.
+//             struct AssertIsIntoBytes<T: $crate::IntoBytes>(T);
+//             let _ = AssertIsIntoBytes(e);
+//             struct AssertIsFromBytes<U: $crate::FromBytes>(U);
+//             #[allow(unused, unreachable_code)]
+//             let u = AssertIsFromBytes(loop {});
+//             u.0
+//         } else {
+//             // SAFETY: `core::mem::transmute` ensures that the type of `e` and
+//             // the type of this macro invocation expression have the same size.
+//             // We know this transmute is safe thanks to the `IntoBytes` and
+//             // `FromBytes` bounds enforced by the `false` branch.
+//             //
+//             // We use this reexport of `core::mem::transmute` because we know it
+//             // will always be available for crates which are using the 2015
+//             // edition of Rust. By contrast, if we were to use
+//             // `std::mem::transmute`, this macro would not work for such crates
+//             // in `no_std` contexts, and if we were to use
+//             // `core::mem::transmute`, this macro would not work in `std`
+//             // contexts in which `core` was not manually imported. This is not a
+//             // problem for 2018 edition crates.
+//             let u = unsafe {
+//                 // Clippy: We can't annotate the types; this macro is designed
+//                 // to infer the types from the calling context.
+//                 #[allow(clippy::missing_transmute_annotations)]
+//                 $crate::util::macro_util::core_reexport::mem::transmute(e)
+//             };
+//             $crate::util::macro_util::must_use(u)
+//         }
+//     }}
+// }
+///
 #[macro_export]


Did you mean to delete this?

I am changing this also.

Aditya-PS-05 · 2024-11-12T13:39:28Z

@jswrenn , I request you to review this pr.

jswrenn · 2024-11-12T15:11:44Z

This PR has huge swaths of commented-out code. As a rule, we don't leave commented-out code in the zerocopy codebase.

jswrenn · 2024-11-13T15:35:44Z

src/macros.rs

-    }}
+    }};
+
+    (@prefix $src_type:ty, $dest_type:ty, $e:expr) => {{


We'd like the types to be inferred at the callsite (consistent with our non-prefix transmutations), rather than supplied explicitly.

I made try_transmute as

#[macro_export] macro_rules! try_transmute { ($e:expr) => {{ let e = $e; if false { // SAFETY: `core::mem::transmute` ensures that the type of `e` and // the type of this macro invocation expression have the same size. // This check is enforced by the false branch, so the actual transmute // operation is safe. Ok(unsafe { #[allow(clippy::missing_transmute_annotations)] $crate::util::macro_util::core_reexport::mem::transmute(e) }) } else { $crate::util::macro_util::try_transmute(e) } }}; (@prefix $e:expr) => {{ let e = $e; if false { #[allow(clippy::missing_transmute_annotations)] // SAFETY: This code is never executed, and only verifies size // compatibility at compile time using `MaxSizesOf`. Thus, no runtime // transmute is actually performed here. let _ = unsafe { $crate::util::macro_util::core_reexport::mem::transmute::<_, $crate::util::macro_util::MaxSizesOf<_, _>>(e) }; // SAFETY: The above transmute check ensures that the prefix transmute // is safe, as `core::mem::transmute_copy` will only copy valid bytes. Ok(unsafe { $crate::util::macro_util::core_reexport::mem::transmute_copy(&e) }) } else { $crate::util::macro_util::try_transmute(e) } }}; }

When I write a test to try it out Type annotations needed cannot infer type error is occuring.

let _ = unsafe { $crate::util::macro_util::core_reexport::mem::transmute::<_, $crate::util::macro_util::MaxSizesOf<_, _>>(e) };

@jswrenn , should I avoid writing a test?

jswrenn · 2024-11-13T15:36:29Z

src/util/macro_util.rs

+// Helper trait for static assertions
+#[allow(dead_code)]
+trait StaticAssert {
+    fn assert() -> bool;
+}
+
+// Implementation to verify size relationships
+impl<Src, Dst> StaticAssert for (Src, Dst)
+where
+    Src: Sized,
+    Dst: Sized,
+{
+    fn assert() -> bool {
+        mem::size_of::<Src>() >= mem::size_of::<Dst>()
+    }
+}


This is dead code; remove it.

jswrenn · 2024-11-13T15:37:14Z

src/util/macro_util.rs

+// Macro for static assertions
+#[inline]
+pub fn try_transmute_prefix<Src, Dst>(src: Src) -> Result<Dst, ValidityError<Src, Dst>>
+where
+    Src: IntoBytes,
+    Dst: TryFromBytes,
+{
+    // Ensure source size is greater than or equal to destination size
+    if mem::size_of::<Src>() < mem::size_of::<Dst>() {
+        return Err(ValidityError::new(src));
+    }
+
+    let mut src = ManuallyDrop::new(src);
+    let ptr = Ptr::from_mut(&mut src);
+
+    // Similar to try_transmute, but we handle partial reads
+    match try_cast_prefix_or_pme::<_, ManuallyDrop<Unalign<Dst>>, _, BecauseExclusive>(ptr) {
+        Ok(ptr) => {
+            let dst = ptr.bikeshed_recall_aligned().as_mut();
+            // SAFETY: By shadowing `dst`, we ensure that `dst` is not re-used
+            // after taking its inner value.
+            let dst = unsafe { ManuallyDrop::take(dst) };
+            Ok(dst.into_inner())
+        }
+        Err(_) => Err(ValidityError::new(ManuallyDrop::into_inner(src))),
+    }
+}


We don't want to introduce any new public functions or macros in our API for prefix transmutation.

jswrenn · 2024-11-13T15:37:38Z

src/util/macro_util.rs

@@ -619,7 +719,7 @@ where
 {
    match try_cast_or_pme::<Src, Dst, _, BecauseImmutable>(Ptr::from_ref(src)) {
        Ok(ptr) => {
-            static_assert!(Src, Dst => mem::align_of::<Dst>() <= mem::align_of::<Src>());
+            assert!(mem::align_of::<Dst>() <= mem::align_of::<Src>());


This changes the semantics of the function. It's important that the assertion as at post-monomorphization time.

jswrenn · 2024-11-13T15:37:47Z

src/util/macro_util.rs

@@ -649,7 +749,7 @@ where
 {
    match try_cast_or_pme::<Src, Dst, _, BecauseExclusive>(Ptr::from_mut(src)) {
        Ok(ptr) => {
-            static_assert!(Src, Dst => mem::align_of::<Dst>() <= mem::align_of::<Src>());


This changes the semantics of the function. It's important that the assertion as at post-monomorphization time.

Aditya-PS-05 force-pushed the add/1852-prefix-transmutes branch 2 times, most recently from 7c86112 to 7ace93d Compare November 5, 2024 17:24

jswrenn requested changes Nov 6, 2024

View reviewed changes

jswrenn reviewed Nov 6, 2024

View reviewed changes

Aditya-PS-05 force-pushed the add/1852-prefix-transmutes branch 3 times, most recently from 95bff4f to 6738594 Compare November 7, 2024 11:31

Aditya-PS-05 requested a review from jswrenn November 7, 2024 11:37

Aditya-PS-05 force-pushed the add/1852-prefix-transmutes branch 2 times, most recently from 4bf9ee7 to 5cd9285 Compare November 13, 2024 06:05

added prefix transmute

92f1b6d

Aditya-PS-05 force-pushed the add/1852-prefix-transmutes branch from 5cd9285 to 92f1b6d Compare November 13, 2024 06:07

jswrenn reviewed Nov 13, 2024

View reviewed changes

jswrenn requested changes Nov 13, 2024

View reviewed changes

Aditya-PS-05 requested a review from jswrenn November 15, 2024 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added prefix transmute at transmute! #2024

added prefix transmute at transmute! #2024

Aditya-PS-05 commented Nov 5, 2024

codecov-commenter commented Nov 5, 2024 •

edited

Loading

Aditya-PS-05 commented Nov 6, 2024

jswrenn Nov 6, 2024

Aditya-PS-05 Nov 6, 2024

jswrenn Nov 6, 2024

Aditya-PS-05 Nov 6, 2024

jswrenn Nov 6, 2024

Aditya-PS-05 Nov 6, 2024

jswrenn Nov 6, 2024

Aditya-PS-05 Nov 7, 2024

Aditya-PS-05 commented Nov 12, 2024

jswrenn commented Nov 12, 2024

jswrenn Nov 13, 2024

Aditya-PS-05 Nov 13, 2024

Aditya-PS-05 Nov 13, 2024

jswrenn Nov 13, 2024

jswrenn Nov 13, 2024

jswrenn Nov 13, 2024

jswrenn Nov 13, 2024

		// First transmute to union to verify size relationships
		let union_val = $crate::util::macro_util::core_reexport::mem::transmute::<_, MaxSizesOf<_, _>>(e);

added prefix transmute at transmute! #2024

Are you sure you want to change the base?

added prefix transmute at transmute! #2024

Conversation

Aditya-PS-05 commented Nov 5, 2024

codecov-commenter commented Nov 5, 2024 • edited Loading

Codecov Report

Aditya-PS-05 commented Nov 6, 2024

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

Aditya-PS-05 commented Nov 12, 2024

jswrenn commented Nov 12, 2024

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 Nov 5, 2024 •

edited

Loading