add unfinished safety comments for initialization and pinning macros #61

liamjdavis · 2025-05-28T07:35:21Z

The changes replace placeholder "SAFETY: TODO" comments with detailed explanations of the safety guarantees.

Improvements

src/__internal.rs:
- Added detailed safety explanations for AllData<T> and HasInitData implementations.
src/lib.rs:
- Updated safety comments for stack_try_pin_init!
- Clarified safety guarantees for Init<T, E> and PinInit<T, E> implementations.
src/macros.rs:
- Enhanced safety comments for __pinned_drop and __pin_data macros.[1] [2] [3] [4]
- Improved documentation for __init_internal macro. [1] [2] [3]

BennoLossin

Hi, thanks for the PR!

Have you already read the contributing guidelines? You will need to give your commit a meaningful commit message and add your Signed-off-by tag, since this commit will end up in the Linux kernel.

I had already improved the safety comments for the blanket impls of [Pin]Init in this commit, but that isn't merged yet. Since that also touches the surrounding area, I'd prefer to take that, so please remove those changes from the PR, thanks!

I left a couple comments to improve the PR. Some safety comments in src/macros.rs are pretty difficult, since they require a lot of justifications and need to refer to complex parts of the declarative macro. If you feel like you don't get closer to the correct answer, let me know then I'll write it up.

src/__internal.rs

src/lib.rs

src/macros.rs

liamjdavis · 2025-05-30T16:50:01Z

Thanks for the comments! I'll get to work on them.

BennoLossin

Could you separate the changes into multiple commits? Logically related changes should be in the same one (you can have commits that combine several smaller changes that are unrelated, but this one is too big).

BennoLossin · 2025-06-05T16:07:39Z

src/__internal.rs

-/// Only the `init` module is allowed to use this trait.
+/// Implementers (typically the `#[pin_data]` macro) must ensure that this method
+/// returns a `PinData` instance that accurately reflects the structural pinning
+/// and field layout of `Self`. Incorrect metadata can lead to undefined behavior
+/// when used by the `pin-init` macros.


This trait should only be implement by the #[pin_data] macro. Same for the other traits in this file. Also please use bullet points for safety sections with multiple requirements (one requirement per point). I'm not sure we want all of the internal workings written down in the safety comment here. I don't really see the value in that.

BennoLossin · 2025-06-05T16:12:18Z

src/lib.rs

-// SAFETY: Every type can be initialized by-value.
+// SAFETY: TODO.
 unsafe impl<T, E> Init<T, E> for T {
    unsafe fn __init(self, slot: *mut T) -> Result<(), E> {
-        // SAFETY: TODO.
+        // SAFETY: `slot` points to a valid, uninitialized memory of the correct size and alignment for type `T`.
        unsafe { slot.write(self) };
        Ok(())
    }
 }

-// SAFETY: Every type can be initialized by-value. `__pinned_init` calls `__init`.
+// SAFETY: TODO.
 unsafe impl<T, E> PinInit<T, E> for T {
    unsafe fn __pinned_init(self, slot: *mut T) -> Result<(), E> {
-        // SAFETY: TODO.
+        // SAFETY: `self` is a valid value of type `T`, and all requirements for `__init` are met.


please remove these changes as discussed last time.

BennoLossin · 2025-06-05T16:13:56Z

src/macros.rs

+                /// The caller must ensure that `slot` is a valid pointer to uninitialized memory
+                /// that is properly aligned and large enough to hold a value of type `$p_type`.


Suggested change

/// The caller must ensure that `slot` is a valid pointer to uninitialized memory

/// that is properly aligned and large enough to hold a value of type `$p_type`.

/// `slot` is a pointer valid for writes and any value written to it is pinned.

BennoLossin · 2025-06-05T16:14:26Z

src/macros.rs

+                /// The caller must ensure that `slot` is a valid pointer to uninitialized memory
+                /// that is properly aligned and large enough to hold a value of type `$type`.
+                /// The memory at `slot` must also be valid for writes.


Suggested change

/// The caller must ensure that `slot` is a valid pointer to uninitialized memory

/// that is properly aligned and large enough to hold a value of type `$type`.

/// The memory at `slot` must also be valid for writes.

/// `slot` is valid for writes.

BennoLossin · 2025-06-05T16:15:20Z

src/__internal.rs

 pub unsafe trait HasInitData {
    type InitData: InitData;

-    #[expect(clippy::missing_safety_doc)]


Please add a # Safety section on this function.

BennoLossin · 2025-06-05T16:17:24Z

src/macros.rs

+        // SAFETY: The `$get_data()` call (which resolves to either `HasPinData::__pin_data()`
+        // or `HasInitData::__init_data()`) is safe because the `#[pin_data]` macro, when applied
+        // to type `$t`, correctly implements the respective trait. This ensures that the returned
+        // metadata accurately reflects `$t`'s structure and pinning properties, fulfilling the
+        // `# Safety` contract of the called `__pin_data()` or `__init_data()` method.
+        // The `paste!` macro is used for re-tokenization and does not affect this safety argument.


there is no # Safety section on __pin_data or __init_data, so this comment has nothing to refer to. please add such a section and then mention the correct safety requirements there and the correct justification here.

The requirement should be along the lines "only called by macros in the pin-init crate".

BennoLossin · 2025-06-05T16:19:49Z

Also please make sure the CI is succeeding, thanks!

liamjdavis · 2025-07-18T23:25:20Z

I'm not sure why it says there are merge conflicts, I think all of them should be resolved. I'm not sure why the CI isn't running, either.

BennoLossin · 2025-07-19T21:26:38Z

your fork is 14 commits behind the main branch, so you just need to rebase it and then the CI should start

BennoLossin · 2025-07-19T21:32:01Z

also please give your commits a description of why we need the change & what you did. Please follow the usual process for the kernel.

liamjdavis · 2025-07-20T01:12:43Z

Forgot to sync my fork, that's my bad

BennoLossin · 2025-07-20T14:32:52Z

the 8 commits that you want to merge include a merge commit, but it shouldn't backmerge the changes from main (eg the CI doesn't handle it correctly). Can you put all of your commits directly on top of main? Also many of your commits don't have a description. Thanks!

BennoLossin · 2025-07-22T18:34:35Z

FYI, the clippy failure also happens in #73 so no worries about that one. I'm currently pressed for time, so I won't be fixing it for the next two weeks.

liamjdavis · 2025-07-22T23:52:57Z

Thanks, that's good to know. The issue with the OS checks is that the UI tests compares exact compiler output, which differs for each OS. How should I handle this?

BennoLossin · 2025-07-23T10:11:49Z

I decided to fix the CI & the clippy warning now, so just try to rebase and see if it passes. Note that the CI tests each commit individually, so if you changed something that's only fixed in a later commit, that won't pass the CI.

Add comprehensive safety documentation to pin-init macros, particularly for the #[pin_data] macro which generates unsafe accessor functions. The enhanced documentation clarifies: - Safety requirements for slot pointer validity and alignment - Pinning invariants that must be upheld by implementers - Memory safety contracts for macro-generated unsafe functions - Proper usage patterns for the pin-init API This addresses gaps in the safety documentation that made it difficult for users to understand the safety contracts when implementing custom initializers or working with the generated code. Also updates test expectations to match the new macro-generated documentation. Signed-off-by: Liam Davis <davisliam123@gmail.com>

BennoLossin requested changes May 28, 2025

View reviewed changes

liamjdavis force-pushed the liamjdavis/incomplete-safety-documentation branch from b644bd3 to 9493caa Compare May 31, 2025 06:34

liamjdavis requested a review from BennoLossin May 31, 2025 06:37

BennoLossin requested changes Jun 5, 2025

View reviewed changes

liamjdavis requested a review from BennoLossin July 18, 2025 23:24

liamjdavis force-pushed the liamjdavis/incomplete-safety-documentation branch 2 times, most recently from 995e722 to 2ad31d9 Compare July 22, 2025 18:24

liamjdavis force-pushed the liamjdavis/incomplete-safety-documentation branch 2 times, most recently from 62df219 to 8c27f55 Compare July 23, 2025 16:01

liamjdavis force-pushed the liamjdavis/incomplete-safety-documentation branch from 8c27f55 to 45a49e6 Compare July 24, 2025 19:47

		/// The caller must ensure that `slot` is a valid pointer to uninitialized memory
		/// that is properly aligned and large enough to hold a value of type `$p_type`.

	/// The caller must ensure that `slot` is a valid pointer to uninitialized memory
	/// that is properly aligned and large enough to hold a value of type `$p_type`.
	/// `slot` is a pointer valid for writes and any value written to it is pinned.

add unfinished safety comments for initialization and pinning macros #61

Are you sure you want to change the base?

add unfinished safety comments for initialization and pinning macros #61

Uh oh!

Conversation

liamjdavis commented May 28, 2025

Improvements

Uh oh!

BennoLossin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamjdavis commented May 30, 2025

Uh oh!

BennoLossin left a comment

Choose a reason for hiding this comment

Uh oh!

BennoLossin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

BennoLossin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

BennoLossin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

BennoLossin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

BennoLossin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

BennoLossin Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

BennoLossin commented Jun 5, 2025

Uh oh!

liamjdavis commented Jul 18, 2025

Uh oh!

BennoLossin commented Jul 19, 2025

Uh oh!

BennoLossin commented Jul 19, 2025

Uh oh!

liamjdavis commented Jul 20, 2025

Uh oh!

BennoLossin commented Jul 20, 2025

Uh oh!

BennoLossin commented Jul 22, 2025

Uh oh!

liamjdavis commented Jul 22, 2025

Uh oh!

BennoLossin commented Jul 23, 2025

Uh oh!

Uh oh!