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

docs(cordyceps): use ptr::addr_of_mut! instead of casts #258

Merged
merged 3 commits into from
Jul 20, 2022
Merged

Conversation

hawkw
Copy link
Owner

@hawkw hawkw commented Jul 20, 2022

Currently, the cordyceps tests and docs show the use of a #[repr(C)]
struct with the Links field as the first member of the struct, and use
a cast from NonNull<Self> to NonNull<Links<Self>> in order to
implement the Linked::links method. This is done to avoid the creation
of a temporary reference by dereferencing the pointer and borrowing the
Links field, which Stacked Borrows dislikes.

This approach is not ideal for a couple of reasons:

  • It performs a layout-dependent cast, and therefore requires the
    #[repr(C)] attribute be present to ensure soundness. This means
    that forgetting to add that attribute may result in miscompiles.
    Also, relying on struct layout makes me feel uncomfortable and we
    should try to avoid it where possible.
  • It means that a given struct may not contain multiple types of
    Links for different intrusive data structures. This is because the
    layout-dependent cast requires the Links field to be the first
    member of the struct, and multiple fields obviously cannot be the
    first. :)

We can also avoid the creation of a temporary reference by using the
core::ptr::addr_of_mut! macro to offset a pointer to a subfield of the
pointed type. This avoids the disadvantages of the cast while still
passing Miri.

Therefore, this branch updates the cordyceps examples, docs, and
tests, to show the use of addr_of_mut! rather than casting. In
addition, I've added a section to the Linked trait documentation
explaining why it's necessary to avoid temporary references, and
discussing why addr_of_mut! should be used for this purpose.

In the future, we may want to consider adding a macro (or a derive
attribute?) that generates correct Linked implementations using
addr_of_mut!, to make it easier to use intrusive structures safely...

Currently, the `cordyceps` tests and docs show the use of a `#[repr(C)]`
struct with the `Links` field as the first member of the struct, and use
a cast from `NonNull<Self>` to `NonNull<Links<Self>>` in order to
implement the `Linked::links` method. This is done to avoid the creation
of a temporary reference by dereferencing the pointer and borrowing the
`Links` field, which Stacked Borrows dislikes.

This approach is not ideal for a couple of reasons:

 * It performs a layout-dependent cast, and therefore *requires* the
   `#[repr(C)]` attribute be present to ensure soundness. This means
   that forgetting to add that attribute may result in miscompiles.
   Also, relying on struct layout makes me feel uncomfortable and we
   should try to avoid it where possible.
 * It means that a given struct may not contain multiple types of
   `Links` for different intrusive data structures. This is because the
   layout-dependent cast requires the `Links` field to be the *first*
   member of the struct, and multiple fields obviously cannot be the
   first. :)

We can also avoid the creation of a temporary reference by using the
`core::ptr::addr_of_mut!` macro to offset a pointer to a subfield of the
pointed type. This avoids the disadvantages of the cast while still
passing Miri.

Therefore, this branch updates the `cordyceps` examples, docs, and
tests, to show the use of `addr_of_mut!` rather than casting. In
addition, I've added a section to the `Linked` trait documentation
explaining why it's necessary to avoid temporary references, and
discussing why `addr_of_mut!` should be used for this purpose.

In the future, we may want to consider adding a macro (or a derive
attribute?) that generates correct `Linked` implementations using
`addr_of_mut!`, to make it easier to use intrusive structures safely...
@hawkw hawkw requested a review from jamesmunns July 20, 2022 16:44
hawkw added a commit that referenced this pull request Jul 20, 2022
This replaces a use of a layout-dependent cast on a `#[repr(C)]` type
with the use of `core::ptr::addr_of_mut!`, in the buddy block
allocator's `Linked` implementation. See #258 for details on why this is
preferable.
hawkw added a commit that referenced this pull request Jul 20, 2022
This replaces uses of layout-dependent casts on a `#[repr(C)]` type
with the use of `core::ptr::addr_of_mut!` in the `cordyceps::Linked`
implementations for various types in `maitake`. See #258 for details on
why this is preferable.

I did *not* remove all of the layout-dependent casts in the `Task`
struct's vtable methods, so that type remains `#[repr(C)]` It would be
nice to replace those with `addr_of!`/`addr_of_mut!` as well. However,
we would likely still want this type to be `#[repr(C)]` for performance
reasons, so that the `Header`, which contains its hottest fields, is
prefetched on CPUs with spatial prefetch. However, it would be nice if
we didn't *depend* on the layout for soundness.

Since that hasn't been fully addressed in this PR, though, I've instead
added a test in `maitake::task` that asserts the address of the `Header`
field is equal to the address of the `Task` struct itself. This will
fail if we make changes that inadvertantly change the layout.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw enabled auto-merge (squash) July 20, 2022 17:21
@hawkw hawkw merged commit 6e2a04c into main Jul 20, 2022
@hawkw hawkw deleted the eliza/addrof branch July 20, 2022 17:24
hawkw added a commit that referenced this pull request Jul 22, 2022
This replaces a use of a layout-dependent cast on a `#[repr(C)]` type
with the use of `core::ptr::addr_of_mut!`, in the buddy block
allocator's `Linked` implementation. See #258 for details on why this is
preferable.
hawkw added a commit that referenced this pull request Jul 22, 2022
This replaces uses of layout-dependent casts on a `#[repr(C)]` type
with the use of `core::ptr::addr_of_mut!` in the `cordyceps::Linked`
implementations for various types in `maitake`. See #258 for details on
why this is preferable.

I did *not* remove all of the layout-dependent casts in the `Task`
struct's vtable methods, so that type remains `#[repr(C)]` It would be
nice to replace those with `addr_of!`/`addr_of_mut!` as well. However,
we would likely still want this type to be `#[repr(C)]` for performance
reasons, so that the `Header`, which contains its hottest fields, is
prefetched on CPUs with spatial prefetch. However, it would be nice if
we didn't *depend* on the layout for soundness.

Since that hasn't been fully addressed in this PR, though, I've instead
added a test in `maitake::task` that asserts the address of the `Header`
field is equal to the address of the `Task` struct itself. This will
fail if we make changes that inadvertantly change the layout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant